Skip to content

[Metrics] Clean up repo exporters to use new DI patterns#3648

Merged
CodeBlanch merged 8 commits into
open-telemetry:mainfrom
CodeBlanch:exporter-cleanup-2
Sep 13, 2022
Merged

[Metrics] Clean up repo exporters to use new DI patterns#3648
CodeBlanch merged 8 commits into
open-telemetry:mainfrom
CodeBlanch:exporter-cleanup-2

Conversation

@CodeBlanch

@CodeBlanch CodeBlanch commented Sep 12, 2022

Copy link
Copy Markdown
Member

Changes

  • Updates the metric exporters to use the new DI API surface.

  • Adds support for named options in metric exporter registrations.

    The SdkSupportsMultipleReaders test exposed an issue with our pattern! We are doing serviceProvider.GetRequiredService<IOptions<OptionsType>>() everywhere. That is giving us (more or less) a global options instance (really what it does is just use the default name). Works fine when only a single exporter is registered, but if multiple are in play there be dragons. To solve this I added overloads where users can specify a name to control the options. Users who don't need this don't need to do anything they will just continue to use the default instance.

TODOs

  • CHANGELOG updates.
  • Unit tests.

@CodeBlanch CodeBlanch requested a review from a team September 12, 2022 20:51

@alanwest alanwest left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Named options.. nice!

@codecov

codecov Bot commented Sep 12, 2022

Copy link
Copy Markdown

Codecov Report

Merging #3648 (0cbd128) into main (6ff512c) will increase coverage by 0.24%.
The diff coverage is 98.55%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3648      +/-   ##
==========================================
+ Coverage   87.62%   87.87%   +0.24%     
==========================================
  Files         284      283       -1     
  Lines       10195    10215      +20     
==========================================
+ Hits         8933     8976      +43     
+ Misses       1262     1239      -23     
Impacted Files Coverage Δ
...nTelemetryProtocol/OtlpMetricExporterExtensions.cs 94.11% <95.65%> (+32.57%) ⬆️
...rter.InMemory/InMemoryExporterMetricsExtensions.cs 97.82% <100.00%> (+5.32%) ⬆️
.../PrometheusExporterApplicationBuilderExtensions.cs 100.00% <100.00%> (ø)
...rometheusExporterEndpointRouteBuilderExtensions.cs 100.00% <100.00%> (ø)
...rometheusExporterMeterProviderBuilderExtensions.cs 100.00% <100.00%> (+7.14%) ⬆️
...theusHttpListenerMeterProviderBuilderExtensions.cs 92.85% <100.00%> (+5.35%) ⬆️
src/OpenTelemetry/Metrics/MeterProviderSdk.cs 89.90% <0.00%> (+0.32%) ⬆️
...Telemetry/Internal/SelfDiagnosticsEventListener.cs 97.65% <0.00%> (+0.78%) ⬆️
...metryProtocol/Implementation/ActivityExtensions.cs 96.15% <0.00%> (+1.09%) ⬆️
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs 79.16% <0.00%> (+1.66%) ⬆️
... and 3 more

@CodeBlanch

Copy link
Copy Markdown
Member Author

[Updated the description with info about why the named options got introduced.]

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.

3 participants