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

[api+sdk] Groundwork for cross-cutting builder extensions #5265

Merged
merged 9 commits into from
Feb 7, 2024

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Jan 26, 2024

Relates to #5137

Changes

  • Adds IOpenTelemetryBuilder in API (extensions).
  • Adds IOpenTelemetryBuilder extensions in SDK which now drive the "With" operations we have today.

Details

The goal is to allow components to author extensions which target all signals (see #4940):

public static IOpenTelemetryBuilder AddOtlpExporter(this IOpenTelemetryBuilder builder) {}
  • We don't want to force things to take a dependency on OpenTelemetry.Extensions.Hosting to do this. This is why IOpenTelemetryBuilder has been introduced.

  • We don't want to force things to take a dependency on SDK to do this. This is why IOpenTelemetryBuilder is going into OpenTelemetry.Api.ProviderBuilderExtensions.

  • The "With" extensions in SDK are being added to handle "type switching" which will result anytime a cross-cutting extension is invoked:

    Today:

    services.AddOpenTelemetry()
        .ConfigureResource(...) // invoke OpenTelemetryBuilder.ConfigureResource instance method
        .WithTracing(...) // invoke OpenTelemetryBuilder.WithTracing instance method
        .WithMetrics(...) // invoke OpenTelemetryBuilder.WithMetrics instance method

    Future:

    services.AddOpenTelemetry()
        .AddOtlpExporter() // invoke extension on IOpenTelemetryBuilder and return IOpenTelemetryBuilder
        .ConfigureResource(...) // invoke IOpenTelemetryBuilder ConfigureResource SDK extension method
        .WithTracing(...) // invoke IOpenTelemetryBuilder WithTracing SDK extension method
        .WithMetrics(...) // invoke IOpenTelemetryBuilder WithMetrics SDK extension method

A secondary goal this also lays groundwork for is to offer a new bootstrap pattern for .NET Framework (really manual/non-host sdk management) which uses the "With" pattern our hosting package offers. This was explored a bit on #5137 and mentioned here. That pattern is more concise, allows the cross-cutting extensions to be used more widely, and (hopefully) simplifies our documentation.

Public API Changes

  • OpenTelemetry.Api.ProviderBuilderExtensions
namespace OpenTelemetry;

+public interface IOpenTelemetryBuilder
+{
+    IServiceCollection Services { get; }
+}
  • OpenTelemetry
namespace OpenTelemetry;

+ public static class OpenTelemetryBuilderSdkExtensions
+ {
+     public static IOpenTelemetryBuilder ConfigureResource(this IOpenTelemetryBuilder builder, Action<ResourceBuilder> configure) {}
+     public static IOpenTelemetryBuilder WithMetrics(this IOpenTelemetryBuilder builder) {}
+     public static IOpenTelemetryBuilder WithMetrics(this IOpenTelemetryBuilder builder, Action<MeterProviderBuilder> configure) {}
+     public static IOpenTelemetryBuilder WithTracing(this IOpenTelemetryBuilder builder) {}
+     public static IOpenTelemetryBuilder WithTracing(this IOpenTelemetryBuilder builder, Action<TracerProviderBuilder> configure) {}

#if EXPOSE_EXPERIMENTAL_FEATURES
+     public static IOpenTelemetryBuilder WithLogging(this IOpenTelemetryBuilder builder) {}
+     public static IOpenTelemetryBuilder WithLogging(this IOpenTelemetryBuilder builder, Action<LoggerProviderBuilder> configure) {}
+     public static IOpenTelemetryBuilder WithLogging(this IOpenTelemetryBuilder builder, Action<LoggerProviderBuilder>? configureBuilder, Action<OpenTelemetryLoggerOptions>? configureOptions) {}
#endif
+ }

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated There are existing unit tests in Hosting for the extensions being added to SDK. I decided to leave them there for now. They really should be moved into the SDK tests now but that is going to take some refactoring. I'll do that separately so as to not explode the diff.
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@CodeBlanch CodeBlanch added pkg:OpenTelemetry.Api Issues related to OpenTelemetry.Api NuGet package pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package pkg:OpenTelemetry.Extensions.Hosting Issues related to OpenTelemetry.Extensions.Hosting NuGet package labels Jan 26, 2024
Copy link

codecov bot commented Jan 26, 2024

Codecov Report

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

Comparison is base (6250307) 83.38% compared to head (cb1947f) 83.04%.
Report is 62 commits behind head on main.

❗ Current head cb1947f differs from pull request most recent head ce5b220. Consider uploading reports for the commit ce5b220 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5265      +/-   ##
==========================================
- Coverage   83.38%   83.04%   -0.35%     
==========================================
  Files         297      273      -24     
  Lines       12531    11982     -549     
==========================================
- Hits        10449     9950     -499     
+ Misses       2082     2032      -50     
Flag Coverage Δ
unittests ?
unittests-Solution-Experimental 82.97% <82.35%> (?)
unittests-Solution-Stable 83.03% <82.35%> (?)

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

Files Coverage Δ
....Api/Context/Propagation/TraceContextPropagator.cs 90.00% <100.00%> (+0.52%) ⬆️
...etryProtocol/Implementation/ExperimentalOptions.cs 100.00% <ø> (ø)
...tation/OpenTelemetryProtocolExporterEventSource.cs 100.00% <100.00%> (ø)
...rotocol/Implementation/OtlpLogRecordTransformer.cs 93.45% <100.00%> (ø)
...lemetry.Extensions.Hosting/OpenTelemetryBuilder.cs 85.71% <100.00%> (-14.29%) ⬇️
...tation.AspNetCore/Implementation/HttpInListener.cs 89.79% <100.00%> (+0.21%) ⬆️
...AspNetCore/Implementation/HttpInMetricsListener.cs 89.74% <100.00%> (+0.26%) ⬆️
...ntation.GrpcNetClient/GrpcClientInstrumentation.cs 100.00% <100.00%> (ø)
...NetClient/GrpcClientTraceInstrumentationOptions.cs 100.00% <ø> (ø)
...ent/Implementation/GrpcClientDiagnosticListener.cs 75.80% <100.00%> (-2.77%) ⬇️
... and 17 more

... and 32 files with indirect coverage changes

@CodeBlanch CodeBlanch marked this pull request as ready for review January 31, 2024 22:15
@CodeBlanch CodeBlanch requested a review from a team January 31, 2024 22:15
Copy link
Contributor

@rajkumar-rangaraj rajkumar-rangaraj left a comment

Choose a reason for hiding this comment

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

LGTM.

@alanwest alanwest merged commit 3fa189a into open-telemetry:main Feb 7, 2024
37 checks passed
@CodeBlanch CodeBlanch deleted the api-iopentelemetrybuilder branch February 7, 2024 01:26
Mpdreamz added a commit to elastic/elastic-otel-dotnet that referenced this pull request Mar 4, 2024
This is not yet released see open-telemetry/opentelemetry-dotnet#5265 for more background.

For now we include a temporary copy with hacks to call internal bits.

This allows AgentBuilder to be a native opentelemetry builder and we'd inherit all extension methods from the OpenTelemetry community
Mpdreamz added a commit to elastic/elastic-otel-dotnet that referenced this pull request Mar 5, 2024
* Move our AgentBuilder to IOpenTelemetryBuilder

This is not yet released see open-telemetry/opentelemetry-dotnet#5265 for more background.

For now we include a temporary copy with hacks to call internal bits.

This allows AgentBuilder to be a native opentelemetry builder and we'd inherit all extension methods from the OpenTelemetry community

* clean up AddElasticOpenTelemetry()
Mpdreamz added a commit to elastic/elastic-otel-dotnet that referenced this pull request Mar 5, 2024
* Move our AgentBuilder to IOpenTelemetryBuilder

This is not yet released see open-telemetry/opentelemetry-dotnet#5265 for more background.

For now we include a temporary copy with hacks to call internal bits.

This allows AgentBuilder to be a native opentelemetry builder and we'd inherit all extension methods from the OpenTelemetry community

* clean up AddElasticOpenTelemetry()

* stage

* add netstandard2.0 and 2.1

* add net462

* add net6.0

* update release folder name now that we have multiple TFMs

* rename TraceParentRe
@cmeeren
Copy link

cmeeren commented Apr 12, 2024

Could the migration path be clarified?

I currently have this, to set up telemetry with HotChocolate, as explained a few snippets down from this heading:

builder.Services
    .AddOpenTelemetry()
    .WithTracing(fun b ->
        b.AddHotChocolateInstrumentation().SetResourceBuilder(otlResourceBuilder)
        |> ignore

        let connStr =
            builder.Configuration.GetValue<string> "APPLICATIONINSIGHTS_CONNECTION_STRING"

        if not (String.isNullOrEmpty connStr) then
            b.AddAzureMonitorTraceExporter(fun o -> o.ConnectionString <- connStr) |> ignore
        |> ignore
    )
|> ignore

After updating OpenTelemetry.Extensions.Hosting, this now fails compilation (due to warnings as errors) because of the obsoletion. Based on the OP here, I don't understand what I can change to make this work.

@StachuDotNet
Copy link

Wondering the same thing as @cmeeren. Any advice would be appreciated, @CodeBlanch :)

@CodeBlanch
Copy link
Member Author

@cmeeren @StachuDotNet This is interesting. It seems the same code in C# works fine but for some reason with F# it warns. I added this to discuss on our SIG meeting today. I'm considering just removing the obsoletion. I'll report back after our discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:OpenTelemetry.Api Issues related to OpenTelemetry.Api NuGet package pkg:OpenTelemetry.Extensions.Hosting Issues related to OpenTelemetry.Extensions.Hosting NuGet package pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants