[Instrumentation.EventCounters] Stabilize and Enable Testing

[mic-max]

Towards #215 since spec is not stable

Changes

• Cleans up code for EventCounters instrumentation
• Fixes tests
• Only create counters for EventCounters named events by default and that are from a configured EventSource name, which includes nothing by default.

Resources

• https://docs.microsoft.com/en-us/dotnet/core/diagnostics/event-counters
• Similar class
• https://github.com/dotnet/diagnostics/blob/main/documentation/design-docs/eventcounters.md
• OpenTelemetry.Instrumentation.Wcf/TracerProviderBuilderExtensions copied implementation of only allowing a single addition of the instrumentation.

Follow-up PR Plans

• Multiple counters from the same source with different names
• Multiple counters from the same source with the same name
• Call meterProvider.AddEventCounterListener multiple times.
• Verify stress testing the instrumentation does not incur memory leaks, or slowdowns.
• Consider making a Meter for each of the EventSources listened to. (this might make any future wildcard functionality being added to AddEventSources more complicated...)
• Add tests that ensure EventSource warning logs are created when expected. (use InMemoryEventListener vs. TestEventListener?)

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

• Appropriate CHANGELOG.md updated for non-trivial changes

[codecov]

Codecov Report

Merging #620 (65521ee) into main (7fb378d) will decrease coverage by 0.06%.
The diff coverage is 77.94%.

❗ Current head 65521ee differs from pull request most recent head a6bfef2. Consider uploading reports for the commit a6bfef2 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #620      +/-   ##
==========================================
- Coverage   77.76%   77.69%   -0.07%     
==========================================
  Files         175      175              
  Lines        5261     5249      -12     
==========================================
- Hits         4091     4078      -13     
- Misses       1170     1171       +1     

Impacted Files	Coverage Δ
...ounters/EventCountersInstrumentationEventSource.cs	27.27% <16.66%> (+27.27%)	⬆️
...trumentation.EventCounters/EventCountersMetrics.cs	80.76% <80.76%> (ø)
...entCounters/EventCountersInstrumentationOptions.cs	100.00% <100.00%> (ø)
...on.EventCounters/MeterProviderBuilderExtensions.cs	100.00% <100.00%> (ø)

[noahfalk]

Thanks! I like this feature for OTel : ) I left some suggestions inline

[utpilla]

The PR looks good. There are some duplicate checks with instrument names that should be avoided.

[utpilla]

Could eventData be null?

[utpilla]

You could move the try-catch block from UpdateInstrumentWithEvent to here to include the Convert.ToDouble method in the try block as that could also throw exceptions.

[utpilla]

Is this check required?

[mic-max]

Ya, just dropping an event if the constructor hasn't finished setting the options field. I'll add comments as this is an obscure situation

[utpilla]

We never enable events until the options are set, right? If options is null we just enqueue it and wait till the ctor has set options before proceeding to enable events for the enqueued EventSources.

[mic-max]

Ya this is to avoid creating instruments for event sources we don't care about.

[utpilla]

Left some suggestions. They are not blocking this PR. We would also have to enable Public API Analyzer for this project.