Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enrich and Filter support for metrics [ASP.NET Core] and [HttpClient] #1733

Open
vishweshbankwar opened this issue Oct 7, 2023 · 23 comments
Labels
comp:instrumentation.aspnetcore Things related to OpenTelemetry.Instrumentation.AspNetCore comp:instrumentation.http Things related to OpenTelemetry.Instrumentation.Http documentation Improvements or additions to documentation

Comments

@vishweshbankwar
Copy link
Member

vishweshbankwar commented Oct 7, 2023

Currently, ASP.NET Core metric instrumentation has added support for Enrich and Filter (https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreMetricsInstrumentationOptions.cs). A similar functionality for HttpClient does not exist today.

These features will be removed prior to stable release of the instrumentation library. With native support for metrics added to ASP.NET Core and HttpClient in .NET8.0, any futher enhancements are expected to be added directly to these libraries. For ASP.NET Core a similar functionality is already added https://learn.microsoft.com/en-us/aspnet/core/log-mon/metrics/metrics?view=aspnetcore-8.0#enrich-the-aspnet-core-request-metric. For HttpClient, dotnet/runtime#88460 is proposed.

This issue to document the plan for these APIs with respect to stable release of these libraries.

This does not affect the Enrich and Filter APIs that are part of Tracing.

@vishweshbankwar vishweshbankwar added the enhancement New feature or request label Oct 7, 2023
@vishweshbankwar
Copy link
Member Author

@open-telemetry/dotnet-approvers @open-telemetry/dotnet-maintainers FYI

@sshumakov
Copy link

@vishweshbankwar, could you clarify what is the plan for http client metric tags enrichment in Otel targeting .NET 8? Is it something that is going to be available in the next release?

I am trying to understand if we should expect this functionality or if this is something that is not going to be available in the initial .NET8.0 and OTel release.

@cijothomas
Copy link
Member

If you are in .NET 8.0 or newer, the .NET's built-in mechanism would help with Enrich, so you won't miss anything.

If you pre .NET 8.0 world, then no luck...

@vishweshbankwar
Copy link
Member Author

could you clarify what is the plan for http client metric tags enrichment in Otel targeting .NET 8? Is it something that is going to be available in the next release?

The functionality is available today but there is no official doc for it. @JamesNK @antonfirsov - Could you please confirm the recommendation from .NET side.

using var httpclient = new HttpClient();
var message = new HttpRequestMessage();
message.RequestUri = new Uri("http://www.bing.com/");
HttpMetricsEnrichmentContext.AddCallback(message, (ctx) =>
{
ctx.AddCustomTag("MyCustomTag", "MyCustomValue");
});

@sshumakov
Copy link

sshumakov commented Oct 11, 2023

The functionality is available today but there is no official doc for it. @JamesNK @antonfirsov - Could you please confirm the recommendation from .NET side.

using var httpclient = new HttpClient();
var message = new HttpRequestMessage();
message.RequestUri = new Uri("http://www.bing.com/");
HttpMetricsEnrichmentContext.AddCallback(message, (ctx) =>
{
ctx.AddCustomTag("MyCustomTag", "MyCustomValue");
});

Trying to use this way, but callback provided is not getting called. Not sure what is missing there:

    using var httpclient = new HttpClient();
    var message = new HttpRequestMessage();
    message.RequestUri = new Uri("http://www.bing.com/");
    HttpMetricsEnrichmentContext.AddCallback(message, (ctx) =>
    {
        ctx.AddCustomTag("MyCustomTag", "MyCustomValue");
    });
    await httpclient.SendAsync(message);

@JamesNK
Copy link

JamesNK commented Oct 11, 2023

@antonfirsov Please take a look

@antonfirsov
Copy link

@sshumakov you need to make sure the http.client.request.duration instrument is enabled. Is anything collecting the System.Net.Http metrics?

@antonfirsov
Copy link

Could you please confirm the recommendation from .NET side.

@vishweshbankwar yes, the quoted code is correct for Encrichment. Not sure what is your definition of Filtering, but there is no built-in support for removing/renaming tags.

Note that HttpMetricsEnrichmentContext is a low-level primitive that needs a callback registration call for each HttpRequestMessage, and the assumption was that higher-level libraries will build their own (more convenient) enrichment experiences around it.

@sshumakov
Copy link

My assumption was that this code:

services.AddOpenTelemetry().WithMetrics(metrics => metrics.AddHttpClientInstrumentation());

Would enable instrument http.client.request.duration internally, but I can't find this code.

@vishweshbankwar - do we have examples how to use http client instrumentation with .NET 8, using native metric http.client.request.duration?

@sshumakov
Copy link

sshumakov commented Oct 12, 2023

These features will be removed prior to stable release of the instrumentation library. With native support for metrics added to ASP.NET Core and HttpClient in .NET8.0, any futher enhancements are expected to be added directly to these libraries. For ASP.NET Core a similar functionality is already added https://learn.microsoft.com/en-us/aspnet/core/log-mon/metrics/metrics?view=aspnetcore-8.0#enrich-the-aspnet-core-request-metric. For HttpClient, dotnet/runtime#88460 is proposed.

This issue to document the plan for these APIs with respect to stable release of these libraries.

This does not affect the Enrich and Filter APIs that are part of Tracing.

Could you elaborate more on the reasons of removing metrics filtering/enrichments APIs from OTel .NET SDK? It sounds like this should be internal implementation details of how OTel .NET SDK is providing metering (diagnostic listener vs .NET native metric).

@vishweshbankwar

@sshumakov
Copy link

If you are in .NET 8.0 or newer, the .NET's built-in mechanism would help with Enrich, so you won't miss anything.

If you pre .NET 8.0 world, then no luck...

I found it confusing that AspNetCore instrumentation does provide enrichments/filtering, but not HttpClient instrumentation. The same feature set is available for http in/out tracing.

There are at least two PRs which attempted to introduce this feature, but never got merged:

Do you have any plans to make this feature available?

@cijothomas

@vishweshbankwar
Copy link
Member Author

If you are in .NET 8.0 or newer, the .NET's built-in mechanism would help with Enrich, so you won't miss anything.
If you pre .NET 8.0 world, then no luck...

I found it confusing that AspNetCore instrumentation does provide enrichments/filtering, but not HttpClient instrumentation. The same feature set is available for http in/out tracing.

There are at least two PRs which attempted to introduce this feature, but never got merged:

Do you have any plans to make this feature available?

@cijothomas

No, We are not planning to include the proposed changes in instrumentation library.

@vishweshbankwar
Copy link
Member Author

My assumption was that this code:

services.AddOpenTelemetry().WithMetrics(metrics => metrics.AddHttpClientInstrumentation());

Would enable instrument http.client.request.duration internally, but I can't find this code.

@vishweshbankwar - do we have examples how to use http client instrumentation with .NET 8, using native metric http.client.request.duration?

The meters are not yet included in the instrumentation library. If you are trying to test the enrichment. You would need to explicitly call AddMeter("System.Net.Http") while configuring OTel SDK.

Once the changes proposed in open-telemetry/opentelemetry-dotnet#4931 are merged and released then you would not need to manually call AddMeter and AddHttpClientInstrumentation() will continue to work.

@joebeernink
Copy link

joebeernink commented Dec 5, 2023

I've been trying to figure out how to consistently add tags to metrics for HttpClient calls... and the removal of the enrichment and filtering from the AddHttpClientInstrumentation() method in .NET8 seems like a giant step backwards to me... but maybe I'm missing something.

Let me walk you through a real world example devs face every day:

  1. Imagine a system composed of a dozen micro-services, where each one provides an SDK to the caller.
  2. Each microservice calls out to one or more 3rd party http based services that may or may not supply an SDK.
  3. In the most simple production deployment for a cloud service, these service are deployed to 3 different production regions for redundancy
  4. There is also a dev deployment for each developer of the system, a CI deployment, and a PPE deployment
  5. When a call comes in to service A, we want to tag the inbound http request metrics with the region of the service, the environment (CI, PPE, DEV, Prod)
  6. When the call goes outbound from the service (to any http endpoint), we also want to tag the metrics with the region and environment.
  7. We also have healthz endpoints for each service, but the traffic to each one of these is really noisy, and not relevant for the overall system metrics when calculating availability for a customer, so we want to filter these out of our metrics.

With the ability to add enrichment and filtering at the Application level via OpenTelemetry, we don't have to add code to each http client call. Remember that the application likely has no control over the creation of the http client or message as that is wrapped in an SDK that may be 3rd party.

So without this ability, how do I add tags and filter metrics when I don't control the creation of the http client / message? To me, this ability was the open-telemetry/opentelemetry-dotnet#1 reason to convert our apps to OpenTelemetry in the first place (a close second was consistency in the traceIds). Without it... it's not really doing anything valuable anymore.

What am I missing?

Below is the code as it currently stands, where resourceAttributes is a collection of KVP's of the tags I want added to the metrics (i.e. [{ "env":"Prod"}, {"region": "eus"}]. Are the httpclient metrics going to automatically pick up the resources from the resource builder now? Doesn't seem like that is happening (yet?)

            .WithMetrics(builder =>
            {
                builder
                    .SetResourceBuilder(ResourceBuilder.CreateDefault()
                                        .AddAttributes(resourceAttributes))
                    .AddMeter(willowContext?.MeterOptions.Name ?? "Unknown", willowContext?.MeterOptions.Version ?? "Unknown")
                    .AddAspNetCoreInstrumentation()
                    .AddHttpClientInstrumentation()
                    .AddAzureMonitorMetricExporter(o =>
                    {
                        o.ConnectionString = appInsightsConnection;
                    });
            });

@cijothomas
Copy link
Member

Are the httpclient metrics going to automatically pick up the resources from the resource builder now? Doesn't seem like that is happening (yet?)

It is the expected behavior (and it was the expected behavior always)! All metrics/traces/logs will get the Resource attached to it automatically.

I guess the issue you are hitting is likely AzureMonitor specific - it does not fully support Resources. Azure/azure-sdk-for-net#35487 or a new issue in that repo would be best to get further help on this!

we want to tag the inbound http request metrics with the region of the service, the environment (CI, PPE, DEV, Prod)

This should be modelled as Resource, and not via HttpClient/AspnetCore enrichments! I can see that you are already trying out Resource, but wanted to re-iterate that.

@joebeernink
Copy link

Thanks @cijothomas.

I am looking at what it would take to implement Azure Managed Prometheus... but we are running our containers on an Azure Container Environment, and I don't see any documentation or capabilities in the portal that describe how to make sure export to Prometheus is enabled for an ACE.

Before I go down the road of digging into figuring out how to deploy Prometheus to an ACE, do you know if the Prometheus Exporter has the same issue with Tags as the Azure Monitor Exporter does?

@joebeernink
Copy link

Also, if filter support is taken away in the AddAspNetCoreInstrumentation() call for logs and metrics, how would we then prevent really noisy calls from being logged? Is there another approach already available with .NET8. I've been trying the filter approach, and it hasn't worked anyway, so if there is a better approach, please let me know.

@cijothomas
Copy link
Member

Prometheus Exporter has the same issue with Tags as the Azure Monitor Exporter does?

Yes. PrometheusExporter does not support Resource today. Its just a implementation limitation in this repo. (Other languages have added support for Resource mapping in Prom Exporter, just not yet occurred in this repo)

@cijothomas
Copy link
Member

Also, if filter support is taken away in the AddAspNetCoreInstrumentation() call for logs and metrics, how would we then prevent really noisy calls from being logged? Is there another approach already available with .NET8. I've been trying the filter approach, and it hasn't worked anyway, so if there is a better approach, please let me know.

There was no specific support for Logs, as Logs can leverage ILogger's built-in filtering.
Only support that got removed was for metrics.

@joebeernink
Copy link

Sorry, I meant Traces, not logs (I use the terms interchangeably because I am am old).

We currently have this code in our configuration for the traces...
.AddAspNetCoreInstrumentation(o =>
{
o.Filter = (httpContext) =>
{
return !httpContext.Request.Path.ToString().EndsWith("healthz");
};
}
But we still see the following in the traces table in AppInsights. I don't know how I could filter that out using ILogger filters...
image

@cijothomas
Copy link
Member

That screenshot look like a Log only. i.e the one emitted using ILogger, so it wont be affected by the AspNetCoreInstrumentation filtering! There should have been a category name from ILogger, that would have helped understanding who is producing it and how to filter it, but its not shown in the screenshot. If you can use console exporter and repro it, we can help here. Otherwise, its best to report the issue in AzureMonitor repo, as it'l be AzureMonitor specific issue.

@reyang reyang transferred this issue from open-telemetry/opentelemetry-dotnet May 13, 2024
@reyang reyang added comp:instrumentation.aspnetcore Things related to OpenTelemetry.Instrumentation.AspNetCore comp:instrumentation.http Things related to OpenTelemetry.Instrumentation.Http labels May 13, 2024
@ajryan
Copy link

ajryan commented Nov 8, 2024

Is there a more recent discussion of HttpClient instrumentation meter enrichment? This discussion doesn't have a conclusion. @cijothomas

@cijothomas
Copy link
Member

Is there a more recent discussion of HttpClient instrumentation meter enrichment? This discussion doesn't have a conclusion. @cijothomas

The functionality is provided natively by asp.net core and httpclient libraries, but not well documented. Here's the PR attempting to document it:
#2059

@TimothyMothra TimothyMothra added documentation Improvements or additions to documentation and removed enhancement New feature or request labels Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:instrumentation.aspnetcore Things related to OpenTelemetry.Instrumentation.AspNetCore comp:instrumentation.http Things related to OpenTelemetry.Instrumentation.Http documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

9 participants