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

Attempt to reduce provider lifetime issues #2848

Merged
merged 4 commits into from
Feb 2, 2022

Conversation

cijothomas
Copy link
Member

@cijothomas cijothomas commented Feb 2, 2022

Have seen users not managing the provider lifetime correctly. Partly because of copy-paste from console example: https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/docs/metrics/getting-started/Program.cs#L28, to other workloads like ASP.NET, without changes.

This also adds a log, so self-diag can offer some hints about the issue, in some situations.

@cijothomas cijothomas requested a review from a team February 2, 2022 20:43
Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov
Copy link

codecov bot commented Feb 2, 2022

Codecov Report

Merging #2848 (83df085) into main (c1c5436) will decrease coverage by 0.17%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2848      +/-   ##
==========================================
- Coverage   83.91%   83.74%   -0.18%     
==========================================
  Files         250      250              
  Lines        8877     8882       +5     
==========================================
- Hits         7449     7438      -11     
- Misses       1428     1444      +16     
Impacted Files Coverage Δ
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs 72.22% <100.00%> (-2.31%) ⬇️
.../OpenTelemetry/Logs/OpenTelemetryLoggerProvider.cs 81.63% <100.00%> (+0.38%) ⬆️
src/OpenTelemetry/Metrics/MeterProviderSdk.cs 87.13% <100.00%> (+0.05%) ⬆️
src/OpenTelemetry/Trace/TracerProviderSdk.cs 96.49% <100.00%> (+0.01%) ⬆️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs 36.36% <0.00%> (-22.73%) ⬇️
...entation/ExportClient/OtlpGrpcTraceExportClient.cs 35.71% <0.00%> (-14.29%) ⬇️
...mentation/ExportClient/BaseOtlpGrpcExportClient.cs 50.00% <0.00%> (-12.50%) ⬇️
...tation/OpenTelemetryProtocolExporterEventSource.cs 75.00% <0.00%> (-10.00%) ⬇️
...lemetry/Internal/SelfDiagnosticsConfigRefresher.cs 86.53% <0.00%> (-2.89%) ⬇️
... and 1 more

@cijothomas cijothomas merged commit 3833a4f into open-telemetry:main Feb 2, 2022
@cijothomas cijothomas deleted the cijothomas/docfixmin2 branch February 2, 2022 21:57
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.

Late to the 🎉, but LGTM.

An additional thought might be to include some lifetime guidance in some of the XML comments. Maybe on the Create*ProviderBuilder methods? https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry/Sdk.cs#L57-L74

I could take a stab...

@cijothomas
Copy link
Member Author

Late to the 🎉, but LGTM.

An additional thought might be to include some lifetime guidance in some of the XML comments. Maybe on the Create*ProviderBuilder methods? https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry/Sdk.cs#L57-L74

I could take a stab...

Yes please!

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.

5 participants