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

The names of the metrics produced by the new meters do not seem clear #48309

Closed
ghost opened this issue May 18, 2023 · 6 comments · Fixed by #48375
Closed

The names of the metrics produced by the new meters do not seem clear #48309

ghost opened this issue May 18, 2023 · 6 comments · Fixed by #48375
Assignees
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares feature-diagnostics Diagnostic middleware and pages (except EF diagnostics)

Comments

@ghost
Copy link

ghost commented May 18, 2023

Summary

#47536 introduced metrics for ASP.NET Core through the new metrics API and this new feature is really cool!

However, I'm not sure to understand the choice for the name of the metrics and they don't seem to follow the OpenTelemetry specification. Let's take for example the metric current-requests recorded by the meter Microsoft.AspNetCore.Hosting. Following the OTEL specification, the name should instead be http.server.active_requests. The name current-requests is not clear enough. When I browse the metrics available in my Prometheus or via dotnet-counters, I don't know if the current-requests metric corresponds to the number of active incoming (server) or outgoing (client) requests, whereas http.server.active_requests is immediately clearer.

I may be wrong, can anyone help me?

cc @JamesNK

@ghost ghost added the design-proposal This issue represents a design proposal for a different issue, linked in the description label May 18, 2023
@amcasey amcasey added feature-diagnostics Diagnostic middleware and pages (except EF diagnostics) and removed design-proposal This issue represents a design proposal for a different issue, linked in the description labels May 18, 2023
@JamesNK
Copy link
Member

JamesNK commented May 23, 2023

The standard way to export metrics counters to Prometheus is to use OpenTelemetry. However, the OpenTelemetry standard for exporting to Prometheus creates a problem: the counter name in Prometheus is just the metrics counter name, and the Meter is added as a label to the counter value.

The relevant part of the OTel spec: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/compatibility/prometheus_and_openmetrics.md#instrumentation-scope. I think this design was chosen so OTel could round-trip data without data loss. If they did something different, like combining the meter and counter names in Prometheus, then when values came back to OTel, OTel wouldn't know where to split them up.

Problem:
This decision means that if ASP.NET Core has a "current-requests" counter for incoming HTTP requests, and HttpClient has a "current-requests" counter for outgoing HTTP requests, those values are mixed in Prometheus. A filter must be used with the meter name to filter to the desired .NET counter values.

The existing event counter names in .NET assume the event source name and counter name are combined - https://learn.microsoft.com/en-us/dotnet/core/diagnostics/available-counters. An example of a counter that isn't unique is "current-requests". Both Microsoft.AspNetCore.Hosting and System.Net.Http have it. Unfortunately, combining the meter and counter name to create a unique identifier doesn't hold in the cloud-native tooling world of metrics.

What someone sees in Prometheus when searching for metrics:
image

What does current_requests mean? Current requests of what? Or what does connection duration mean? Is it duration for HTTP connections or SQL connections? etc

In the cloud-native world, OpenTelemetry is the library for gathering metrics, and Prometheus is the metrics database. I think we need to cater to their way of doing things.

Solution:
We'll need to provide unique names for new metrics counters. For example, the System.Net.Http meter will have a "http-client-current-requests" counter and Microsoft.AspNetCore.Hosting will have a "http-server-current-requests" counter. This isn't a huge change or sacrifice. And it makes counter names by themselves more descriptive.

@JamesNK
Copy link
Member

JamesNK commented May 23, 2023

However, I'm not sure to understand the choice for the name of the metrics and they don't seem to follow the OpenTelemetry specification.

For a bunch of reasons:

  • OTel semantic conventions specs aren't finalized
  • .NET has extra counters that aren't in the spec
  • .NET counters include tags that aren't in the spec

We'd prefer .NET to consistently follow its own standard instead be 90% similar to OTel's. Being almost the same as another spec but still incompatible has stung us in the past.

@ghost
Copy link
Author

ghost commented May 23, 2023

We'd prefer .NET to consistently follow its own standard instead be 90% similar to OTel's. Being almost the same as another spec but still incompatible has stung us in the past.

And that's a shame. Microsoft should be a driving force in standardizing things rather than creating a new standard/convention within a project (especially for a new feature). If only 10% of the metrics/attributes are missing in the OTel specification, why not contribute and propose to add them?

@davidfowl
Copy link
Member

Thoughts @reyang?

And that's a shame. Microsoft should be a driving force in standardizing things rather than creating a new standard/convention within a project (especially for a new feature). If only 10% of the metrics/attributes are missing in the OTel specification, why not contribute and propose to add them?

We need room for both. There will always be things that are implementation specific that need to be counted and we aren't not going to add them because they aren't in the otel spec. It's like how chromium adds support for new APIs with a different prefix until it becomes standard, then the API becomes the standard.

@reyang
Copy link

reyang commented May 23, 2023

Thoughts @reyang?

And that's a shame. Microsoft should be a driving force in standardizing things rather than creating a new standard/convention within a project (especially for a new feature). If only 10% of the metrics/attributes are missing in the OTel specification, why not contribute and propose to add them?

We need room for both. There will always be things that are implementation specific that need to be counted and we aren't not going to add them because they aren't in the otel spec. It's like how chromium adds support for new APIs with a different prefix until it becomes standard, then the API becomes the standard.

I'm an active member of the OpenTelemetry Technical Committee and I can provide some context and also share my thinking:

  1. OTel does have the eventual goal of improving the overall ecosystem by encouraging more software components to be instrumented by default, rather than relying on 3rd party instrumentation libraries - this will be a multi-years journey and requires lots of work from the API, semantic conventions and education.
  2. .NET runtime and ASP.NET Core have been very supportive of the OTel effort since the beginning of the project, with the shared goal of improving telemetry story for all developers and embracing standards. Actually .NET is the first runtime to incorporate OTel API directly in the low-level runtime libraries (so that other runtime libraries and frameworks can take dependency).
  3. In .NET 8, I think the .NET/ASP.NET team decided to step further by adding instruments, which is a high risk and high reward effort. Hence, the team has to keep a reasonable control of risks - 1) tight schedule 2) external dependencies 3) moving targets 4) limited engineering resources.
  4. Speaking of moving targets, the OTel HTTP semantic convention has gone through several changes and is not yet stable. Actually, Microsoft has been actively contributing to the schema standardization. @lmolkova @reyang @trask have been actively contributing to the standardization https://github.com/orgs/open-telemetry/projects/41/views/1 and alignment with other existing systems https://opentelemetry.io/blog/2023/ecs-otel-semconv-convergence/. I don't feel OpenTelemetry is ready to have any language runtime taking direct dependency as of today, and I don't feel .NET/ASP.NET is ready to directly use the Experimental version of the semantic conventions in the upcoming .NET8 release.
  5. Through the collaboration, many folks do share the findings and suggestions to the OTel specification and semantic conventions. IIRC @JamesNK provided great feedback to the OTel community when he noticed anything while implementing it in ASP.NET Core.
  6. In the long run, .NET might still have an intermediate layer as @davidfowl mentioned in his reply "we need room for both".
  7. OpenTelemetry .NET project does provide several instrumentation libraries, and these libraries will be 100% aligned with the OTel semantic conventions. Eventually, I expect these libraries to become thinner as more logic can be removed by leveraging what's being implemented by .NET runtime.

@danmoseley
Copy link
Member

danmoseley commented May 25, 2023

cc @geeknoid as we need to align metrics names in https://github.com/dotnet/extensions too

@amcasey amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares and removed area-runtime labels Jun 2, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares feature-diagnostics Diagnostic middleware and pages (except EF diagnostics)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants
@davidfowl @JamesNK @danmoseley @amcasey @reyang and others