Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Logs] Add extension for registering OpenTelemetryLoggerProvider with ILoggingBuilder #3489

Merged
merged 6 commits into from
Jul 28, 2022

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Jul 26, 2022

Following up on #3454 (comment)

Changes

  • Adds an extension for manually registering an OpenTelemetryLoggerProvider with an ILoggingBuilder. You don't really need this extension to do that, but I think it helps with discoverability and our documentation/examples to have it.

Details

Here is the use case I am thinking about.

The Serilog startup pattern configures the log pipeline before the host builder. That sort of prescribes that users will need to manually build their own OpenTelemetryLoggerProvider. What this new extension does is just smooth that out.

    public static int Main(string[] args)
    {
        OpenTelemetryLoggerProvider provider = new OpenTelemetryLoggerProvider(options =>
        {
            options.IncludeFormattedMessage = true;
            options.AddConsoleExporter();
        });

        Log.Logger = new LoggerConfiguration()
            .WriteTo.OpenTelemetry(provider, disposeProvider: true)
            .CreateLogger();

        try
        {
            Log.Information("Starting web host");
            CreateHostBuilder(args, provider).Build().Run();
            return 0;
        }
        catch (Exception ex)
        {
            Log.Fatal(ex, "Host terminated unexpectedly");
            return 1;
        }
        finally
        {
            Log.CloseAndFlush();
        }
    }

    public static IHostBuilder CreateHostBuilder(string[] args, OpenTelemetryLoggerProvider openTelemetryLoggerProvider) =>
            Host.CreateDefaultBuilder(args)
                .ConfigureLogging(builder =>
                {
                    builder.AddOpenTelemetry(openTelemetryLoggerProvider, disposeProvider: false); // <- New extension used here
                })
                .ConfigureWebHostDefaults(webBuilder =>
                {
                    webBuilder.UseStartup<Startup>();
                });

Public API

+ public static ILoggingBuilder AddOpenTelemetry(this ILoggingBuilder builder)
+ public static ILoggingBuilder AddOpenTelemetry(this ILoggingBuilder builder, Action<OpenTelemetryLoggerOptions>? configure)
+ public static ILoggingBuilder AddOpenTelemetry(this ILoggingBuilder builder, OpenTelemetryLoggerProvider openTelemetryLoggerProvider)
+ public static ILoggingBuilder AddOpenTelemetry(this ILoggingBuilder builder, OpenTelemetryLoggerProvider openTelemetryLoggerProvider, bool disposeProvider)
- public static ILoggingBuilder AddOpenTelemetry(this ILoggingBuilder builder, Action<OpenTelemetryLoggerOptions>? configure = null)

The reason that one method is being removed is I first tried this...

public static ILoggingBuilder AddOpenTelemetry(this ILoggingBuilder builder, Action<OpenTelemetryLoggerOptions>? configure = null)
public static ILoggingBuilder AddOpenTelemetry(this ILoggingBuilder builder, OpenTelemetryLoggerProvider openTelemetryLoggerProvider)

...which gave me the warning:

Symbol 'AddOpenTelemetry' violates the backcompat requirement: 'Public API with optional parameter(s) should have the most parameters amongst its public overloads'. See 'https://github.com/dotnet/roslyn/blob/master/docs/Adding%20Optional%20Parameters%20in%20Public%20API.md' for details.

The layout of the new methods fixes that. Checked with @tarekgh. New design should be mostly safe. If users were calling these methods with reflection and specifically checking the metadata for "optional" parameter, they would break. But I feel like that is a super edge case. Anyone just compiling should be fine.

TODOs

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Changes in public API reviewed
  • Tests

@CodeBlanch CodeBlanch requested a review from a team as a code owner July 26, 2022 19:03
@codecov
Copy link

codecov bot commented Jul 26, 2022

Codecov Report

Merging #3489 (b7903b0) into main (f089ec7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3489   +/-   ##
=======================================
  Coverage   86.49%   86.49%           
=======================================
  Files         272      272           
  Lines        9931     9939    +8     
=======================================
+ Hits         8590     8597    +7     
- Misses       1341     1342    +1     
Impacted Files Coverage Δ
...enTelemetry/Logs/OpenTelemetryLoggingExtensions.cs 100.00% <100.00%> (ø)
src/OpenTelemetry/Logs/OpenTelemetryLogger.cs 86.66% <0.00%> (-4.45%) ⬇️
...Telemetry/Internal/SelfDiagnosticsEventListener.cs 96.87% <0.00%> (-0.79%) ⬇️
...tation/OpenTelemetryProtocolExporterEventSource.cs 85.00% <0.00%> (+10.00%) ⬆️

Comment on lines -40 to +48
public static ILoggingBuilder AddOpenTelemetry(this ILoggingBuilder builder, Action<OpenTelemetryLoggerOptions>? configure = null)
public static ILoggingBuilder AddOpenTelemetry(this ILoggingBuilder builder, Action<OpenTelemetryLoggerOptions>? configure)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥳 🎉

Copy link
Member

@alanwest alanwest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cijothomas cijothomas merged commit 2b1e75d into open-telemetry:main Jul 28, 2022
@CodeBlanch CodeBlanch deleted the loggerprovider-extension branch July 28, 2022 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants