[OtlpExporter] Registration extension configuration delegate fix

[CodeBlanch]

Fixes #4043

[Check out #4043 for a really good writeup of the issue.]

Changes

• Restores the 1.3 behavior (where configuration delegates were executed inline and not through Options API) for AddOtlpExporter extensions when named options are NOT used.

[alanwest]

nit: Kinda weird to have this method in OtlpTraceExporterHelperExtensions. I don't feel strongly this needs to change right now.

[CodeBlanch]

I moved the internal helper extension onto OtlpExporterOptions that seemed like a slightly better spot for it? 🤣

[codecov]

Codecov Report

Merging #4058 (f506089) into main (2359dde) will decrease coverage by 0.00%.
The diff coverage is 98.41%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4058      +/-   ##
==========================================
- Coverage   85.50%   85.49%   -0.01%     
==========================================
  Files         289      289              
  Lines       11229    11259      +30     
==========================================
+ Hits         9601     9626      +25     
- Misses       1628     1633       +5     

Impacted Files	Coverage Δ
...ryProtocol.Logs/OtlpLogExporterHelperExtensions.cs	88.23% <90.00%> (-4.63%)	⬇️
....Exporter.Jaeger/JaegerExporterHelperExtensions.cs	98.21% <100.00%> (ø)
...orter.OpenTelemetryProtocol/OtlpExporterOptions.cs	100.00% <100.00%> (ø)
...nTelemetryProtocol/OtlpMetricExporterExtensions.cs	96.61% <100.00%> (+0.86%)	⬆️
...metryProtocol/OtlpTraceExporterHelperExtensions.cs	96.07% <100.00%> (+0.95%)	⬆️
....Exporter.Zipkin/ZipkinExporterHelperExtensions.cs	98.18% <100.00%> (ø)
.../OpenTelemetry/Internal/ConfigurationExtensions.cs	100.00% <100.00%> (ø)
...lementation/SqlClientInstrumentationEventSource.cs	70.83% <0.00%> (-4.17%)	⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs	79.41% <0.00%> (-2.95%)	⬇️
...ter.ZPages/Implementation/ZPagesActivityTracker.cs	97.14% <0.00%> (-2.86%)	⬇️
... and 1 more

[julealgon]

Just read the release notes and noticed this change was in there.

• https://github.com/open-telemetry/opentelemetry-dotnet/releases/tag/core-1.4.0-rc.2

I just wanted to mention here that, unless I badly misunderstood the change, I feel like this is just not a good approach. If you run the delegate "outside of the Options mechanism", this would mean that people relying on IConfigureOptions<T> could potentially conflict with inline configurations.

The fact that you guys are doing stuff like this:

Already feels extremely bad practice: options classes should not be instantiated manually and should always be injected via IOptions<T>.

These "manually newed-up" options instances will never participate in the rest of the Options pipeline. People attempting to configure it using standard options-based mechanisms (binding to arbitrary config sections, using IConfigureOptions, etc) will be stumped by the fact it just doesn't seem to work at all.

Figured I'd add this comment/concern over here as "release discussions" are not enabled in the repo.

[CodeBlanch]

@julealgon

I just wanted to mention here that, unless I badly misunderstood the change, I feel like this is just not a good approach. If you run the delegate "outside of the Options mechanism", this would mean that people relying on IConfigureOptions could potentially conflict with inline configurations.

Hard to say, it is very nuanced. I'll provide a bit more detail here on the change.

When named options are not used...

We retrieve the options instance using Options API. Example here. Users registering Options API "things" (IConfigureOptions, IPostConfigureOptions, IValidateOptions, etc.) those will all fire in the correct order at that time. Then the delegate passed to AddOtlpExporter (if supplied) will be executed inline.

Is this how we would like it to work? No 🤣 Sharing an options class between the 3 signals was probably a mistake. I made a branch to introduce new classes but it was very large and obsoleted a lot of stuff. We felt like this smaller change will work well for most users and is safer for a break fix right before a stable release. But there is another effort to breakup the OtlpExporter project into separate projects for http and grpc. Chatting with @alanwest we hope to fix this up at that time.

this would mean that people relying on IConfigureOptions could potentially conflict with inline configurations.

Given the above, do you still feel there is an issue? If yes can you share a small repro so I can dig into it? Also if users do run into any issues, do you feel like it is easy enough to switch to named options?

The fact that you guys are doing stuff like this: [screenshot]

That screenshot is from the logs code right? Don't judge us by the logs code yet! 🤣 The OTel logs spec is up in the air right now so my hands are more or less tied. There is no "DI" support (IServiceCollection/IServiceProvider) at all yet in logs so I can't use Options API. But I will fix that "soon" (as possible). That's what the main-logs branch is all about.

[julealgon]

When named options are not used...

We retrieve the options instance using Options API. Example here. Users registering Options API "things" (IConfigureOptions, IPostConfigureOptions, IValidateOptions, etc.) those will all fire in the correct order at that time. Then the delegate passed to AddOtlpExporter (if supplied) will be executed inline.

Is this how we would like it to work? No 🤣

It is exactly how I'd assume it would work though, given there is a single settings class.

Sharing an options class between the 3 signals was probably a mistake.

I can see that. Even if the underlying options are the same, if the usage paths are different, different models should indeed be created: one for each.

Having said that, as a consumer of this library, it would certainly be really nice if there was an optional, unified path, where you could configure things such as the exporter, in a single place and didn't have to replicate it 3 times. I understand you want to remain flexible and allow distinct configurations for each metric type, but from my experience, that's hardly put to use, as the simplest flow is just to configure resources, exporters, and even instrumentations, all across with the same settings.

I'm not saying there shouldn't be an option to customize for each metric type, though I do think a simpler, unified configuration mode should also be available for those who don't need the per-pipeline configurations.

For instance, I wish there was a higher level AddXInstrumentation that would apply metrics and tracing instrumentation automatically for that instrumentation package, instead of requiring you to call that in 2 places: once for metrics and another time for tracing.

Adding additional extension methods that did this internally could be an easy way to approach that.

I liked the change that allows centralization of the resource builder configuration, for example (even though the logging part is still separate).

But there is another effort to breakup the OtlpExporter project into separate projects for http and grpc.

That makes sense to me if only because of the differences in dependencies (gRPC carries a lot of additional stuff with it I'm pretty sure). I assume you'll have to change the design a bit and change the simple enum-based selection, to a method-based selection to support that kind of separation, right? Perhaps have a "transport" option that you have to call inside AddOtlpExport, have the "default" library only have the "http" version and the "grpc" version added as an extension as part of a separate assembly like OpenTelemetry.Exporter.OpenTelemetryProtocol.Transports.Grpc or something.

The only bad thing about this is that "http" would become "the default", and "grpc" would be seen as an extension (which is not the case today with gRPC being the default mode).

The fact that you guys are doing stuff like this: [screenshot]

That screenshot is from the logs code right? Don't judge us by the logs code yet! 🤣 The OTel logs spec is up in the air right now so my hands are more or less tied. There is no "DI" support (IServiceCollection/IServiceProvider) at all yet in logs so I can't use Options API. But I will fix that "soon" (as possible). That's what the main-logs branch is all about.

Gotcha, thanks for clarifying.

this would mean that people relying on IConfigureOptions could potentially conflict with inline configurations.

Given the above, do you still feel there is an issue? If yes can you share a small repro so I can dig into it? Also if users do run into any issues, do you feel like it is easy enough to switch to named options?

If you are only manually instantiating the settings in the Logs path and that's supposed to be a temporary approach, then I don't see a huge problem right now, no.

As for using named options..... that feels a bit weird here. Hopefully you'd be able to move forward with the separate options classes instead.

Options were never supposed to be a "bag of generic settings that can be reused across many different services", but a very specific model for a specific purpose. The fact that multiple options models could have similar or even identical properties, should not change that (this is the type of "duplication of code" that is totally ok and intended).

I see the use of named options in this case as a workaround to that original poor design.

Another thing to keep in mind is that named options is probably considered an advanced mechanism that a ton of folks are unaware of. The added complexity could damage the usability of this library IMHO and should be avoided as much as possible.