Allow to filter requests when collecting ASP.NET Core metrics.

[nahk-ivanov]

This change updates ASP.NET Core instrumentation to respect the filter, specified in the AspNetCoreInstrumentationOptions.Filter.

When using the OpenTelemetry library we already could filter out ASP.NET Core request traces, but not metrics. This forced us to completely disable standard metrics instrumentation, because health checks were polluting the measurements and we could not exclude them.

This change updates the MeterProviderBuilderExtensions for ASP.NET Core, to accept the AspNetCoreInstrumentationOptions configuration delegate and the HttpInMetricsListener to respect the filter option.

Code is very similar to the traces instrumentation with one exception - instead of creating the listener in the extension method and passing it to the instrumentation class, I decided to keep listener initialization in the instrumentation class (AspNetCodeMetrics), because it already encapsulated .NET diagnostics Meter, which is required by the metrics listener. I didn't want to push that code out of the instrumentation class (and deal with new disposable in the outer scope), so I just updated the instrumentation class to take in options in the constructor and pass them through to the listener.

Lastly, I updated the events emitted by the instrumentation itself to, namely RequestIsFilteredOut and RequestFilterException, to include the name of the handler that was running the filter. This will allow us to distinguish filter delegates specified for metrics vs traces.

It is worth mentioning that I did not update the ASP.NET (non-Core), as it is not an immediate need for us at this point. There are also few more TODOs left to respect other options. We can work on them separately later, if needed.

Fixes #3324.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: nahk-ivanov / name: Alexander Ivanov (4af6c66)

[cijothomas]

@nahk-ivanov Please sign the EasyCLA so we can accept contributions.

[cijothomas]

because health checks were polluting the measurements and we could not exclude them.

Could you open an issue describing the problem? I am not sure if we want to run Filters in the Metric path, so if you describe the issue, we can see if there are alternates.

[nahk-ivanov]

@cijothomas Created #3324.

If you are using health check APIs (which you should be), then by default these requests will be counted as well by the standard instrumentation that this library provides. I don't know how many people are interested in the number of health check requests (we are not), so the filter will allow to eliminate them. I described an alternative (filter downstream) in the issue, but I'd rather prefer to cut it as soon as possible, rather than consuming resources generating, serializing and sending unnecessary data.

What is the concern with this code path? Performance? This should not be more than an additional null check, if you don't use the feature, so I'd assume not a big deal. If you use the feature, then you are committing to the perf impact of your filter delegate code.

[nahk-ivanov]

I didn't bump the event versions. It doesn't seem like we care about ETW consumers that cache manifests.

[nahk-ivanov]

This is where the discrepancy is between traces and metrics - in traces we will new-up HttpInLIstener here and pass it to the instrumentation (AspNetCoreMetrics in this case). Here it is a little bit more complicated, because metrics listener also expects the Meter instance, which is disposable. Currently AspNetCoreMetrics will maintain meter's lifecycle, and I didn't want to push it out to here, which will be hard to do, given that this is a static POP by itself. So I just pass in options to the instrumentation class, and it passes them to the listener. Should not be a big deal, just one thing to note.

[github-actions]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[nahk-ivanov]

Ping?

Don't make me fork it. 😄

[cijothomas]

@cijothomas Created #3324.

If you are using health check APIs (which you should be), then by default these requests will be counted as well by the standard instrumentation that this library provides. I don't know how many people are interested in the number of health check requests (we are not), so the filter will allow to eliminate them. I described an alternative (filter downstream) in the issue, but I'd rather prefer to cut it as soon as possible, rather than consuming resources generating, serializing and sending unnecessary data.

What is the concern with this code path? Performance? This should not be more than an additional null check, if you don't use the feature, so I'd assume not a big deal. If you use the feature, then you are committing to the perf impact of your filter delegate code.

In General, we need to try to be as close as feasible to the OpenTelemetry specification. The Enrich and Filter in some of these instrumentation libraries are our own creations, and not following a OTel general spec. For tracing, the only way spec-ed out is to use SpanProcessor (activityProcessor in .NET). (Enrich/Filter provided easy access to context..)
For metrics, there is no equivalent concept "MetricProcessor".

I am not convinced that it should be extended to Metrics as well.. Maybe the right way here would be to have a OTel semantic convention, to mark "health" requests with special attribute (like "synthetic_request" : "true/false"), as opposed to exposing new API surface, for multiple instrumentation libraries.

[nahk-ivanov]

I think we need to separate the spec and an application that is being instrumented for this conversation. This change only applies to the application (ASP.NET) that is being instrumented with OpenTelemetry. For instance, if I was to instrument my own code, I could decide which metrics and in what situations I want to emit. ASP.NET is not instrumented for metrics to begin with and this library is essentially instrumenting the ASP.NET application to emit metrics with standard schema. We would not need this for any other implementation of the OpenTelemetry, as ASP.NET is, well, for .NET and not "for multiple instrumentation libraries". Following your interpretation of the OpenTelemetry concept, I should not be allowed to expose configuration for my application as to which metrics it should or should not emit in a particular environment, which is obviously not true as I can instrument my application any way I like. If there is not configuration exposed as to how ASP.NET application should be instrumented in this instrumentation library, then other people can write their own ASP.NET instrumentation libraries (fork this one) and emit the same metric, but filtered based on the filter, inclusion list, or whatever they see fit. I'm not sure how that is a concern of OpenTelemetry specification.

I'm not sure why OpenTelemetry would want to step in to the territory of prescribing what exact metrics data application should be emitting (even in terms of common schema like http - what if I use gRPC with RSocket for my service?), but unless every single library now adopts OpenTelemetry (which is obviously never going to happen) - we will not be able to properly use semantic conventions. In this particular case health check endpoints are implemented by another library, which will need to be updated to mark the request activities with new request flag, which only makes sense for OpenTelemetry. I don't think that should ever happen, because (as usual) OpenTelemetry will be superseded by the next cool library 5 years down the road and we don't want to take dependency on a certain instrumentation framework in the common library. Alternatively, I could inject a middleware to check the request path and update the request activity with additional properties, which still results in me writing more code than what should be needed for a task as simple as this. With that we would still need to have a check and a flag in this library to collect/discard metrics for such requests + update the OpenTelemetry spec as this is now a concern of the specification. While this may appear as being a "more generic" solution to some, I don't see why we would go all the way around to achieve something as simple as filter few requests from metrics collection from my application, when nothing in OpenTelemetry spec dictates how configuration for telemetry collection in my application should look like.

As to the SpanProcessor argument - I don't think Filter/Enrich that are defined in the specific ASP.NET instrumentation library map to the concept. SpanProcessor should be something that is available to me despite what and how is being instrumented, outside of anything ASP.NET. This change is literally for a specific implementation of the ASP.NET instrumentation that is only necessary to mate ASP.NET traces with common OpenTelemetry metrics schema, and aside from the schema itself, OpenTelemetry does not and should not dictate which methods are called during the metrics collection from the ASP.NET. This is just a feature of the specific instrumentation library for a specific application, it doesn't need to be implemented anywhere else. There might be a web server implementation that integrates health checks as a "first-class citizen", and they don't even go through the same middleware and don't ever hit instrumentation library for that server to begin with.

[github-actions]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[github-actions]

Closed as inactive. Feel free to reopen if this PR is still being worked on.