opentelemetry-dotnet icon indicating copy to clipboard operation
opentelemetry-dotnet copied to clipboard

Move LoggerProvider and friends (OTEL1000) to a stable API

Open CodeBlanch opened this issue 1 year ago • 2 comments

Fixes #5442 Fixes #4014

Changes

  • Expose the experimental APIs covered by OTEL1000 diagnostic
  • Obsolete old APIs

Migration guidance for users

These two patterns will now generate warnings due to obsolete APIs:

appBuilder.Logging.AddOpenTelemetry(options =>
{
    options.IncludeFormattedMessage = true;
    options.AddOtlpExporter(o => o.Endpoint = new Uri("http://myotlpendpoint:4317/logs"));
});
using var loggerFactory = LoggerFactory.Create(builder => builder
    .AddOpenTelemetry(options =>
    {
        options.IncludeFormattedMessage = true;
        options.AddOtlpExporter(o => o.Endpoint = new Uri("http://myotlpendpoint:4317/logs"));
    });

Migrate these to call UseOpenTelemetry instead:

appBuilder.Logging.UseOpenTelemetry(
    logging => logging.AddOtlpExporter(o => o.Endpoint = new Uri("http://myotlpendpoint:4317/logs")),
    options => options.IncludeFormattedMessage = true);
using var loggerFactory = LoggerFactory.Create(builder => builder
    .UseOpenTelemetry(
        logging => logging.AddOtlpExporter(o => o.Endpoint = new Uri("http://myotlpendpoint:4317/logs")),
        options => options.IncludeFormattedMessage = true));

For this to work, your exporter needs to expose an extension which targets LoggerProviderBuilder. Extensions are provided for OtlpExporter, ConsoleExporter, and InMemoryExporter.

If you are using an exporter from the https://github.com/open-telemetry/opentelemetry-dotnet-contrib repository or some other exporter, it may not provide this extension yet. First try to upgrade your exporter package. If there isn't an update available, you may use this pattern to resolve obsoletion warnings by continuing to call the existing extension (while waiting for the component to be updated):

appBuilder.Logging.UseOpenTelemetry(
    logging => { },
    options =>
    {
        options.IncludeFormattedMessage = true;

        // Note: Existing extensions targeting OpenTelemetryLoggerOptions can still be called.
        options.AddGenevaLogExporter(o => o.ConnectionString = "...");
    });

Migration guidance for library authors

Library authors should add extensions targeting LoggerProviderBuilder.

Before:

    public static OpenTelemetryLoggerOptions AddMyExporter(
        this OpenTelemetryLoggerOptions loggerOptions,
        Action<MyExporterOptions> configure) {}

After:

    [Obsolete("Call LoggerProviderBuilder.AddMyExporterinstead this method will be removed in a future version.")]
    public static OpenTelemetryLoggerOptions AddMyExporter(
        this OpenTelemetryLoggerOptions loggerOptions,
        Action<MyExporterOptions> configure) {}

    public static LoggerProviderBuilder AddMyExporter(
        this LoggerProviderBuilder builder,
        Action<MyExporterOptions> configure) {}

LoggerProviderBuilder has the same AddProcessor and SetResourceBuilder methods previously available on OpenTelemetryLoggerOptions as well as the full set of methods\features available to MeterProviderBuilder and TracerProviderBuilder.

Merge requirement checklist

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

CodeBlanch avatar May 23 '24 22:05 CodeBlanch

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 86.15%. Comparing base (6250307) to head (6611356). Report is 249 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5648      +/-   ##
==========================================
+ Coverage   83.38%   86.15%   +2.77%     
==========================================
  Files         297      254      -43     
  Lines       12531    11054    -1477     
==========================================
- Hits        10449     9524     -925     
+ Misses       2082     1530     -552     
Flag Coverage Δ
unittests ?
unittests-Project-Experimental 86.05% <100.00%> (?)
unittests-Project-Stable 86.00% <100.00%> (?)
unittests-Solution 86.01% <100.00%> (?)
unittests-UnstableCoreLibraries-Experimental 20.19% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...endencyInjectionLoggerProviderBuilderExtensions.cs 100.00% <ø> (ø)
...encyInjectionLoggingServiceCollectionExtensions.cs 100.00% <ø> (ø)
src/OpenTelemetry.Api/Logs/LoggerProvider.cs 100.00% <ø> (ø)
...rc/OpenTelemetry.Api/Logs/LoggerProviderBuilder.cs 100.00% <ø> (ø)
...porter.Console/ConsoleExporterLoggingExtensions.cs 0.00% <ø> (ø)
...rter.InMemory/InMemoryExporterLoggingExtensions.cs 100.00% <ø> (+33.33%) :arrow_up:
...lemetryProtocol/OtlpLogExporterHelperExtensions.cs 94.26% <ø> (+23.19%) :arrow_up:
...lemetry.Extensions.Hosting/OpenTelemetryBuilder.cs 100.00% <ø> (ø)
src/OpenTelemetry/BatchExportProcessor.cs 84.11% <100.00%> (+1.86%) :arrow_up:
...ry/Logs/Builder/LoggerProviderBuilderExtensions.cs 100.00% <ø> (ø)
... and 6 more

... and 117 files with indirect coverage changes

codecov[bot] avatar May 23 '24 22:05 codecov[bot]

Some thoughts for anyone taking a look at this...

  • We don't necessarily have to obsolete anything. I put them in initially to see the fallout. I added migration guidance to the PR so we can see what it would look like.

  • There is no Sdk.CreateLoggerProviderBuilder being added here. I moved that to the Log Bridge experimental API (OTEL1001). The only use case I can think of for Sdk.CreateLoggerProviderBuilder is in order to build a LoggerProvider manually to feed into a bridge. For the use cases we have today, LoggerFactory.Create works fine so I didn't feel this API was needed at this time.

CodeBlanch avatar May 23 '24 22:05 CodeBlanch

Some thoughts for anyone taking a look at this...

  • We don't necessarily have to obsolete anything. I put them in initially to see the fallout. I added migration guidance to the PR so we can see what it would look like.
  • There is no Sdk.CreateLoggerProviderBuilder being added here. I moved that to the Log Bridge experimental API (OTEL1001). The only use case I can think of for Sdk.CreateLoggerProviderBuilder is in order to build a LoggerProvider manually to feed into a bridge. For the use cases we have today, LoggerFactory.Create works fine so I didn't feel this API was needed at this time.

@CodeBlanch - I think we should not mark things obsolete rn.

Regarding the migration guidance:

  1. Migration guidance for case 1:

    appBuilder.Logging.AddOpenTelemetry(options =>
    {
        options.IncludeFormattedMessage = true;
        options.AddOtlpExporter(o => o.Endpoint = new Uri("http://myotlpendpoint:4317/logs"));
    });
    

    With this change, my understanding was, they should be able to move to following pattern (bringing consistency with other two signals for hosting scenarios and the update you have in the example)

    appBuilder.Services.AddOpenTelemetry().WithLogging()
    
  2. Migration guidance for case 2:

    using var loggerFactory = LoggerFactory.Create(builder => builder
            .UseOpenTelemetry(
            logging => logging.AddOtlpExporter(o => o.Endpoint = new Uri("http://myotlpendpoint:4317/logs")),
            options => options.IncludeFormattedMessage = true));
    

    I think we should not include it with this change. As per my understanding, it depends on the final outcome of https://github.com/open-telemetry/opentelemetry-dotnet/pull/5325. If we go ahead with the OpenTelemetrySdk pattern then we could just ask users to follow that (Separate discussion).

In short, the only use case we would enable right now is .WithLogging() for hosting scenarios.

For future, we could decide whether we want to have the migration guidance for case 2 or go with OpenTelemetrySdk pattern.

vishweshbankwar avatar Jun 04 '24 17:06 vishweshbankwar

@vishweshbankwar

@CodeBlanch - I think we should not mark things obsolete rn.

Based on what thinking/reasons? Please elaborate a bit 😄

CodeBlanch avatar Jun 04 '24 19:06 CodeBlanch

@vishweshbankwar

@CodeBlanch - I think we should not mark things obsolete rn.

Based on what thinking/reasons? Please elaborate a bit 😄

  • Based on my point # 2: Once we have a clear plan/alternate for users on case 2 then we can consider the obsoleting part.
  • Also, following this staged approach would give enough time for library authors also to support the extensions based on LoggerProviderBuilder.

vishweshbankwar avatar Jun 04 '24 19:06 vishweshbankwar

We decided on the SIG meeting 6/4/2024 that we would:

  • Not obsolete anything for 1.9.0. We will obsolete things in a future release. This was done to give library authors time to add extensions targeting LoggerProviderBuilder which will ease migration for users when things are obsoleted.

  • ILoggingBuilder.UseOpenTelemetry will not be exposed initially. It may be replaced by the design on #5325.

@alanwest @vishweshbankwar PR has been updated.

CodeBlanch avatar Jun 05 '24 17:06 CodeBlanch

@CodeBlanch Can you update the PR description to include the changes expected to be done by end users as well, even though they are not forced to do it? Also call out that, this should be done only if their exporter supports this new model (and mention that exporters shipped from this repo already supports this model)

cijothomas avatar Jun 05 '24 18:06 cijothomas

@cijothomas I added "Migration guidance for users" back on the PR description.

CodeBlanch avatar Jun 05 '24 20:06 CodeBlanch