[sdk-logs] Introduce ConfigureOpenTelemetry ILoggingBuilder extension (proposal 1)

[CodeBlanch]

Relates to #4433

/cc @noahfalk

Changes

• Introduce ILoggingBuilder.ConfigureOpenTelemetry extension for configuring LoggerProviderBuilder
• Introduce LoggerProviderBuilder.ConfigureLoggerOptions extension for configuring OpenTelemetryLoggerOptions

Details

Today we configure logging like this:

appBuilder.Logging.AddOpenTelemetry(options =>
{
   options.IncludeFormattedMessage = true;
   options.AddConsoleExporter();
});

"options" is OpenTelemetryLoggerOptions class.

Now the OpenTelemetry Specification has LoggerProvider and we have a LoggerProviderBuilder API. The SDK LoggerProvider is fed into the ILogger integration (OpenTelemetryLoggerProvider).

There are different ways we could approach this. Today we have extensions for logging that target OpenTelemetryLoggerOptions. We will need to target LoggerProviderBuilder now. We could forever have two versions of every extension, but that seems lame.

This PR is adding a new extension ConfigureOpenTelemetry to try and bridge the two worlds. AddProcessor and SetResourceBuilder on OpenTelemetryLoggerOptions would be obsoleted as would our existing extensions on OpenTelemetryLoggerOptions.

API Usage

All of these styles can be interchanged.

// Configure OTel via ILoggingBuilder
appBuilder.Logging
   .AddOpenTelemetry(options => options.IncludeFormattedMessage = true)
   .ConfigureOpenTelemetry(builder => builder.AddConsoleExporter());

// Configure OTel via OpenTelemetryBuilder
appBuilder.Services
   .AddOpenTelemetry()
   .WithLogging(builder => builder.AddConsoleExporter());

// Configure ILogger options via ILoggingBuilder
appBuilder.Logging
   .AddOpenTelemetry(options => options.IncludeFormattedMessage = true)

// Configure OTel via OpenTelemetryBuilder
appBuilder.Services
   .AddOpenTelemetry()
   .WithLogging(builder => builder.AddConsoleExporter());

// Configure ILogger options via Options API
appBuilder.Services.Configure<OpenTelemetryLoggerOptions>(options => options.IncludeFormattedMessage = true);

// Configure OTel via ILoggingBuilder
appBuilder.Logging
   .AddOpenTelemetry()
   .ConfigureOpenTelemetry(builder => builder.AddConsoleExporter());

// Configure ILogger options via Options API
appBuilder.Services.Configure<OpenTelemetryLoggerOptions>(options => options.IncludeFormattedMessage = true);

// Configure OTel via OpenTelemetryBuilder
appBuilder.Services
   .AddOpenTelemetry()
   .WithLogging(builder => builder
      .ConfigureLoggerOptions(options => options.IncludeFormattedMessage = true)
      .AddConsoleExporter());

Merge requirement checklist

• CONTRIBUTING guidelines followed (nullable enabled, static analysis, etc.)
• Unit tests added/updated

[noahfalk]

This got a bit longer than I originally imagined :) Hopefully its useful though.

Potential alternative API

What do you think of this alternative?

// Configure OTel via ILoggingBuilder
appBuilder.Logging
   .SendToOpenTelemetry(builder => builder
           .AddConsoleExporter()
           .ConfigureLoggerOptions(options => options.IncludeFormattedMessage = true));

// Configure OTel via OpenTelemetryBuilder
appBuilder.Services
   .AddOpenTelemetry()
   .WithLogging(builder => builder
       .AddConsoleExporter()
       .ConfigureLoggerOptions(options => options.IncludeFormattedMessage = true));

// Configure OTel via OpenTelemetryBuilder
appBuilder.Services
   .AddOpenTelemetry()
   .WithLogging(builder => builder.AddConsoleExporter());

// Configure ILogger options via Options API
appBuilder.Services.Configure<OpenTelemetryLoggerOptions>(options => options.IncludeFormattedMessage = true);

At the implementation level I am imagining you would mark ILoggingBuilder.AddOpenTelemetry() obsolete or just recommend SendToOpenTelemetry() as the newer, more functionally complete alternative if you don't want to be adding warnings to anyone's build.

Assuming the runtime adds MetricsBuilder and DistributedTracingBuilder in the future then I am imagining the SendToOpenTelemetry() pattern would be the method that gets reproduced, not LoggingBuilder.AddOpenTelemetry(options => {}):

// In the future this would work:
appBuilder.Logging.SendToOpenTelemetry(otelLoggingBuilder => ...);
appBuilder.Metrics.SendToOpenTelemetry(otelMetricsBuilder => ...);
appBuilder.DistributedTracing.SendToOpenTelemetry(otelTracingBuilder => ...);

// Though I still hope we can get this pattern supported as well because it allows for
// less duplication setting up each signal. This is what I would recommend in examples:
services.AddOpenTelemetry(builder => builder
    .AddConsoleExporter()
    .SetResource(...)
    .WithLogging()
    .WithMetrics()
    .WithTracing()

Defaults

As an orthogonal issue, if WithLogging() is automatically subscribing to LoggingBuilder I think it would set a precedent that devs would expect WithMetrics() and WithTracing() should automatically subscribe to MetricsBuilder and DistributedTracingBuilder if/when those exist in the future. I think that kind of default would be nice, but it does mean:

• The current version X of OTel doesn't support that behavior and then in some future version Y of OTel you do support the behavior. Users who upgrade OTel from X to Y could, in theory, observe new telemetry being sent without having made any other code changes.
• In order to subscribe to MetricsBuilder or DistributedTracingBuilder OTel might need to reference some new assembly like M.E.Metrics.Abstractions.dll, or M.E.DistributedTracing.Abstractions.dll, in the same way you reference M.E.Logging.Abstractions.dll today.

If either of those behaviors are worrisome then you might want to go the other way set the precedent that data coming from LoggingBuilder is off-by-default when calling WithLogging(). Then devs would enable it either by calling an extension method on LoggingBuilder (AddOpenTelemetry, SendToOpenTelemetry, ConfigureOpenTelemetry, etc), or via some other switch on the OTel API surface. In a future off-by-default world an example might look like this:

// Option A - opt-in using SendToOpenTelemetry() (or alternate named extension method)
host.Services.AddOpenTelemetry(builder => builder
    .AddConsoleExporter()
    .SetResource(...)
host.Logging.SendToOpenTelemetry();
host.Metrics.SendToOpenTelemetry();
host.DistributedTracing.SendToOpenTelemetry();

// Option B - opt-in using some newly defined OTel API
host.Services.AddOpenTelemetry(builder => builder
    .AddConsoleExporter()
    .SetResource(...)
    .AddDefaultAppInstrumentation() // this would subscribe to all the runtime configured telemetry
                                    // across all signals, same behavior as option A.

[CodeBlanch]

Note: I updated this to include ConfigureLoggerOptions that bit is also on proposal 2 & 3. I think it is useful enough to be part of all 3.

[codecov]

Codecov Report

Merging #4496 (1fee7fa) into main (d63c32f) will decrease coverage by 0.11%.
The diff coverage is 12.50%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4496      +/-   ##
==========================================
- Coverage   85.21%   85.11%   -0.11%     
==========================================
  Files         315      316       +1     
  Lines       12549    12589      +40     
==========================================
+ Hits        10694    10715      +21     
- Misses       1855     1874      +19     

Impacted Files	Coverage Δ
...ry/Logs/Builder/LoggerProviderBuilderExtensions.cs	92.15% <0.00%> (-5.76%)	⬇️
...lemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs	85.00% <ø> (ø)
...try/Logs/ILogger/OpenTelemetryLoggingExtensions.cs	89.18% <20.00%> (-10.82%)	⬇️

... and 6 files with indirect coverage changes

[samsp-msft]

I think the ideal pattern is that you can register the Logging integration with OTel, either via logging which you do today, or via OTEL and adding logging to it, so you can setup all the telemetry in one place/off one OTEL object.

I don't think that using WithMetrics() on its own does much today does it until you add either a Meter name or register a provider such as ASPNetCoreMetrics.

[noahfalk]

@samsp-msft - Am I paraphrasing your comment correctly? "WithLogging() should not listen to ILogger by default, it should be opt-in"

I don't think that using WithMetrics() on its own does much today

That is my understanding as well.

[noahfalk]

@CodeBlanch - is this draft going to be closed in favor of #4501, or you are evaluating different options side by side to get feedback?

[CodeBlanch]

@noahfalk I ended up opening 4 different ones:

• [sdk-logs] Introduce ConfigureOpenTelemetry ILoggingBuilder extension (proposal 1) #4496
• [sdk-logs] Introduce AddOpenTelemetryLogging ILoggingBuilder extension (proposal 2) #4501
• [sdk-logs] Support builder behavior on OpenTelemetryLoggerOptions (proposal 3) #4502
• [sdk-logs] Introduce OpenTelemetryLoggingBuilder (proposal 4) #4506

Going to leave them open for a bit to gather feedback. We'll probably discuss on the next SIG meeting. Hoping one will emerge as the favorite/least bad choice 🤣

[noahfalk]

Hoping one will emerge as the favorite/least bad choice 🤣

Probably no surprise my vote is for 2. Among the rest 1 would be my next choice, 3 and 4 feel very shady.

[github-actions]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[github-actions]

Closed as inactive. Feel free to reopen if this PR is still being worked on.