-
Notifications
You must be signed in to change notification settings - Fork 772
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
[Traces] Add named options support to OtlpTraceExporter builder extensions #3653
[Traces] Add named options support to OtlpTraceExporter builder extensions #3653
Conversation
static OpenTelemetry.Metrics.OtlpMetricExporterExtensions.AddOtlpExporter(this OpenTelemetry.Metrics.MeterProviderBuilder builder, string name, System.Action<OpenTelemetry.Exporter.OtlpExporterOptions> configureExporter) -> OpenTelemetry.Metrics.MeterProviderBuilder | ||
*REMOVED*static OpenTelemetry.Trace.OtlpTraceExporterHelperExtensions.AddOtlpExporter(this OpenTelemetry.Trace.TracerProviderBuilder builder, System.Action<OpenTelemetry.Exporter.OtlpExporterOptions> configure = null) -> OpenTelemetry.Trace.TracerProviderBuilder |
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.
If some user was binding to this reflectively and specifically looking at metadata for the optional parameter they would no longer find it and break but I don't think that is really worth worrying about. The benefit to this change is to keep consistency with everything else and allow for future safe expansion/overload additions without default parameter ambiguity issues.
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.
If they explicitly had builder.AddOtlpExporter(null)
could nullablility cause any problems? I think the answer is no, but just asking. Basically should the new overload be
static OpenTelemetry.Trace.OtlpTraceExporterHelperExtensions.AddOtlpExporter(this OpenTelemetry.Trace.TracerProviderBuilder builder, System.Action<OpenTelemetry.Exporter.OtlpExporterOptions>? configure) -> OpenTelemetry.Trace.TracerProviderBuilder
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.
If we had nullable enabled in OpenTelemetry.Exporter.OpenTelemetryProtocol project then yes we would want the ?
. But we don't, so the existing signature accepts null as a traditional ref would. Basically null
is expected and works fine. RE: Nullable, I'm almost done annotating SDK after which I was thinking I would do API and then start branching out into other projects. It is a very slow burn 😄
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.
Oh yea right, forgot it wasn't enabled yet.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3653 +/- ##
==========================================
+ Coverage 87.62% 87.82% +0.19%
==========================================
Files 283 283
Lines 10234 10237 +3
==========================================
+ Hits 8968 8991 +23
+ Misses 1266 1246 -20
|
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.
Just a question about the new overload, but LGTM
Changes
Applies the named options pattern from #3648 to OtlpTraceExporter.
TODOs
CHANGELOG.md
updated for non-trivial changes