-
Notifications
You must be signed in to change notification settings - Fork 778
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
Changes from all commits
0d84db1
773f90a
617317c
a201153
74d9d9e
59c3125
fdf754e
b5872d8
a52d079
4a58bc4
456354f
2e178c1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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! |
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( | ||
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; } | ||
} | ||
} |
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Are there other options that might minimize the need for refactoring? Would an additional overload of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I hesitate to do this. There is an ILoggerFactory.AddProvider API. BUT it is after There is also an issue if we add an extension something like What other options do we have?
|
||
{ | ||
Guard.ThrowIfNull(sdk); | ||
|
||
return (ILoggerFactory?)sdk.Services.GetService(typeof(ILoggerFactory)) | ||
?? NoopLoggerFactory; | ||
} | ||
} |
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); | ||
} | ||
} |
There was a problem hiding this comment.
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.
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 usesOpenTelemetrySdk.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 doingSdk.CreateTracerProviderBuilder()
and they upgrade some library which begins doingOpenTelemetrySdk.Create
and suddenly blows up the host.The initial concern over...
My question is, do we really have an issue here? Users can already find themselves in odd situations today:
That is basically the same thing you are concerned about, no?
What we could do is
Obsolete
the "Sdk.Create*" APIs in favor ofOpenTelemetrySdk.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?