-
Notifications
You must be signed in to change notification settings - Fork 750
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
Conversation
Could you explain how this will be handled
|
Are you asking about the options? Or just two calls to "AddOtlpExporter" in there? I'm going to assume the later. @alanwest Raised the same question here: #5137 (comment) This isn't the PR for that discussion, really. We'll work through that when we add the But my current thinking is: It is a non-issue 🤣 It will be handled IMO the same way this is handled... using var tracerProvider = Sdk.CreateTracerProviderBuilder()
.AddOtlpExporter()
.AddOtlpExporter()
.Build(); You will get two exporters. There are perfectly valid reasons to do that so I don't think we should assume anything or guess what the user wants. But we can work through it and discuss it more when we get there. |
@CodeBlanch I have a general question regarding the design, could you summarize 1) what you would like to achieve and 2) the design principles? Take one concrete example (note that this is just to facilitate the discussion, we won't be able to enumerate all scenarios, a set of rules are needed here): I want to have OTLP exporter for all 3 signals, however, I want to set the metrics exporting frequency to 10 seconds, and logs/traces to 1 minute. I also want to put my custom redaction processor before the OTLP log exporter. |
What I want to achieve is primarily the simplification of the startup and teardown code. One call to make which initializes the SDK and one call to make to dispose/shutdown the SDK. The design principle is "crawl, walk, run" is there a name for that? 🤣 The secondary goal is to simplify docs and examples using this.
This PR is NOT adding a cross-cutting // New
using var sdk = OpenTelemetrySdk.Create(builder => builder
.ConfigureResource(r => r.AddService("MyApp"))
.WithLogging(builder => builder.AddOtlpExporter())
.WithMetrics(builder => builder.AddOtlpExporter())
.WithTracing(builder => builder.AddOtlpExporter()));
// Old
var resource = ResourceBuilder.CreateDefault()
.AddService("MyApp");
using var loggingProvider = Sdk.CreateLoggerProviderBuilder()
.SetResourceBuilder(resource)
.AddOtlpExporter()
.Build();
using var meterProdier = Sdk.CreateMeterProviderBuilder()
.SetResourceBuilder(resource)
.AddOtlpExporter()
.Build();
using var tracerProvider = Sdk.CreateTracerProviderBuilder()
.SetResourceBuilder(resource)
.AddOtlpExporter()
.Build(); It is a convenience thing allowing the "manual" bootstrap pattern to utilize the "With" pattern we have in hosting. The main benefit is it allows you to track a single thing to dispose. Seems minor, but most code I see does NOT clean up things correctly 😭 ScenariosSince you brought up scenarios let's imagine where we could take this. Crawl: I want to have OTLP exporter for all 3 signals[My guess is this scenario is what most users want to do.] With this PR we don't have a cross-cutting using var sdk = OpenTelemetrySdk.Create(builder => builder
.ConfigureResource(r => r.AddService("MyApp"))
.WithLogging(builder => builder.AddOtlpExporter())
.WithMetrics(builder => builder.AddOtlpExporter())
.WithTracing(builder => builder.AddOtlpExporter())); How would it work once we have a cross-cutting method? using var sdk = OpenTelemetrySdk.Create(builder => builder
.ConfigureResource(r => r.AddService("MyApp"))
.AddOtlpExporter()
.WithLogging()
.WithMetrics()
.WithTracing()); Walk: I want to have OTLP exporter for all 3 signals sending to different endpointsWith this PR we don't have a cross-cutting using var sdk = OpenTelemetrySdk.Create(builder => builder
.ConfigureResource(r => r.AddService("MyApp"))
.WithLogging(builder => builder.AddOtlpExporter(o => o.Endpoint = new("http://mylogendpoint/logs")))
.WithMetrics(builder => builder.AddOtlpExporter(o => o.Endpoint = new("http://mymetricendpoint/metrics")))
.WithTracing(builder => builder.AddOtlpExporter(o => o.Endpoint = new("http://mytraceendpoint/traces")))); How would it work once we have a cross-cutting method? Any number of ways. It depends on what options we expose, the API shape, etc. It could look like this: using var sdk = OpenTelemetrySdk.Create(builder => builder
.ConfigureResource(r => r.AddService("MyApp"))
.AddOtlpExporter(options => {
options.LoggingOptions.Endpoint = new("http://mylogendpoint/logs");
options.MetricsOptions.Endpoint = new("http://mymetricendpoint/metrics");
options.TracingOptions.Endpoint = new("http://mytraceendpoint/trace");
})
.WithLogging()
.WithMetrics()
.WithTracing()); Run: I want to have OTLP exporter for all 3 signals, however, I want to set the metrics exporting frequency to 10 seconds, and logs/traces to 1 minute. I also want to put my custom redaction processor before the OTLP log exporter.Different ways you could achieve this. If we had a simple using var sdk = OpenTelemetrySdk.Create(builder =>
{
builder.Services.Configure<MetricReaderOptions>(o => o.PeriodicExportingMetricReaderOptions.ExportIntervalMilliseconds = 10_000);
builder.Services.Configure<BatchExportActivityProcessorOptions>(o => o.ScheduledDelayMilliseconds = 60_000);
builder.Services.Configure<LogRecordExportProcessorOptions>(o => o.BatchExportProcessorOptions.ScheduledDelayMilliseconds = 60_000);
builder
.ConfigureResource(r => r.AddService("MyApp"))
.WithLogging(logging => logging.AddProcessor(new CustomLogRedactionProcessor()))
.WithMetrics()
.WithTracing()
.AddOtlpExporter();
}); Registration order
Essentially it works the same way we have it today when using the hosting package. Nothing here is really new. The code I have above: builder
.ConfigureResource(r => r.AddService("MyApp"))
.WithLogging(logging => logging.AddProcessor(new CustomLogRedactionProcessor()))
.WithMetrics()
.WithTracing()
.AddOtlpExporter(); Is just orchestrating these calls: services.ConfigureOpenTelemetryLoggerProvider(builder => builder.ConfigureResource(r => r.AddService("MyApp")));
services.ConfigureOpenTelemetryMeterProvider(builder => builder.ConfigureResource(r => r.AddService("MyApp")));
services.ConfigureOpenTelemetryTracerProvider(builder => builder.ConfigureResource(r => r.AddService("MyApp")));
services.ConfigureOpenTelemetryLoggerProvider(builder => builder.AddProcessor(new CustomLogRedactionProcessor()));
services.ConfigureOpenTelemetryLoggerProvider(builder => builder.AddOtlpExporter());
services.ConfigureOpenTelemetryMeterProvider(builder => builder.AddOtlpExporter());
services.ConfigureOpenTelemetryTracerProvider(builder => builder.AddOtlpExporter()); The order in which they are called is the order in which they are applied. Same ordering requirements are actually in play for the APIs we have today... This user is happy: using var loggerFactory = LoggerFactory.Create(builder => builder
.AddOpenTelemetry(options => options
.AddProcessor(new CustomLogRedactionProcessor()))
.AddOtlpExporter()); This user is sad: using var loggerFactory = LoggerFactory.Create(builder => builder
.AddOpenTelemetry(options => options
.AddOtlpExporter()
.AddProcessor(new CustomLogRedactionProcessor()))); |
This is exactly why I think it is unclear, the PR description mentioned: using var openTelemetry = OpenTelemetrySdk.Create(builder => builder
.ConfigureResource(r => r.AddService("MyService"))
.AddOtlpExporter() // (Future thing)
.AddConsoleExporter() // (Future thing)
.WithTracing()
.WithLogging() // (Experimental API)
.WithMetrics());
// To access the providers:
// openTelemetry.LoggerProvider (Experimental API)
// openTelemetry.MeterProvider
// openTelemetry.TracerProvider
// To get the ILoggerFactory:
// openTelemetry.GetLoggerFactory(); What I want - the overall goal/direction, I cannot review a concrete PR without first understanding what's the overall goal, if the PR is adding a new way of doing things. Specifically, I'm concerned that we end up with a set of incomplete stories "as a user, you can choose path A, B or C, and all of them come with caveats that you should know ahead of time". |
This brings me some pause as well. My original impression was that manifesting this was the north star for all this work. If it remains uncertain whether this goal will be achievable, then I agree more thought and design is required before we commit to landing anything. This may entail reevaluating the work already landed in #5265 - at a bare minimum, it sounds like the new APIs introduced in #5265 should be marked experimental, so that in the event we don't end up shipping any cross-cutting extensions we can delete the experimental APIs. I think the A number of open questions were raised by folks in a recent SIG meeting which I captured in this comment |
Let's merge this and I can do a follow-up which makes it all experimental? |
I disagree. |
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
/// </summary> | ||
/// <param name="configure"><see cref="IOpenTelemetryBuilder"/> configuration delegate.</param> | ||
/// <returns>Created <see cref="OpenTelemetrySdk"/>.</returns> | ||
public static OpenTelemetrySdk Create( |
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.
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?
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
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.
LGTM.
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
Relates to #5137
Relates to #5265
Relates to #5648
Changes
OpenTelemetrySdk
API.Details
#5265 laid the groundwork for cross-cutting extensions (ex #5400). The issue is (currently) only users of
OpenTelemetry.Extensions.Hosting
will be able to utilize those. What this PR does is add a pattern in SDK for non-hosting users (primarily .NET Framework) to utilize all the cross-cutting things. It also has the benefit of potentially unifying our startup/configuration story/documentation so everything uses the same "With" style (see #5262 (comment)).We have already seen distros working around this gap by rolling their own custom builders for example: https://github.com/elastic/elastic-otel-dotnet/blob/main/src/Elastic.OpenTelemetry/ElasticOpenTelemetryBuilder.cs
Usage
New style:
Old style:
Migration story enabled for LoggerFactory.Create to LoggerProviderBuilder
#5648 brought
LoggerProviderBuilder
&WithLogging
into the stable public API. Users callingLoggerFactory.Create
today don't currently have an API which exposesLoggerProviderBuilder
. The API introduced on this PR can be used to migrate users callingLoggerFactory.Create
to theWithLogging
style:Before:
After:
Public API changes
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes