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

[Metrics] Improve dependency injection support in meter provider build-up using SDK #3646

Merged

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Sep 9, 2022

Changes

Brings MeterProviderBuilder up to ~parity with TracerProviderBuilder changes from #3533.

SDK public API

namespace Microsoft.Extensions.DependencyInjection
{
+   public static class MeterProviderBuilderServiceCollectionExtensions
+   {
       // These are basically what existed previously in Hosting minus the IHostedService registration
+      public static IServiceCollection ConfigureOpenTelemetryMetrics(this IServiceCollection services) {}
+      public static IServiceCollection ConfigureOpenTelemetryMetrics(this IServiceCollection services, Action<MeterProviderBuilder> configure) {}
+   }
}

namespace OpenTelemetry.Metrics
{
   public static class MeterProviderBuilderExtensions
   {
       // These existed previously in Hosting, now they are part of the SDK and can be used anywhere SDK is referenced
+      public static MeterProviderBuilder AddInstrumentation<T>(this MeterProviderBuilder meterProviderBuilder) {}
+      public static MeterProviderBuilder AddReader<T>(this MeterProviderBuilder meterProviderBuilder) where T : MetricReader {}
+      public static MeterProviderBuilder ConfigureBuilder(this MeterProviderBuilder meterProviderBuilder, Action<IServiceProvider, MeterProviderBuilder> configure) {}
+      public static MeterProviderBuilder ConfigureServices(this MeterProviderBuilder meterProviderBuilder, Action<IServiceCollection> configure) {}
   }
}

OpenTelemetry.Extensions.Hosting public API

namespace OpenTelemetry.Trace
{
   public static class MeterProviderBuilderExtensions
   {
-      public static MeterProviderBuilder AddInstrumentation<T>(this MeterProviderBuilder meterProviderBuilder) {}
-      public static MeterProviderBuilder AddReader<T>(this MeterProviderBuilder meterProviderBuilder) where T : MetricReader {}
-      public static MeterProvider Build(this MeterProviderBuilder meterProviderBuilder, IServiceProvider serviceProvider) {}

        // Obsoleted these so no one breaks upgrading
+      [Obsolete("Call ConfigureBuilder instead this method will be removed in a future version.")]
       public static MeterProviderBuilder Configure(this MeterProviderBuilder meterProviderBuilder, Action<IServiceProvider, MeterProviderBuilder> configure) {}
+      [Obsolete("Call ConfigureServices instead this method will be removed in a future version.")]
       public static IServiceCollection? GetServices(this MeterProviderBuilder meterProviderBuilder) {}
   }
}

Existing scenarios made available in SDK

using var meterProvider = Sdk.CreateMeterProviderBuilder()
    // Scenario 1: Register services...
    .ConfigureServices(services =>
    {
        services.AddSingleton<MyCustomExporter>();
    })
    // Scenario 2: Add a reader retrieved through DI...
    .AddReader<MyCustomReader>()
    // Scenario 3: Add instrumentation retrieved through DI...
    .AddInstrumentation<MyCustomInstrumentation>()
    // Scenario 4: Configure the builder once the service provider is available...
    .ConfigureBuilder((sp, builder) =>
    {
        builder.AddReader(new BaseExportingMetricReader(sp.GetRequiredService<MyCustomExporter>()));
    })

New scenarios enabled

AddExporter?

For logs + traces we have AddExporter extensions. I didn't add one for metrics. @alanwest and I tried to figure out how it would work, but it is more complicated for metrics. There isn't the same concept of simple/batch processor type and exporters can be push, pull, or both. We decided to tackle this separately or not at all.

Detached configuration method

Some users like @martinjt have asked for a way to configure things completely detached from the builder.

The SDK ConfigureOpenTelemetryMetrics extension is safe to be called multiple times. Only one MeterProvider will exist in the IServiceProvider and multiple calls will all contribute to its configuration, in order. This makes it possible to detach from the MeterProviderBuilder. This enables a scenario like this...

  // ### Application bootstrap services configuration...
  services.AddMyLibrary(); // <- Does detached metric configuration
  // This is using the existing hosting extension which will start the MeterProvider automatically
  services.AddOpenTelemetryMetrics();

  // ### Can be implemented anywhere...
  public static IServiceCollection AddMyLibrary(this IServiceCollection  services)
  {
     services.AddSingleton<MyLibraryService>();
     // This is using the new SDK extension, no dependency on hosting required
     services.ConfigureOpenTelemetryMetrics(builder => builder.AddMeter"MyLibraryMeter"));
  }

TODOs

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Unit tests
  • Changes in public API reviewed

@CodeBlanch CodeBlanch changed the title [Metrics] Improve dependency injection support in metric provider build-up using SDK [Metrics] Improve dependency injection support in meter provider build-up using SDK Sep 9, 2022
@codecov
Copy link

codecov bot commented Sep 9, 2022

Codecov Report

Merging #3646 (91494c5) into main (cea14d3) will increase coverage by 0.19%.
The diff coverage is 91.60%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3646      +/-   ##
==========================================
+ Coverage   87.29%   87.49%   +0.19%     
==========================================
  Files         282      284       +2     
  Lines       10132    10195      +63     
==========================================
+ Hits         8845     8920      +75     
+ Misses       1287     1275      -12     
Impacted Files Coverage Δ
....Hosting/Metrics/MeterProviderBuilderExtensions.cs 0.00% <0.00%> (-68.43%) ⬇️
.../OpenTelemetry/Metrics/CompositeMetricReaderExt.cs 73.91% <75.00%> (ø)
src/OpenTelemetry/Metrics/MeterProviderSdk.cs 89.57% <79.06%> (+0.06%) ⬆️
src/OpenTelemetry/Metrics/CompositeMetricReader.cs 73.13% <83.33%> (ø)
...MeterProviderBuilderServiceCollectionExtensions.cs 85.71% <85.71%> (ø)
...emetry/Metrics/Builder/MeterProviderBuilderBase.cs 96.00% <96.00%> (ø)
...ensions.Hosting/OpenTelemetryServicesExtensions.cs 100.00% <100.00%> (+22.22%) ⬆️
.../Metrics/Builder/MeterProviderBuilderExtensions.cs 82.05% <100.00%> (ø)
...lemetry/Metrics/Builder/MeterProviderBuilderSdk.cs 92.85% <100.00%> (ø)
...der/MeterProviderBuilderServiceCollectionHelper.cs 100.00% <100.00%> (ø)
... and 22 more

@CodeBlanch CodeBlanch marked this pull request as ready for review September 9, 2022 21:16
@CodeBlanch CodeBlanch requested a review from a team as a code owner September 9, 2022 21:16
Copy link
Member

@alanwest alanwest left a comment

Choose a reason for hiding this comment

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

LGTM

@CodeBlanch CodeBlanch merged commit 6ff512c into open-telemetry:main Sep 12, 2022
@CodeBlanch CodeBlanch deleted the meterprovider-dependencyinjection branch September 12, 2022 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants