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

[Exporter.Geneva] Skip dispose for ETW singleton of GenevaMetricExporter #1537

Conversation

xiang17
Copy link
Contributor

@xiang17 xiang17 commented Jan 18, 2024

Fixes #1520. The current code disposes the MetricEtwDataTransport singleton when an instance of GenevaMetricExporter is disposed. However, the singleton shouldn't be disposed. Future instances of GenevaMetricExporter will still need to reference it to send data to ETW.

This only happens on Windows, because ETW only allows one instance of EventSource with the same Guid (#1225). On Linux, such limit doesn't exist: GenevaMetricExporter uses Unix Domain Socket to send Metrics data.

Changes

Fix a bug in GenevaMetricExporter where the MetricEtwDataTransport singleton is disposed.

Tested manually with the program in the issue. Didn't add tests in this PR since it's a bit tricky. We don't have any test of checking whether data is actually sent into ETW. Might have to use a EventListener to read and parse data from the ETW.

For significant contributions please make sure you have completed the following items:

  • Appropriate CHANGELOG.md updated for non-trivial changes

Copy link

codecov bot commented Jan 18, 2024

Codecov Report

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

Comparison is base (71655ce) 73.91% compared to head (b23bab8) 58.09%.
Report is 134 commits behind head on main.

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

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #1537       +/-   ##
===========================================
- Coverage   73.91%   58.09%   -15.83%     
===========================================
  Files         267       29      -238     
  Lines        9615     3064     -6551     
===========================================
- Hits         7107     1780     -5327     
+ Misses       2508     1284     -1224     
Flag Coverage Δ
unittests-Exporter.Geneva 58.09% <55.14%> (?)

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

Files Coverage Δ
....Exporter.Geneva/GenevaExporterHelperExtensions.cs 100.00% <ø> (+31.81%) ⬆️
...Telemetry.Exporter.Geneva/GenevaExporterOptions.cs 90.00% <ø> (ø)
...OpenTelemetry.Exporter.Geneva/GenevaLogExporter.cs 85.00% <ø> (+7.50%) ⬆️
...lemetry.Exporter.Geneva/GenevaLoggingExtensions.cs 100.00% <ø> (+14.28%) ⬆️
...enTelemetry.Exporter.Geneva/GenevaTraceExporter.cs 82.50% <ø> (+7.50%) ⬆️
...xporter.Geneva/Internal/ConnectionStringBuilder.cs 92.94% <100.00%> (ø)
...ry.Exporter.Geneva/Internal/ExporterEventSource.cs 14.28% <ø> (+9.52%) ⬆️
...eneva/Internal/ReentrantActivityExportProcessor.cs 100.00% <ø> (ø)
...porter.Geneva/Internal/ReentrantExportProcessor.cs 100.00% <ø> (ø)
...ry.Exporter.Geneva/Internal/TableNameSerializer.cs 98.93% <ø> (ø)
... and 18 more

... and 233 files with indirect coverage changes

@Kielek Kielek added the comp:exporter.geneva Things related to OpenTelemetry.Exporter.Geneva label Jan 18, 2024
@rajkumar-rangaraj
Copy link
Contributor

Could you explain in the PR description how the current changes address the issue?

@xiang17
Copy link
Contributor Author

xiang17 commented Jan 23, 2024

Could you explain in the PR description how the current changes address the issue?

I've updated the description with details.

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.

@cijothomas cijothomas merged commit 8f72b75 into open-telemetry:main Feb 2, 2024
32 checks passed
@xiang17 xiang17 deleted the xiang17/GenevaExporterFixSingletonDisposeBug branch February 3, 2024 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:exporter.geneva Things related to OpenTelemetry.Exporter.Geneva
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Geneva metric exporter disposes shared transport object
9 participants