-
Notifications
You must be signed in to change notification settings - Fork 656
Allow requiring OpenTelemetry protocol if needed #10507
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces the ability to explicitly require a specific OpenTelemetry protocol (gRPC or HTTP/Protobuf) when configuring the OTLP exporter, including new enums, overloads, annotations, and accompanying tests.
- Defines a new
OtlpProtocol
enum and extends the annotation model withRequiredProtocol
- Adds overloads for
AddOtlpEnvironment
andWithOtlpExporter
to accept a protocol parameter - Implements protocol-specific environment-variable registration logic and verifies behavior in tests
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
File | Description |
---|---|
tests/Aspire.Hosting.Tests/WithOtlpExporterTests.cs | Adds tests for protocol selection and exception when HTTP endpoint is missing |
src/Aspire.Hosting/OtlpProtocol.cs | Introduces OtlpProtocol enum |
src/Aspire.Hosting/OtlpConfigurationExtensions.cs | Implements protocol-aware environment setup and new overloads |
src/Aspire.Hosting/ApplicationModel/OtlpExporterAnnotation.cs | Extends annotation model with nullable RequiredProtocol property |
Comments suppressed due to low confidence (2)
src/Aspire.Hosting/OtlpConfigurationExtensions.cs:44
- The
<param>
description forprotocol
should be updated to reflect that this overload requires the protocol explicitly and does not use a fallback behavior. For example: "The protocol to use for the OTLP exporter."
/// <param name="protocol">The protocol to use for the OTLP exporter. If not set, it will try gRPC then Http.</param>
src/Aspire.Hosting/ApplicationModel/OtlpExporterAnnotation.cs:15
- The summary refers to a "default protocol," but the property is
RequiredProtocol
. Consider updating to: "Gets or sets the required protocol for the OTLP exporter."
/// Gets or sets the default protocol for the OTLP exporter.
For some scenarios (mine is runnin it ASP.NET framework applications), gRPC is not supported. This change adds an overload for AddOtlpExporter APIs to have a required protocol. Since there's not a default http endpoint if one is not set, this will throw an exception pointing the user to set that variable.
/// <param name="configuration">The configuration to use for the OTLP exporter endpoint URL.</param> | ||
/// <param name="environment">The host environment to check if the application is running in development mode.</param> | ||
/// <param name="protocol">The protocol to use for the OTLP exporter. If not set, it will try gRPC then Http.</param> | ||
public static void AddOtlpEnvironment(IResource resource, IConfiguration configuration, IHostEnvironment environment, OtlpProtocol protocol) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing AddOtlpEnvironment
overload should probably call this one to reduce code duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The protocol
parameter is non nullable here and so the RequiredProtocol
is set to it, while if nothing is passed the RequiredProtocol
will be null.
I did it this way since I don't think I can add a optional nullable parameter to the existing signature, to it was the simplest I could make it without having a third private method that took a nullable protocol which didn't seem to add much value here.
For some scenarios (mine is running ASP.NET framework applications), gRPC is not supported. This change adds an overload for AddOtlpExporter APIs to have a required protocol. Since there's not a default http endpoint if one is not set, this will throw an exception pointing the user to set that variable.
Fixes #6789