Move LoggerProvider and friends (OTEL1000) to a stable API #5648

CodeBlanch · 2024-05-23T22:07:47Z

Changes

Expose the experimental APIs covered by OTEL1000 diagnostic
- ILoggingBuilder.UseOpenTelemetry extension left experimental for now. Pending [sdk] Add OpenTelemetrySdk builder pattern #5325 it may or may not be exposed at all.
- Sdk.CreateLoggerProviderBuilder moved to OTEL1001 (remains experimental).

Migration guidance for library authors

Library authors should begin adding extension methods targeting LoggerProviderBuilder. In a future release the ILoggingBuilder.AddOpenTelemetry extension method and the OpenTelemetryLoggerOptions.AddProcessor & OpenTelemetryLoggerOptions.SetResourceBuilder methods will be marked obsolete.

Before:

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

After:

    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.

Migration guidance for users

Note: No migration is currently required for users. All existing code should function as it did before. The following migration paths are purely optional.

When using OpenTelemetry.Extensions.Hosting

Before:

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

After:

appBuilder.Services.AddOpenTelemetry()
    .WithLogging(
        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 and continue to call the existing extension (while waiting for the component to be updated):

appBuilder.Services.AddOpenTelemetry()
    .WithLogging(
	logging => {},
	options =>
    	{
        	options.IncludeFormattedMessage = true;

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

When using LoggerFactory.Create

There is no dedicated API provided yet to migrate from ILoggerBuilder.AddOpenTelemetry (which provides OpenTelemetryLoggerOptions) to something which provides LoggerProviderBuilder. This API will come in a future version.

If for some reason you want to use LoggerProviderBuilder before that API is available, it is possible to use IServiceCollection.ConfigureOpenTelemetryLoggerProvider to configure the LoggerProviderBuilder using ILoggingBuilder.Services:

Before

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

After

using var loggerFactory = LoggerFactory.Create(builder =>
{
    builder.AddOpenTelemetry(options => options.IncludeFormattedMessage = true);

    builder.Services.ConfigureOpenTelemetryLoggerProvider(
        logging => logging.AddConsoleExporter());
});

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)

codecov · 2024-05-23T22:12:29Z

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

@@            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%)`	⬆️
...lemetryProtocol/OtlpLogExporterHelperExtensions.cs	`94.26% <ø> (+23.19%)`	⬆️
...lemetry.Extensions.Hosting/OpenTelemetryBuilder.cs	`100.00% <ø> (ø)`
src/OpenTelemetry/BatchExportProcessor.cs	`84.11% <100.00%> (+1.86%)`	⬆️
...ry/Logs/Builder/LoggerProviderBuilderExtensions.cs	`100.00% <ø> (ø)`
... and 6 more

... and 117 files with indirect coverage changes

CodeBlanch · 2024-05-23T22:44:32Z

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.

… examples, and docs.

vishweshbankwar · 2024-06-04T17:41:43Z

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:

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()

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 [sdk] Add OpenTelemetrySdk builder pattern #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.

CodeBlanch · 2024-06-04T19:15:43Z

@vishweshbankwar

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

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

vishweshbankwar · 2024-06-04T19:41:13Z

@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.

alanwest

Can't wait to start tell users they can finally use WithLogging!

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpLogExporterHelperExtensions.cs

alanwest · 2024-06-04T21:10:47Z

src/OpenTelemetry/.publicApi/Experimental/PublicAPI.Unshipped.txt

-static OpenTelemetry.Logs.LoggerProviderExtensions.Shutdown(this OpenTelemetry.Logs.LoggerProvider! provider, int timeoutMilliseconds = -1) -> bool
-static OpenTelemetry.OpenTelemetryBuilderSdkExtensions.WithLogging(this OpenTelemetry.IOpenTelemetryBuilder! builder) -> OpenTelemetry.IOpenTelemetryBuilder!
-static OpenTelemetry.OpenTelemetryBuilderSdkExtensions.WithLogging(this OpenTelemetry.IOpenTelemetryBuilder! builder, System.Action<OpenTelemetry.Logs.LoggerProviderBuilder!>! configure) -> OpenTelemetry.IOpenTelemetryBuilder!
-static OpenTelemetry.OpenTelemetryBuilderSdkExtensions.WithLogging(this OpenTelemetry.IOpenTelemetryBuilder! builder, System.Action<OpenTelemetry.Logs.LoggerProviderBuilder!>? configureBuilder, System.Action<OpenTelemetry.Logs.OpenTelemetryLoggerOptions!>? configureOptions) -> OpenTelemetry.IOpenTelemetryBuilder!
 static OpenTelemetry.Sdk.CreateLoggerProviderBuilder() -> OpenTelemetry.Logs.LoggerProviderBuilder!


I'm ok keeping this experimental for now. Might ultimately be nice simply for consistency, but I see no reason for this discussion to block the introduction of the new APIs, especially WithLogging.

…ed obsoletions.

CodeBlanch · 2024-06-05T17:58:15Z

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 [sdk] Add OpenTelemetrySdk builder pattern #5325.

@alanwest @vishweshbankwar PR has been updated.

src/OpenTelemetry.Api/.publicApi/Stable/PublicAPI.Unshipped.txt

src/OpenTelemetry.Api/Logs/LoggerProvider.cs

cijothomas · 2024-06-05T18:43:52Z

@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 · 2024-06-05T18:56:53Z

examples/AspNetCore/Program.cs

-
-    var resourceBuilder = ResourceBuilder.CreateDefault();
-    configureResource(resourceBuilder);
-    options.SetResourceBuilder(resourceBuilder);


❤️ seeing common Resource across all 3!

And also ❤️ fixing the awkwardness of calling methods like AddExporter on a supposedly pure "Options" class!

cijothomas · 2024-06-05T18:58:05Z

docs/logs/getting-started-aspnetcore/Program.cs

-        // In production environment, ConsoleExporter should be replaced with other exporters (e.g. OTLP Exporter).
-        .AddConsoleExporter();
-});
+// Add OpenTelemetry logging provider by calling the WithLogging extension.


should we add a note here to warn users that this approach only works if their exporter also supports this model?

Might get confusing. I think either a) we leave it as is or b) we show the old style. Let me know which you prefer! I'm hoping the gap is short-lived and we can get these extensions added soon after 1.9.0 is out.

okay to keep this as-is, assuming the gap is short-lived.

CodeBlanch · 2024-06-05T20:44:35Z

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

vishweshbankwar

LGTM

CodeBlanch added 2 commits May 23, 2024 14:17

Move OTEL1000 to a stable API.

dc3e25a

Use WithLogging in example app.

07501bf

CodeBlanch added 7 commits May 23, 2024 22:04

Fixes.

0022ab5

Add support for detecting experimental features in tests, benchmarks,…

d7286d4

… examples, and docs.

Merge branch 'main' into expose-logger-provider

98461e4

Build fix.

bd1756b

Merge remote-tracking branch 'upstream/main' into expose-logger-provider

43776a2

CHANGELOG updates for new APIs.

3a12f33

Updated CHANGELOGs for obsoletions.

33ec1e4

CodeBlanch marked this pull request as ready for review May 30, 2024 23:08

CodeBlanch requested a review from a team May 30, 2024 23:08

alanwest approved these changes Jun 4, 2024

View reviewed changes

CodeBlanch added 8 commits June 5, 2024 09:24

Merge remote-tracking branch 'upstream/main' into expose-logger-provider

95ba778

Moved ILoggingBuilder.UseOpenTelemetry back to experimental and remov…

0693b10

…ed obsoletions.

Lint.

1824b68

Fixes.

6009a0b

Fixes.

f945034

Cleanup.

c85f7ca

Cleanup.

5990c12

CHANGELOG updates.

c8e3fa2

vishweshbankwar reviewed Jun 5, 2024

View reviewed changes

src/OpenTelemetry.Api/.publicApi/Stable/PublicAPI.Unshipped.txt Show resolved Hide resolved

vishweshbankwar reviewed Jun 5, 2024

View reviewed changes

src/OpenTelemetry.Api/Logs/LoggerProvider.cs Show resolved Hide resolved

cijothomas reviewed Jun 5, 2024

View reviewed changes

Merge branch 'main' into expose-logger-provider

6611356

vishweshbankwar approved these changes Jun 5, 2024

View reviewed changes

CodeBlanch merged commit 2710c42 into open-telemetry:main Jun 5, 2024
50 checks passed

CodeBlanch deleted the expose-logger-provider branch June 5, 2024 23:06

cijothomas mentioned this pull request Jun 6, 2024

[logs] Support OTel Logs Bridge API #4433

Open

53 tasks

joegoldman2 mentioned this pull request Jun 8, 2024

[FEATURE REQ] Add AddAzureMonitorLogExporter extension method on LoggerProviderBuilder Azure/azure-sdk-for-net#44464

Closed

konjac mentioned this pull request Jun 11, 2024

Tutorial of getting-started-aspnetcore is not working as it refers un-released API #5686

Closed

CodeBlanch mentioned this pull request Jun 20, 2024

[sdk] Add OpenTelemetrySdk builder pattern #5325

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move LoggerProvider and friends (OTEL1000) to a stable API #5648

Move LoggerProvider and friends (OTEL1000) to a stable API #5648

CodeBlanch commented May 23, 2024 •

edited

Loading

codecov bot commented May 23, 2024 •

edited

Loading

CodeBlanch commented May 23, 2024

vishweshbankwar commented Jun 4, 2024

CodeBlanch commented Jun 4, 2024

vishweshbankwar commented Jun 4, 2024 •

edited

Loading

alanwest left a comment

alanwest Jun 4, 2024

CodeBlanch commented Jun 5, 2024

cijothomas commented Jun 5, 2024

cijothomas Jun 5, 2024

cijothomas Jun 5, 2024

CodeBlanch Jun 5, 2024

cijothomas Jun 5, 2024

CodeBlanch commented Jun 5, 2024

vishweshbankwar left a comment

Move LoggerProvider and friends (OTEL1000) to a stable API #5648

Move LoggerProvider and friends (OTEL1000) to a stable API #5648

Conversation

CodeBlanch commented May 23, 2024 • edited Loading

Changes

Migration guidance for library authors

Migration guidance for users

When using OpenTelemetry.Extensions.Hosting

When using LoggerFactory.Create

Merge requirement checklist

codecov bot commented May 23, 2024 • edited Loading

Codecov Report

CodeBlanch commented May 23, 2024

vishweshbankwar commented Jun 4, 2024

CodeBlanch commented Jun 4, 2024

vishweshbankwar commented Jun 4, 2024 • edited Loading

alanwest left a comment

Choose a reason for hiding this comment

alanwest Jun 4, 2024

Choose a reason for hiding this comment

CodeBlanch commented Jun 5, 2024

cijothomas commented Jun 5, 2024

cijothomas Jun 5, 2024

Choose a reason for hiding this comment

cijothomas Jun 5, 2024

Choose a reason for hiding this comment

CodeBlanch Jun 5, 2024

Choose a reason for hiding this comment

cijothomas Jun 5, 2024

Choose a reason for hiding this comment

CodeBlanch commented Jun 5, 2024

vishweshbankwar left a comment

Choose a reason for hiding this comment

CodeBlanch commented May 23, 2024 •

edited

Loading

codecov bot commented May 23, 2024 •

edited

Loading

vishweshbankwar commented Jun 4, 2024 •

edited

Loading