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.

Sign up for GitHub

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] Update EventSource implmentation of GenevaMetricExporter #1225

Merged
merged 4 commits into from
Jun 8, 2023
Merged

[Exporter.Geneva] Update EventSource implmentation of GenevaMetricExporter #1225

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
64a55fe
Update EventSource implementation of GenevaMetricExporter to be a sin…
utpilla Jun 8, 2023
da19d0e
Update CHANGELOG
utpilla Jun 8, 2023
0a5c5c6
Update CHANGELOG
utpilla Jun 8, 2023
5da5663
Update CHANGELOG
utpilla Jun 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/OpenTelemetry.Exporter.Geneva/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@
[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
Copy link
Member

Choose a reason for hiding this comment

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

do we have same problem in log/trace exporter?

Copy link
Member

Choose a reason for hiding this comment

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

Similar but slightly different (e.g. most folks can just to two different log ETW providers, while metrics always use the same provider).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, on second thoughts, maybe not? It's probably just tracked at a process level?

EventSource with Guid edc24920-e004-40f6-a8e1-0e6e48f39d84 already exists.`
when using multiple instances of `GenevaMetricExporter` by updating
`MetricEtwDataTransport` to be a singleton.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

([#1225](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/1225))

## 1.5.0-rc.1

Released 2023-Jun-05
Expand Down
2 changes: 1 addition & 1 deletion src/OpenTelemetry.Exporter.Geneva/Metrics/GenevaMetricExporter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ public GenevaMetricExporter(GenevaMetricExporterOptions options)
case TransportProtocol.Unspecified:
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
this.metricDataTransport = new MetricEtwDataTransport();
this.metricDataTransport = MetricEtwDataTransport.Shared;
break;
}
else
Expand Down
9 changes: 8 additions & 1 deletion src/OpenTelemetry.Exporter.Geneva/Metrics/Transport/MetricEtwDataTransport.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,14 @@ internal sealed class MetricEtwDataTransport : EventSource, IMetricDataTransport
{
private readonly int fixedPayloadEndIndex;

public MetricEtwDataTransport()
static MetricEtwDataTransport()
{
Shared = new();
}

public static readonly MetricEtwDataTransport Shared;

private MetricEtwDataTransport()
{
unsafe
{
Expand Down