open-telemetry / opentelemetry-dotnet-contrib

This repository contains set of components extending functionality of the OpenTelemetry .NET SDK. Instrumentation libraries, exporters, and other components can find their home here.
https://opentelemetry.io
Apache License 2.0
421 stars 253 forks source link

Filter support for ASP.NET Core metrics #1765

Open vishweshbankwar opened 8 months ago

vishweshbankwar commented 8 months ago

Some pre-release versions of Metrics instrumentation provided support for filtering out measurements for http.server.request.duration. This option is removed in https://github.com/open-telemetry/opentelemetry-dotnet/pull/4981 and is not available in any of the supported .NET versions.

Please share your feedback here in case you are affected by this change. Explain your scenario in detail as to why this feature is needed.

ZRich97 commented 7 months ago

Adding comment from discussion on https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/1426:

The main use case in my mind is filtering out unused metrics (noise). Some of our supported services don't need to collect metrics for every route in the application (and they currently don't in their non-OTel monitoring solutions). This filtering could also be done at the alerting/monitoring stage (metrics we don't care about could be identified by the dimensions collected/enriched by this instrumentation and filtered out of any dashboards/alerts), but why not avoid collecting unused metrics altogether?

Metric-level filters seem like the best way to reduce possible noise and collect metrics only for the routes/requests service owners are most interested in. Is there another OTel construct or tool aside from the Instrumentation packages that would fill the same role in its absence? Is there any downside to having filter options available?

Our team is starting to move towards OTel across our .NET services. If there is a better approach we'd be interested in trying it out as well. Thanks!

cd21h commented 7 months ago

Please bring filtering back. As it was mentioned above, one use case is to reduce noise caused by healthchecks

akoken commented 6 months ago

I really don't understand the reason behind removing already implemented features. Performance or allocation? Even if so, that's a tradeoff which should be considered by the consumer. IMHO, Enrich and Filter should be de facto for all instrumentations.

We operate many microservices, each with multiple instances, within the Kubernetes environment. As you may know, it's best practice to use liveness and readiness probes for health check in K8s. However, it's worth noting that they contribute to increased metric noise.

// Should be filtered
/health/live
/health/ready
/metrics
EraYaN commented 5 months ago

We filter out a lot of health checks, version checks and a couple of internal controllers that are already kept track of in another way. Or for requests that result is very high cardinality. This reduces the total amount of noise in our metrics systems. And the additional load is negligible for us at least, our requests are quite heavy.

vchirikov commented 5 months ago

We also exclude k8s health checks or favicon.ico, robots.txt etc, it's very useful feature.

joebeernink commented 5 months ago

Adding my vote here for filtering out routes for health checks.

  1. Since metrics are created in the AppMetrics table in LogAnalytics, adding all of these unnecessary routes increases our Azure costs
  2. Filtering these calls out on the query side of metrics means a lot of manual updating of queries when calculating overall Availability of the service for customer facing queries and for calculating service latencies. Since these calls are made very frequently, a service that gets relatively few calls from the customer could show wrong customer facing stats without the filters.

Basically, lack of this feature drives up cost of operations for the system, and the consumer should be able to choose which costs are bearable and which are not.

WeihanLi commented 5 months ago

Also hoping to see the feature, while think it may be better to support from the .NET API so that the metrics we do not care would not be produced

also link to an issue in dotnet/aspnetcore https://github.com/dotnet/aspnetcore/issues/50654

bakgaard commented 4 months ago

We are looking forward to this coming back. There's a lot of extra in our exposed metrics right now (/metrics, /healthz/*)

Elter71 commented 3 months ago

Any update on this issue?

xkursatx commented 2 months ago

by adding OpenTelemetry.Instrumentation.AspNetCore and

builder.Services.AddOpenTelemetry()
    .WithMetrics(builder =>
    {
        builder
            .AddPrometheusExporter()
            .AddMeter("Microsoft.AspNetCore.Hosting", "Microsoft.AspNetCore.Server.Kestrel");

    })
    .WithTracing(builder =>
    {
        builder
            .AddAspNetCoreInstrumentation((options) => options.Filter = httpContext =>
            {
                if (httpContext.Request.Path.Value == "/metrics")
                {
                    return false;
                }
                return true;
            });
    });

i think this will resolve my problem. not tested yet but seems bright enough

vishweshbankwar commented 2 months ago

Thanks everyone for the feedback - The feature is requested for .NET9.0. Please track https://github.com/dotnet/aspnetcore/issues/50654 for updates.

akoken commented 3 weeks ago

It appears we need to wait for this fundamental feature until the .NET 9 release. Most of our customers only upgrade to LTS versions, which means we likely have to wait until .NET 10 or 11. Splendid!