Add support for some AspNetCoreMetricsInstrumentationOptions

[Temppus]

Changes

Hi. I wanted to include standard http metrics via AddOpenTelemetryMetrics and it works great. But I am missing option to add additional tags to current metrics recorded. In my case I wanted to enrich metrics with tag of current tenant.
I saw there was no support for it yet.

Notes:

• new option class specific for metrics AspNetCoreMetricsInstrumentationOptions was introduced
• added possibility for filtering/enrichment

Tests:

• Added test for both filtering and enrichment

I would like to here your feedback, thanks.

TODO:

• Appropriate CHANGELOG.md updated for non-trivial changes
• Design discussion issue #
• Changes in public API reviewed

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: Temppus / name: Ivan Beňovic (c0e505f)

[CodeBlanch]

@Temppus Please add a note in the project CHANGELOG about the change.

[Temppus]

@CodeBlanch Changelog updated

[CodeBlanch]

LGTM. We should probably update README with some details but that can be done as a follow-up. The current content is at least close 😄

@Temppus Just FYI I'm going to let this sit for a while to see if any other maintainers comment. Lots of people out on vacation.

[alanwest]

I’m becoming suspicious of the Filter and maybe also Enrich callbacks…

For traces I think Filter provides a poor approximation of a sampler. For example, say there's an endpoint that makes a call using HttpClient. If Filter is used to omit spans to this endpoint, often the user would expect the downstream HttpClient span to also be omitted. However, depending on the sampler in use, Filter cannot prevent the HttpClient span from being exported. Makes it seem like a custom sampler would be preferable.

That said, Filter seems more appealing for metrics given metrics have no notion of a sampler. Though, I wonder if using Sdk.SuppressInstrumentation would provide a better solution for users wanting to filter certain metrics. Sdk.SuppressInstrumentation could also have the benefit of suppressing the downstream HttpClient metric. @dnelson-relativity shared a middleware solution using Sdk.SuppressInstrumentation.

I'm a little less suspicious of the value of Enrich, however, I suspect there are alternate solutions for many of the use cases for which Enrich is used.

In my case I wanted to enrich metrics with tag of current tenant.

There have been discussions in the spec to support this kind of scenario with either the View API and baggage (see #2619) or by introducing the concept of a measurement processor (much like a span processor but is invoked when a measurement is recorded).

I'll put this PR on tomorrow's SIG meeting agenda for discussion (11 am Pacific).

[Temppus]

I'll put this PR on tomorrow's SIG meeting agenda for discussion (11 am Pacific).

Hi, what was the outcome of it ?

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: Temppus / name: Ivan Beňovic (c0e505f, cf486ec, c9da7f8, aba7fc4, 3359f06, be5e043, cae48b8)

[alanwest]

Hi, what was the outcome of it ?

Apologies for delayed update. For now, I believe the consensus is that we are ok moving forward with Filter/Enrich functionality for metric instrumentation. Namely to maintain parity with traces. It is admittedly confusing to have it for one and not the other.

Given no instrumentation has yet received a stable release, we will continue to evaluate the use cases Filter and Enrich serve. At a minimum, if Fitler/Enrich are kept when we release stable, we intend to better document their limitations over other solutions, so that end users can better understand if Filter/Enrich will meet their needs.

[Temppus]

Can you rerun the CI again ? Thanks.

[Temppus]

Can you rerun the CI again ? Thanks.
one more time pls :)

[Temppus]

Can you rerun the CI again ? Thanks.

Hopefully last time ...

[Temppus]

Is TestRecoveryAfterFailedExport flaky ? It seems totally unrelated to my changes.

[utpilla]

Is TestRecoveryAfterFailedExport flaky ? It seems totally unrelated to my changes.

Yes, the Code Coverage check is flaky.

[Temppus]

Yes, the Code Coverage check is flaky.

So there is a chance that when CI will be rerun it it will pass ?

[Kielek]

Yes, the Code Coverage check is flaky.

So there is a chance that when CI will be rerun it it will pass ?

Don't bother about Code coverage action if there is not issue related to your code. It is not mandatory to pass before merge (as long as it is not stable). Other steps are passing.

[Temppus]

@CodeBlanch is there anything that should be changed to merge this ?

[codecov]

Codecov Report

Merging #3948 (7b0bc0d) into main (44d2fe2) will decrease coverage by 0.12%.
The diff coverage is 83.78%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3948      +/-   ##
==========================================
- Coverage   85.30%   85.17%   -0.13%     
==========================================
  Files         287      288       +1     
  Lines       11026    11059      +33     
==========================================
+ Hits         9406     9420      +14     
- Misses       1620     1639      +19     

Impacted Files	Coverage Δ
...AspNetCore/Implementation/HttpInMetricsListener.cs	74.35% <57.14%> (-9.65%)	⬇️
...ry.Instrumentation.AspNetCore/AspNetCoreMetrics.cs	100.00% <100.00%> (ø)
...NetCore/AspNetCoreMetricsInstrumentationOptions.cs	100.00% <100.00%> (ø)
...ation.AspNetCore/MeterProviderBuilderExtensions.cs	100.00% <100.00%> (ø)
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs	76.47% <0.00%> (-5.89%)	⬇️
...lemetry/Internal/SelfDiagnosticsConfigRefresher.cs	86.53% <0.00%> (-5.77%)	⬇️
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs	80.15% <0.00%> (-2.39%)	⬇️
src/OpenTelemetry/BatchExportProcessor.cs	82.24% <0.00%> (-1.87%)	⬇️