[Exporter.Geneva] Update EventSource implmentation of GenevaMetricExporter #1225
Conversation
utpilla
added
the
comp:exporter.geneva
github-actions
bot
requested review from
cijothomas,
CodeBlanch,
reyang and
Yun-Ting
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1225 +/- ##
==========================================
+ Coverage 73.17% 74.08% +0.90%
==========================================
Files 260 265 +5
Lines 9133 9349 +216
==========================================
+ Hits 6683 6926 +243
+ Misses 2450 2423 -27
|
| @@ -11,6 +11,10 @@ | |||
| [RS0026](https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/Microsoft.CodeAnalysis.PublicApiAnalyzers.md#rs0026-do-not-add-multiple-public-overloads-with-optional-parameters). | |||
| ([#1218](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/1218)) | |||
|
|
|||
| * Update `MetricEtwDataTransport` which is the `EventSource` implementation for | |||
There was a problem hiding this comment.
I guess the user would be very confused by this message since they are not supposed to understand the internals (e.g. MetricEtwDataTransport might mean nothing to them).
Do we really need a changelog entry? (If yes, I suggest that at least we rewrite in a way that the consumer/user would understand)
There was a problem hiding this comment.
I added this CHANGELOG entry so that we can easily figure out which version got this fix.
Update the wording.
There was a problem hiding this comment.
so that we can easily figure out which version got this fix.
I guess we want to look at the commit history (e.g. git log | grep ...) rather than CHANGELOG (which is for the users, not the maintainers/owners)?
There was a problem hiding this comment.
I think it's much quicker to first look for any noteworthy changes for a given release in the CHANGELOG, than going through git log which is the comprehensive source of truth.
| * Fix the issue of running into the `ArgumentException`: `An instance of | ||
| EventSource with Guid edc24920-e004-40f6-a8e1-0e6e48f39d84 already exists.` | ||
| when using multiple instances of `GenevaMetricExporter` by updating | ||
| `MetricEtwDataTransport` to be a singleton. |
There was a problem hiding this comment.
I suggest that we remove "by updating ..." to make it concise (my take is that the reader of the changelog would want to focus on "what does it mean to me" rather than "how you've implemented it", and for folks who have curiosity to know the details, they can always go to the PR - which is why we put the PR link).
I don't feel strongly about this though, so not a blocker.
There was a problem hiding this comment.
my take is that the reader of the changelog would want to focus on "what does it mean to me" rather than "how you've implemented it", and for folks who have curiosity to know the details, they can always go to the PR - which is why we put the PR link
I think CHANGELOG could be meant for developers as well? For example, look at this entry in the collector CHANGELOG under v0.79.0:
- batchprocessor: Change multiBatcher to use sync.Map, avoid global lock on fast path (#7714)
This is too much detail for users but it gives a good reference point for developers when they quickly want to identify which version and above have that change.
| @@ -11,6 +11,11 @@ | |||
| [RS0026](https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/Microsoft.CodeAnalysis.PublicApiAnalyzers.md#rs0026-do-not-add-multiple-public-overloads-with-optional-parameters). | |||
| ([#1218](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/1218)) | |||
|
|
|||
| * Fix the issue of running into the `ArgumentException`: `An instance of | |||
There was a problem hiding this comment.
do we have same problem in log/trace exporter?
There was a problem hiding this comment.
Similar but slightly different (e.g. most folks can just to two different log ETW providers, while metrics always use the same provider).
There was a problem hiding this comment.
@reyang @cijothomas I was wondering if we need to worry about scenarios where there are multiple instances of an application running each with the same GenevaExporter setup. They would run into this issue.
There was a problem hiding this comment.
Actually, on second thoughts, maybe not? It's probably just tracked at a process level?
Fixes #1192
Changes
EventSourceimplementation ofGenevaMetricExporterto be a SingletonCHANGELOG.mdupdated for non-trivial changes