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] Move OpenTelemetryBuilder to SDK and add OpenTelemetrySdk API #5137

Closed

Conversation

CodeBlanch
Copy link
Member

Relates to #4940

Changes

  • Moves OpenTelemetryBuilder from OpenTelemetry.Extensions.Hosting into OpenTelemetry (SDK).
  • Adds OpenTelemetrySdk API.

Details

We have a request from .NET Aspire team to add a cross-cutting AddOtlpExporter extension. I was able to make it work by defining an IOpenTelemetryBuilder interface in SDK and using some generic gymnastics on the extension method so the builder type is preserved but it kind of hack.

What this PR does is move OpenTelemetryBuilder fully into the SDK so that extensions may target it directly.

Taking that a step further I also defined an OpenTelemetrySdk API. What this API does is make sure our .NET Framework friends are not left behind and gives them a path to utilize cross-cutting things. It also has the benefit of potentially unifying our startup/configuration story/documentation so everything uses the same "With" style.

New style:

        using var openTelemetry = OpenTelemetrySdk.Create(builder => builder
            .ConfigureResource(r => r.AddService("MyService"))
            .AddOtlpExporter()
            .WithTracing()
            .WithLogging() // (Experimental API)
            .WithMetrics());

        // To access the providers:
        // openTelemetry.LoggerProvider (Experimental API)
        // openTelemetry.MeterProvider
        // openTelemetry.TracerProvider

        // To get the ILoggerFactory:
        // openTelemetry.GetLoggerFactory(); (Experimental API)

Old style:

        var resourceBuilder = ResourceBuilder.CreateDefault().AddService("MyService");

        using var tracerProvider = Sdk.CreateTracerProviderBuilder()
            .SetResourceBuilder(resourceBuilder)
            .AddOtlpExporter()
            .Build();

        using var meterProvider = Sdk.CreateMeterProviderBuilder()
            .SetResourceBuilder(resourceBuilder)
            .AddOtlpExporter()
            .Build();

        // Experimental API
        using var loggerProvider = Sdk.CreateLoggerProviderBuilder()
            .SetResourceBuilder(resourceBuilder)
            .AddOtlpExporter()
            .Build();

        using var loggerFactory = LoggerFactory.Create(logging =>
        {
            logging.AddOpenTelemetry(builder => builder
                .SetResourceBuilder(resourceBuilder)
                .AddOtlpExporter());
        });

Merge requirement checklist

  • CONTRIBUTING guidelines followed (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)

Copy link

codecov bot commented Dec 6, 2023

Codecov Report

Attention: 27 lines in your changes are missing coverage. Please review.

Comparison is base (6250307) 83.38% compared to head (04a61dc) 82.92%.
Report is 25 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5137      +/-   ##
==========================================
- Coverage   83.38%   82.92%   -0.46%     
==========================================
  Files         297      273      -24     
  Lines       12531    11995     -536     
==========================================
- Hits        10449     9947     -502     
+ Misses       2082     2048      -34     
Flag Coverage Δ
unittests ?
unittests-Solution-Experimental 82.89% <0.00%> (?)
unittests-Solution-Stable 82.68% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...sListener/OpenTelemetryMetricsBuilderExtensions.cs 90.00% <ø> (ø)
...s/IMetricsListener/OpenTelemetryMetricsListener.cs 90.47% <ø> (ø)
src/OpenTelemetry/OpenTelemetryBuilder.cs 100.00% <ø> (ø)
src/OpenTelemetry/OpenTelemetrySdkExtensions.cs 0.00% <0.00%> (ø)
src/OpenTelemetry/OpenTelemetrySdk.cs 0.00% <0.00%> (ø)

... and 32 files with indirect coverage changes

Copy link
Contributor

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.

@github-actions github-actions bot added the Stale label Dec 15, 2023
@github-actions github-actions bot removed the Stale label Dec 16, 2023
Copy link
Contributor

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.

@github-actions github-actions bot added the Stale label Dec 23, 2023
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Dec 31, 2023
@CodeBlanch CodeBlanch reopened this Jan 8, 2024
@github-actions github-actions bot removed the Stale label Jan 9, 2024
Copy link
Contributor

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.

@github-actions github-actions bot added the Stale label Jan 16, 2024
@alanwest alanwest removed the Stale label Jan 16, 2024
@alanwest
Copy link
Member

alanwest commented Jan 16, 2024

Capturing the thoughts/concerns/questions raised from our discussion in last week's SIG meeting.

  1. Concern: new SDK dependency on Microsoft.Extensions.Diagnostics.Abstractions
  2. Can we move OpenTelemetry builder API to OpenTelemetry.Api.ProviderBuilders package instead of in the SDK package directly?
  3. Multiple ways to configure the OTLP exporter could cause confusion (i.e., via the new cross-cutting extension plus the existing extensions available via WithTracing/WithMetrics).
  4. With the new cross-cutting AddOtlpExporter method, how would we (if at all) support different configuration for traces, metrics, and logs?
  5. Currently, if someone were to configure the OTLP exporter using the cross-cutting method and also configure it via WithTracing/WithMetrics, this would result in two exporter. This may not be intuitive and produce unexpected results. Would it be possible for this to instead result in one exporter instance with the config somehow merged?
  6. Need to sort out how configuring the batch export processor and metric reader settings works when using the cross-cutting AddOtlpExporter method.

My personal thoughts:

I support moving the OpenTelemetryBuilder out of the OpenTelemetry.Extensions.Hosting package. Ideally, it belongs in OpenTelemetry.Api.ProviderBuilders. I think I am ok with the new dependency on Microsoft.Extensions.Diagnostics.Abstractions to achieve these ends.

I do not have major concerns about any confusion with multiple ways of configuring the OTLP exporter, but I think we need to discuss points 3-6 further.

Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants