dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.59k stars 10.06k forks source link

Metadata to exclude request from http.server.request.duration metric #50654

Closed JamesNK closed 4 months ago

JamesNK commented 1 year ago

Is there an existing issue for this?

Is your feature request related to a problem? Please describe the problem.

Every request that comes into ASP.NET Core is recorded to the http.server.request.duration metric. There are some situations where its desirable to opt out of that behavior:

Excluding system endpoints that are regularly requested is useful because they distort results.

Describe the solution you'd like

Add metadata that can be used to skip recording the request. Then, an app can configure endpoints with the metadata to exclude them from recording the HTTP metric.

It could be an SuppressHttpMetric() extension method:

app.MapPrometheusScrapingEndpoint().SuppressHttpMetric();

Or attribute:

[HttpGet]
[SuppressHttpMetric]
public IActionResult GetHealth()
{
     // ...
}

Some libraries might automatically include the metadata, e.g. OTel's PrometheusScapingEndpoint internally adds the metadata when it creates the endpoint.

Additional context

No response

Ne4to commented 10 months ago

Is there any workaround? I tried to find filter options in HostingMetrics with no success. MapPrometheusScrapingEndpoint allows to pass custom MeterProvider but it has no methods to override and internally MeterProvider is converted to internal MeterProviderSdk in MeterProviderExtensions

JamesNK commented 10 months ago

Can you provide why you want to disable the metric? It is useful for us to understand why and when a new feature would be used.

Ne4to commented 10 months ago

My top three requests now are /metrics for Prometheus and two endpoints for Kubernetes liveness and readiness probes (based on ASP.NET HealthChecks). When I open these dashboards https://grafana.com/orgs/dotnetteam the first thing I see is not helpful. Some summary graph is 99% misleading for a service on a test environment.

UPD: https://github.com/dotnet/aspnetcore/issues/53322 created ~BTW: official dashboards use different units than published by ASP.NET 8 and http_server_request_duration_seconds_bucket should be changed to http_server_request_duration_s_bucket in the dashboard to start it working (s instead of seconds)~

image image

WeihanLi commented 10 months ago

I have similar issues with @Ne4to , some endpoints should be excluded like the Prometheus scraping endpoint and probe endpoints. This should be a high priority I think.

And I tried with the ShortCircuit() extension introduced in .NET 8, which would not work for this case, should the metrics be excluded when ShortCircuit() is specified?

JamesNK commented 10 months ago

ShortCircuit() skips middleware, but the counter is recorded at the hosting layer. It won't be skipped.

JamesNK commented 10 months ago

To come up with the right improvement for this situation I'm going to reach out to OpenTelemetry team for their advice.

One approach might be to skip recording system endpoints. Another approach would be to still record a value but add a tag to indicate that it's a system endpoint. Then people can choose to use the tag to filter results out.

leandrobattochio commented 10 months ago

Just another thumbs up here. I ended up changing asp net core default grafana dashboard to not include endpoints matching "/metrics" from the results. (not the best approach. I would like to exclude the endpoint entirely, which in my searchs lead me here) But then I also noticed some requests from my SignalR are also being recorded and it is messing with my graphs. I think we should be able to choose to exclude some endpoint from metrics. I don't wan't to record calls to "/metrics" endpoint.

I also wouldn't want SignalR handshakes too. I mean, I expect some connections to be opened for a long time. What I'm interested in is if any of my business API is taking too long to execute.

Have any of you experienced something similiar when it comes to metrics and signalr hubs ?

vchirikov commented 10 months ago

Same here, it will be nice to have more control on metrics, looks like in net8.0 we can only enrich tags from middleware with IHttpMetricsTagsFeature, but not to filter them as it was in net6.0 with otel (AddAspNetCoreInstrumentation had an Filter option).

@JamesNK do you have any news about this?

JamesNK commented 10 months ago

Not in the last 6 days.

.NET 9 won't come out until November so there is lots of time to improve this scenario.

If you're looking for an immediate solution then you could add an "ignore" tag to requests you want to filter out, and then write queries that ignore requests with that tag.

houseofcat commented 8 months ago

@JamesNK Appreciate all your hard work, but the tag-only filtering idea is not really enough for our use case (unless fully integrated by the OT SDK). There should be at least an option to just configure routes to ignore completely internally to an application. For some of us, there is a cost associated when using a managed 3rd Party for Open Telemetry like Signoz.

Not all of us want to upload a billion metrics so that we have to then have to also modify all of our dashboards (keeping it modified for every flavor of system tag we need to exclude and filter on) while also paying for ingress of the data we just hid and will never use.

We all don't have Microsoft's big pennies :)

I actually think the issue is more on the Open Telemetry's side of the SDK once this has been enabled in NET9. I imagine having something programmatically filtering this deep in Kestrel would be not only be clunky to implement but also performance regressing. Perhaps there is a means for the OpenTelemetry SDK to ignore internally from a system tag after invocation. Minor and negligible churn inside the application is very sufficient for us, but would require their team to implement it after this is possibly implemented here. Anywho, just wanted to mention egress cost as another motivator to handle internally.

xkursatx commented 7 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 on production

vishweshbankwar commented 7 months ago

Linking a similar issue from OTel side: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/issues/1765

cc: @noahfalk @samsp-msft

cijothomas commented 7 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 on production

@xkursatx This is filtering traces. This issue is about filtering metrics, so this code will not help.

dario-griffo-paytently commented 6 months ago

We are also looking forward into this implementation, we have time until we go live we would love to see this on NET9

JamesNK commented 6 months ago

The idea proposed in the original issue text - https://github.com/dotnet/aspnetcore/issues/50654#issue-1892351387 - is to use endpoint metadata to opt out of collecting metrics.

Endpoint metadata is great when you want behavior to be static for an endpoint, for example, the health check API or Prometheus scraping API always never reports a value to http.server.request.duration.

// Yes http.server.request.duration values reported
app.MapGet("/api/person", (int id) => _repo.GetPerson(id));

// No http.server.request.duration values reported
app.MapPrometheusScrapingEndpoint().DisableHttpMetrics();
app.MapHealthChecks().DisableHttpMetrics();
app.MapGet("/live", () => "true").DisableHttpMetrics();

That covers what is being discussed in this issue. However, does anyone have a scenario that requires more dynamic behavior is required? For example, a HTTP request decides to not report metrics while it is running? That could be inside the endpoint, or in middleware:


app.Use(async (context, next) =>
{
    if (context.Request.Path.StartsWith("/admin"))
    {
        context.Features.Get<IHttpMetricsDisableFeature>()?.DisableMetrics();
    }

    await next();
});