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

[sdk] Add OpenTelemetrySdk builder pattern #5325

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/OpenTelemetry/.publicApi/Stable/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
OpenTelemetry.OpenTelemetrySdk
OpenTelemetry.OpenTelemetrySdk.Dispose() -> void
OpenTelemetry.OpenTelemetrySdk.LoggerProvider.get -> OpenTelemetry.Logs.LoggerProvider!
OpenTelemetry.OpenTelemetrySdk.MeterProvider.get -> OpenTelemetry.Metrics.MeterProvider!
OpenTelemetry.OpenTelemetrySdk.TracerProvider.get -> OpenTelemetry.Trace.TracerProvider!
OpenTelemetry.OpenTelemetrySdkExtensions
static OpenTelemetry.OpenTelemetrySdk.Create(System.Action<OpenTelemetry.IOpenTelemetryBuilder!>! configure) -> OpenTelemetry.OpenTelemetrySdk!
static OpenTelemetry.OpenTelemetrySdkExtensions.GetLoggerFactory(this OpenTelemetry.OpenTelemetrySdk! sdk) -> Microsoft.Extensions.Logging.ILoggerFactory!
6 changes: 6 additions & 0 deletions src/OpenTelemetry/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@

## Unreleased

* Added `OpenTelemetrySdk.Create` API for configuring OpenTelemetry .NET signals
(logging, tracing, and metrics) via a single builder. This new API simplifies
bootstrap and teardown, and supports cross-cutting extensions targeting
`IOpenTelemetryBuilder`.
([#5325](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5325))

## 1.9.0

Released 2024-Jun-14
Expand Down
124 changes: 124 additions & 0 deletions src/OpenTelemetry/OpenTelemetrySdk.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

using System.Diagnostics;
using Microsoft.Extensions.DependencyInjection;
using OpenTelemetry.Internal;
using OpenTelemetry.Logs;
using OpenTelemetry.Metrics;
using OpenTelemetry.Trace;

namespace OpenTelemetry;

/// <summary>
/// Contains methods for configuring the OpenTelemetry SDK and accessing
/// logging, metrics, and tracing providers.
/// </summary>
public sealed class OpenTelemetrySdk : IDisposable
{
private readonly ServiceProvider serviceProvider;

private OpenTelemetrySdk(
Action<IOpenTelemetryBuilder> configure)
{
Debug.Assert(configure != null, "configure was null");

var services = new ServiceCollection();

var builder = new OpenTelemetrySdkBuilder(services);

configure!(builder);

this.serviceProvider = services.BuildServiceProvider();

this.LoggerProvider = (LoggerProvider?)this.serviceProvider.GetService(typeof(LoggerProvider))
?? new NoopLoggerProvider();
this.MeterProvider = (MeterProvider?)this.serviceProvider.GetService(typeof(MeterProvider))
?? new NoopMeterProvider();
this.TracerProvider = (TracerProvider?)this.serviceProvider.GetService(typeof(TracerProvider))
?? new NoopTracerProvider();
}

/// <summary>
/// Gets the <see cref="Logs.LoggerProvider"/>.
/// </summary>
/// <remarks>
/// Note: The default <see cref="LoggerProvider"/> will be a no-op instance.
/// Call <see
/// cref="OpenTelemetryBuilderSdkExtensions.WithLogging(IOpenTelemetryBuilder)"/> to
/// enable logging.
/// </remarks>
public LoggerProvider LoggerProvider { get; }

/// <summary>
/// Gets the <see cref="Metrics.MeterProvider"/>.
/// </summary>
/// <remarks>
/// Note: The default <see cref="MeterProvider"/> will be a no-op instance.
/// Call <see
/// cref="OpenTelemetryBuilderSdkExtensions.WithMetrics(IOpenTelemetryBuilder)"/>
/// to enable metrics.
/// </remarks>
public MeterProvider MeterProvider { get; }

/// <summary>
/// Gets the <see cref="Trace.TracerProvider"/>.
/// </summary>
/// <remarks>
/// Note: The default <see cref="TracerProvider"/> will be a no-op instance.
/// Call <see
/// cref="OpenTelemetryBuilderSdkExtensions.WithTracing(IOpenTelemetryBuilder)"/>
/// to enable tracing.
/// </remarks>
public TracerProvider TracerProvider { get; }

/// <summary>
/// Gets the <see cref="IServiceProvider"/> containing SDK services.
/// </summary>
internal IServiceProvider Services => this.serviceProvider;

/// <summary>
/// Create an <see cref="OpenTelemetrySdk"/> instance.
/// </summary>
/// <param name="configure"><see cref="IOpenTelemetryBuilder"/> configuration delegate.</param>
/// <returns>Created <see cref="OpenTelemetrySdk"/>.</returns>
public static OpenTelemetrySdk Create(
Copy link
Member Author

Choose a reason for hiding this comment

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

@rajkumar-rangaraj

You commented: #5325 (comment)

I'm dropping a review for the discussion so replies won't get sprinkled all over the PR.

If we proceed with this approach, the simultaneous use of OpenTelemetrySdk.Create() and Sdk.CreateTracerProviderBuilder() should not be allowed. If both methods are used, one should throw an exception to clearly state that these APIs cannot be combined.

Doing that might be dangerous. Let's say someone creates a library. They want to send off telemetry from their library to their own servers. So they have a TracerProvider to do that. I would never recommend a library do this, but we have seen it 🤣 Now the host application comes along and uses OpenTelemetrySdk.Create to send its own telemetry somewhere. If we did this, suddenly the library would start to blow up. Or worse, let's say host is doing Sdk.CreateTracerProviderBuilder() and they upgrade some library which begins doing OpenTelemetrySdk.Create and suddenly blows up the host.

The initial concern over...

        using var openTelemetry = OpenTelemetrySdk.Create(builder => builder
            .ConfigureResource(r => r.AddService("MyService"))
            .UseOtlpExporter());

        using var tracerProvider = Sdk.CreateTracerProviderBuilder()
            .AddAspNetCoreInstrumentation()
            .Build();

My question is, do we really have an issue here? Users can already find themselves in odd situations today:

        using var tracerProviderA = Sdk.CreateTracerProviderBuilder()
            .AddOtlpExporter()
            .Build();

        using var tracerProviderB = Sdk.CreateTracerProviderBuilder()
            .AddAspNetCoreInstrumentation()
            .Build();

That is basically the same thing you are concerned about, no?

What we could do is Obsolete the "Sdk.Create*" APIs in favor of OpenTelemetrySdk.Create. I don't think it really solves the potential for users to shoot themselves in the foot, but it might help surface having the old API lingering around during migration to the new API?

Action<IOpenTelemetryBuilder> configure)
{
Guard.ThrowIfNull(configure);

return new(configure);
}

/// <inheritdoc/>
public void Dispose()
{
this.serviceProvider.Dispose();
}

internal sealed class NoopLoggerProvider : LoggerProvider
{
}

internal sealed class NoopMeterProvider : MeterProvider
{
}

internal sealed class NoopTracerProvider : TracerProvider
{
}

private sealed class OpenTelemetrySdkBuilder : IOpenTelemetryBuilder
{
public OpenTelemetrySdkBuilder(IServiceCollection services)
{
Debug.Assert(services != null, "services was null");

services!.AddOpenTelemetrySharedProviderBuilderServices();

this.Services = services!;
}

public IServiceCollection Services { get; }
}
}
36 changes: 36 additions & 0 deletions src/OpenTelemetry/OpenTelemetrySdkExtensions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging.Abstractions;
using OpenTelemetry.Internal;

namespace OpenTelemetry;

/// <summary>
/// Contains methods for extending the <see cref="OpenTelemetrySdk"/> class.
/// </summary>
public static class OpenTelemetrySdkExtensions
{
private static readonly NullLoggerFactory NoopLoggerFactory = new();

/// <summary>
/// Gets the <see cref="ILoggerFactory"/> contained in an <see
/// cref="OpenTelemetrySdk"/> instance.
/// </summary>
/// <remarks>
/// Note: The default <see cref="ILoggerFactory"/> will be a no-op instance.
/// Call <see
/// cref="OpenTelemetryBuilderSdkExtensions.WithLogging(IOpenTelemetryBuilder)"/>
/// to enable logging.
/// </remarks>
/// <param name="sdk"><see cref="OpenTelemetrySdk"/>.</param>
/// <returns><see cref="ILoggerFactory"/>.</returns>
public static ILoggerFactory GetLoggerFactory(this OpenTelemetrySdk sdk)
Copy link
Member

Choose a reason for hiding this comment

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

Exploring the .NET Framework and console app use cases for a sec...

If a user has already adopted ILogger as their logging framework, then they probably have code somewhere like

var loggerFactory = LoggerFactory.Create((loggingBuilder) => {
    loggingBuilder.AddConsole() ...
});

If they then use WithLogging, they'll need to refactor things to avoid having to interact with two LoggerFactory instances to get loggers.

Are there other options that might minimize the need for refactoring? Would an additional overload of WithLogging accepting a LoggerFactory as an argument help? My assumption is that if they pass in their existing LoggerFactory then we'd be able to add the OpenTelemetry logger provider.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would an additional overload of WithLogging accepting a LoggerFactory as an argument help?

I hesitate to do this. There is an ILoggerFactory.AddProvider API. BUT it is after IServiceProvider has been created. We won't be able to add anything to the IServiceCollection and we'll have to bootstrap manually everything OpenTelemetryLoggerProvider and LoggerProvider need as well as anything any component might need (ex OtlpLogExporter). Could be possible, but it is going to be a big headache and create a sort of parallel universe we'll have to deal with for eternity.

There is also an issue if we add an extension something like WithLogging(this IOpenTelemetryBuilder builder, ILoggerFactory loggerFactory) that will also appear to hosting users where we probably don't want it.

What other options do we have?

  • We could expose the currently experimental ILoggingBuilder.UseOpenTelemetry extensions

    This would allow people to continue to use LoggerFactory.Create style and utilize LoggerProviderBuilder. Which is to say, we don't force anyone to switch to WithLogging.

  • Give different migration guidance

    Let's say .NET Framework user has this today:

    using var loggerFactory = LoggerFactory.Create((loggingBuilder) => {
        loggingBuilder.AddConsole().AddOpenTelemetry(...)...
    });

    I guess what you are thinking @alanwest is they would start doing this:

    using var loggerFactory = LoggerFactory.Create((loggingBuilder) => {
        loggingBuilder.AddConsole()...
    });
    
    using var openTelemetry = OpenTelemetrySdk.Create(builder =>
        builder.WithLogging(...));
    
    var otelLoggerFactory = openTelemetry.GetLoggerFactory();

    In that case they would have two logger factories and need to do some work.

    But they could do this instead:

    using var openTelemetry = OpenTelemetrySdk.Create(builder =>
    {
        builder.Services.AddLogging(logging => logging.AddConsole()...);
    
        builder.WithLogging(...);
    });
    
    var loggerFactory = openTelemetry.GetLoggerFactory();

    They'll end up with a single factory with Console + OTel as they had before.

    We could simplify that a bit with another WithLogging overload like...

    public static IOpenTelemetryBuilder WithLogging(
       this IOpenTelemetryBuilder builder,
       Action<LoggerProviderBuilder>? configureBuilder,
       Action<OpenTelemetryLoggerOptions>? configureOptions,
       Action<ILoggingBuilder>? configureILogger)
    { }

    Then they could do:

    using var openTelemetry = OpenTelemetrySdk.Create(builder =>
    {
        builder.WithLogging(
            logging => logging.AddOtlpExporter(),
            options => options.IncludeFormattedMessage = true,
            ilogger => ilogger.AddConsole());
    });
    
    var loggerFactory = openTelemetry.GetLoggerFactory();

    It is a kind of heavy/complicated API though. Do we think this is common enough/there will be enough demand to justify a dedicated API?

  • We could just wait and see what the feedback is and if there is demand for something.

{
Guard.ThrowIfNull(sdk);

return (ILoggerFactory?)sdk.Services.GetService(typeof(ILoggerFactory))
?? NoopLoggerFactory;
}
}
70 changes: 70 additions & 0 deletions test/OpenTelemetry.Tests/OpenTelemetrySdkTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

#nullable enable

using Microsoft.Extensions.Logging.Abstractions;
using OpenTelemetry.Logs;
using OpenTelemetry.Metrics;
using OpenTelemetry.Trace;
using Xunit;

namespace OpenTelemetry.Tests;

public class OpenTelemetrySdkTests
{
[Fact]
public void BuilderDelegateRequiredTest()
{
Assert.Throws<ArgumentNullException>(() => OpenTelemetrySdk.Create(null!));
}

[Fact]
public void NoopProvidersReturnedTest()
{
bool builderDelegateInvoked = false;

using var sdk = OpenTelemetrySdk.Create(builder =>
{
builderDelegateInvoked = true;
Assert.NotNull(builder.Services);
});

Assert.True(builderDelegateInvoked);

Assert.NotNull(sdk);
Assert.NotNull(sdk.Services);
Assert.True(sdk.LoggerProvider is OpenTelemetrySdk.NoopLoggerProvider);
Assert.True(sdk.MeterProvider is OpenTelemetrySdk.NoopMeterProvider);
Assert.True(sdk.TracerProvider is OpenTelemetrySdk.NoopTracerProvider);
Assert.True(sdk.GetLoggerFactory() is NullLoggerFactory);
}

[Fact]
public void ProvidersCreatedAndDisposedTest()
{
var sdk = OpenTelemetrySdk.Create(builder =>
{
builder
.WithLogging()
.WithMetrics()
.WithTracing();
});

var loggerProvider = sdk.LoggerProvider as LoggerProviderSdk;
var meterProvider = sdk.MeterProvider as MeterProviderSdk;
var tracerProvider = sdk.TracerProvider as TracerProviderSdk;

Assert.NotNull(loggerProvider);
Assert.NotNull(meterProvider);
Assert.NotNull(tracerProvider);

Assert.True(sdk.GetLoggerFactory() is not NullLoggerFactory);

sdk.Dispose();

Assert.True(loggerProvider.Disposed);
Assert.True(meterProvider.Disposed);
Assert.True(tracerProvider.Disposed);
}
}