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

Hosting: Support metrics registration via IServiceCollection & deferred configuration #2412

Merged
merged 11 commits into from
Sep 27, 2021

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Sep 26, 2021

Changes

  • Adds AddOpenTelemetryMetrics extensions on IServiceCollection
  • Renames AddMetricReader -> AddReader. This seemed more consistent with the other methods (AddProcessor, SetSampler, AddInstrumentation, etc.)
  • Adds IDeferredMeterProviderBuilder to support DI scenarios

Details

Follows closely the same patterns used for TracerProviderBuilder + extensions.

TODOs

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

@CodeBlanch CodeBlanch requested a review from a team September 26, 2021 00:03
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

Comment on lines +126 to +129
if (services is null)
{
throw new ArgumentNullException(nameof(services));
}
Copy link
Member

@pellared pellared Sep 27, 2021

Choose a reason for hiding this comment

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

nit: Maybe it would be better to add this null guard to public methods?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to vote to leave as-is on this one. Reasoning: services is the thing being extended. It is unlikely to be null unless someone is going out of their way to call it directly AND pass null. So given that should be rare, having it here saves having to duplicate the check in all the publics. 2 checks instead of 4, basically.

@codecov
Copy link

codecov bot commented Sep 27, 2021

Codecov Report

Merging #2412 (5f50191) into main (8f284e0) will decrease coverage by 0.04%.
The diff coverage is 77.57%.

❗ Current head 5f50191 differs from pull request most recent head 0643496. Consider uploading reports for the commit 0643496 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2412      +/-   ##
==========================================
- Coverage   80.27%   80.23%   -0.05%     
==========================================
  Files         232      235       +3     
  Lines        7505     7564      +59     
==========================================
+ Hits         6025     6069      +44     
- Misses       1480     1495      +15     
Impacted Files Coverage Δ
...rter.InMemory/InMemoryExporterMetricsExtensions.cs 0.00% <0.00%> (ø)
...nTelemetryProtocol/OtlpMetricExporterExtensions.cs 0.00% <0.00%> (ø)
...Telemetry/Trace/TracerProviderBuilderExtensions.cs 88.23% <0.00%> (ø)
...ensions.Hosting/OpenTelemetryServicesExtensions.cs 64.70% <64.70%> (ø)
....Hosting/Metrics/MeterProviderBuilderExtensions.cs 68.42% <68.42%> (ø)
...s.Hosting/Implementation/TelemetryHostedService.cs 69.23% <75.00%> (-0.77%) ⬇️
...ing/Implementation/TracerProviderBuilderHosting.cs 85.71% <75.00%> (-6.60%) ⬇️
...ting/Implementation/MeterProviderBuilderHosting.cs 85.71% <85.71%> (ø)
...elemetry/Metrics/MeterProviderBuilderExtensions.cs 72.72% <85.71%> (-5.06%) ⬇️
.../OpenTelemetry/Metrics/MeterProviderBuilderBase.cs 88.88% <88.88%> (ø)
... and 9 more

@cijothomas
Copy link
Member

@CodeBlanch
Since https://github.com/open-telemetry/opentelemetry-dotnet/pull/2411/files is merged, this PR requires some updates to remove unused using statements else CI will fail.

@CodeBlanch CodeBlanch merged commit 38ee521 into open-telemetry:main Sep 27, 2021
@CodeBlanch CodeBlanch deleted the hosting-metrics branch September 27, 2021 17:30
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.

4 participants