[otlp] UseOtlpExporter cross-cutting extension

[CodeBlanch]

Fixes #4940
Relates to #5325

Changes

• Adds a cross-cutting UseOtlpExporter extension targeting IOpenTelemetryBuilder. This extension behaves as follows:
  • Adds an OtlpExporter for logging, metrics, & tracing at the end of the processor pipeline for logs & tracing.
  • Automatically turns on Logging, Metrics, & Tracing (performs "With" calls).
    • Note: Calls to AddSource \ AddMeter are still required to listen when using tracing & metrics.
  • More than one call to UseOtlpExporter will result in NotSupportedException being thrown.
  • Causes the signal-specific AddOtlpExporter extensions to throw NotSupportedException once called.
• Adds support for the signal-specific environment variables outlined in the spec when using UseOtlpExporter.

Scenarios

Scenario 1:

Add OtlpExporter for all signals (at the end of pipelines) using defaults + envvars to do configuration:

appBuilder.Services.AddOpenTelemetry()
    .ConfigureResource(r => r.AddService("MyService"))
    .UseOtlpExporter();

Scenario 2:

Add OtlpExporter for all signals (at the end of pipelines) and specify common settings:

appBuilder.Services.AddOpenTelemetry()
    .ConfigureResource(r => r.AddService("MyService"))
    .UseOtlpExporter(OtlpExportProtocol.HttpProtobuf, new Uri("http://my_otlp_endpoint/"));

Scenario 3:

Use the cross-cutting UseOtlpExporter method with some pipeline customizations.

This call order...

appBuilder.Services.AddOpenTelemetry()
    .ConfigureResource(r => r.AddService("MyService"))
    .UseOtlpExporter()
    .WithTracing(tracing => tracing
        .AddAspNetCoreInstrumentation()
        .AddProcessor(new MyRedactionProcessor)
        .AddConsoleExporter());

...will result in a tracing processor pipeline like this:

MyRedactionProcessor -> SimpleExportProcessor[ConsoleExporter] -> BatchExportProcessor[OtlpExporter]

Because UseOtlpExporter adds its processor/exporter to the end of the pipeline.

There will be some things which won't work as a result. Such as AutoFlushActivityProcessor, which needs to be at the end.

Merge requirement checklist

• CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
• Unit tests added/updated
• Appropriate CHANGELOG.md files updated for non-trivial changes
• Changes in public API reviewed (if applicable)

[samsp-msft]

Tagging: @davidfowl, @noahfalk

[samsp-msft]

Scenario 4 is where I think the API becomes a bit ugly. If you want to specify custom options per signal type, would it be better to just configure the OTLP exporter for that signal type with its own call to AddOTLPExpoter(o=> ...)? In that model I would expect the global call to set the defaults that can then be overridden by the signal specific if needed.

[noahfalk]

For scenario 4 I agree with Sam that the API seems more complex than AddOtlpExporter() per-signal. What if we just said global AddOtlpExporter() is only for cases 1,2, and 3 and anyone who wants (4) will need to call the signal specific AddOtlpExporter()? My suggestion is close to Sam's but without attempting to use global AddOtlpExporter() as a default config that can be overriden per-signal. I'd suggest that if users did this:

appBuilder.Services.AddOpenTelemetry()
    .ConfigureResource(r => r.AddService("MyService"))
    .AddOtlpExporter(...)
    .WithTracing()
    .WithMetrics().AddOtlpExporter(...)
    .WithLogging();

We just give them two exporters for the metrics pipeline, one with the global config and one with the local config. If both of the exporters are configured to send data to the same place perhaps we can give a warning or error unless there is some scenario where its reasonable you'd want exporters configured that way?

For scenario 5 IMO it would be fine to say that users who don't want the OTLP exporter at the end of the pipeline shouldn't use the global AddOtlpExporter() method. Instead they would need to configure each pipeline individually in the order they want it.

For scenario 6 I think its fine not to support it. If in the future you did want to support something like that perhaps this is the one that looks like Sam's suggested global defaults that are overriden per signal. Something like:

appBuilder.Services.AddOpenTelemetry()
    .ConfigureResource(r => r.AddService("MyService"))
    .ConfigureOtlpExporterDefaults(...)
    .WithTracing().AddOtlpExporter(...)
    .WithMetrics().AddOtlpExporter(...)
    .WithLogging().AddOtlpExporter(...);

[samsp-msft]

@CodeBlanch - I think this PR has gone way beyond what the .NET team was expecting, which is both a blessing and a curse. Can we simplify it down to just supporting scenarios 1, 2 & 3?

For scenario 5 & 6, I don't think we were expecting the instancing to be changing as part of this request. I was expecting the AddOTLPExporter() off AddOpenTelemetry() to really be a helper method, that would implicitly call AddOTLPExporter on each source. The primary goal is to reduce the number of lines of code - especially in scenarios where the configuration is being supplied by OTel environment variables.

I love the configuration aspect of scenario 3, but that could come as a later feature if required. We want this to land for Aspire GA and we are running out of runway.

[CodeBlanch]

@samsp-msft

I'm open to anything.

Originally my thinking was just to add IOpenTelemetryBuilder.AddOtlpExporter() and let all options comes from env vars. Which is really want Aspire is after as you mentioned. But @reyang raised a good point. That puts us into this design...

• If you need A call the cross-cutting AddOtlpExporter method.
• If you need X, Y, or Z call the signal-specific AddOtlpExporter methods.

Today the story is simple:

• Whatever you need call the signal-specific AddOtlpExporter methods.

Do you think the benefit in folding a few lines of code justifies mucking that story up? Especially given the ordering concerns.

For context what I tried to do with this was make the story:

• Whatever you need call the cross-cutting AddOtlpExporter method.

More or less set us up to obsolete the old stuff because there are a myriad of issues with it.

PS: If is actually a bit nicer than that.

Today the story is actually...

• Whatever you need call the signal-specific AddOtlpExporter methods.
• Signal-specific envvars are not supported.

This PR is actually...

• Whatever you need call the cross-cutting AddOtlpExporter method and it will respect signal-specific envvars.

Why are signal-specific envvars not respected today? Tracing, metrics, & logging all use OtlpExporterOptions class and there isn't a way to know when retrieving options which instance is being configured.

[samsp-msft]

@samsp-msft

I'm open to anything.

Originally my thinking was just to add IOpenTelemetryBuilder.AddOtlpExporter() and let all options comes from env vars. Which is really want Aspire is after as you mentioned. But @reyang raised a good point. That puts us into this design...

• If you need A call the cross-cutting AddOtlpExporter method.
• If you need X, Y, or Z call the signal-specific AddOtlpExporter methods.

Today the story is simple:

• Whatever you need call the signal-specific AddOtlpExporter methods.

Do you think the benefit in folding a few lines of code justifies mucking that story up? Especially given the ordering concerns.

For context what I tried to do with this was make the story:

• Whatever you need call the cross-cutting AddOtlpExporter method.

More or less set us up to obsolete the old stuff because there are a myriad of issues with it.

PS: If is actually a bit nicer than that.

Unfortunately I think the clock is going to be the biggest limitation here as we want a solution that works nicely for Aspire. That technically only requires Scenario 1, but 2 & 3 are really nice to have. The deadline is March 22nd, so what can we deliver for that timeline, and deliver the other scenarios in later releases.

[noahfalk]

My thoughts :)

Do you think the benefit in folding a few lines of code justifies mucking that story up?

The APIs today may have a simple premise, but it doesn't feel like that premise aligns with how customers intuitively conceptualize it. I expect many users initial concept is "I configure what telemetry to collect and where it should be sent" rather than "I configure three different telemetry pipelines from building blocks of sources, processors, and exporters". I think of this as "Make the code feel intuitive for users" rather than "Help users write fewer lines of code".

My impression is that if we add the new API targeting at least scenarios 1,2 and 3 it makes getting started easier for most people and it adds a step in the learning curve when transitioning from the trivial scenarios to the customized pipeline cases. This feels like a net win to me, especially because I expect most developers will never need to handle those more complex cases.

In order to ease the learning curve for the smaller number of folks who do need to cross over to the custom pipeline case I think we could be very explicit in the documentation and comments that pop up in IDE. Something like "This method is a shortcut that is equivalent to adding the OTLP exporter to the metrics, logging, and tracing pipelines individually. No signal specific customization APIs are available when using this shortcut. If you need those customizations, use AddOtlpExporter() on the TracerProviderBuilder, MeterProviderBuilder, and LoggingProviderBuilder instead of using this method."

More or less set us up to obsolete the old stuff because there are a myriad of issues with it.

I think the general principle of 'obsolete the old stuff so we can fix serious design problems' is fine. If you still want to support the advanced scenarios of per-signal pipelines where ordering matters and the OTLP exporter needs to not be at the end, that feels a rough fit for this global AddOtlpExporter() method to handle. I think if you wanted to fully obsolete and replace then probably some per-signal OTLP method is needed for those particularly complex scenarios. I'm also guessing that represents one of the most complex cases that is rarely used so it doesn't need to be treated with the same priority.

The deadline is March 22nd, so what can we deliver for that timeline, and deliver the other scenarios in later releases.

As a quick check, is an OTel change prior to March 22nd potentially in the cards? I don't recall your ship schedule to know if there is a new official build of OTel planned to go out before that date?

[CodeBlanch]

I just pushed updates after bouncing ideas off a bunch of different folks:

• Renamed the method UseOtlpExporter (from AddOtlpExporter). This is because it behaves differently from the traditional "Add" methods. Primarily it adds its processor to the end of the pipeline(s). Since it is behaving differently, I also changed it to automatically turn on the signals (no "With" calls are necessary). There is no way to change that "add-to-end" behavior other than go and call the extensions we have today.
• Removed most of the scenarios. The pressing need for Aspire is just envvars. The other stuff we can work on as follow-ups.
• In order to simplify the story, you can't call "UseOtlpExporter" more than once and you can't mix it with signal-specific "AddOtlpExporter" calls. Those will result in NotSupportedExceptions. We can always improve support in the future and loosen those restrictions based on needs that crop up.

[reyang]

@CodeBlanch can you please add a scenario to clarify the expected behavior when both UseOtlpExporter and With... are used?

For example:

appBuilder.Services.AddOpenTelemetry()
    .ConfigureResource(r => r.AddService("MyService"))
    .UseOtlpExporter()
    .WithTracing(tracing => tracing
        .AddAspNetCoreInstrumentation()
        .AddProcessor(new MyRedactionProcessor)
        .AddConsoleExporter());

Specifically, I want to make sure folks understand the ordering challenge, and some scenarios might not work as most users would imagine. For example: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/tree/main/src/OpenTelemetry.Extensions

[CodeBlanch]

@reyang Added to PR description

[reyang]

Now the proposal looks good to me.