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

[release/8.0] Fix duplicate error.type tags in metrics #55301

Merged
merged 1 commit into from May 2, 2024

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Apr 22, 2024

Fix duplicate error.type tags in metrics

Description

ASP.NET Core has a prominent metric - http.request.server.duration - that reports information about HTTP request duration along with important metadata. One piece of metadata is whether an error occurred during the request. This is the error.type tag.

The error.type tag can be reported from multiple places, and it's possible to get duplicate tag values in rare situations. Duplicate metrics tags can break some telemetry tooling.

error.type can be added by exception middleware and it can be added in hosting if there is an unhandled exception. That means there can be duplicates if:

  • Exception handling middleware handles an exception (tag added in middleware) and then there is an unhandled exception afterwards (tag added in hosting)
  • Two configured exception handling middleware instances. An error is thrown and is handled (tag added in middleware 1) and then afterwards another error is thrown and handled by the second middleware (tag added in middleware 2).

Fix by conditionally adding error.type to tag collection only if it isn't present, aka first value wins.

Fixes #55159

Customer Impact

Reported by customer at #55159. Prometheus (popular OSS telemetry database) can't handle duplicates and fails to load new metrics.

There is also open-telemetry/opentelemetry-dotnet#5199 which appears to be from the same issue and has 6 +1 votes.

A workaround is to not use the ASP.NET Core error middleware, but it is a very popular feature, and it's not obvious that it is the fix.

Regression?

  • Yes
  • No

Risk

  • High
  • Medium
  • Low

The fix is simple: don't add error.type tag to metrics collection if it is already present.

Verification

  • Manual (required)
  • Automated

Before:
image

After:
image

Packaging changes reviewed?

  • Yes
  • No
  • N/A

@JamesNK JamesNK added Servicing-consider Shiproom approval is required for the issue area-hosting Includes Hosting labels Apr 22, 2024
@JamesNK JamesNK requested a review from amcasey April 22, 2024 23:35
@dotnet-policy-service dotnet-policy-service bot added this to the 8.0.x milestone Apr 22, 2024
@JamesNK JamesNK changed the title Fix duplicate error.type tags in metrics [release/8.0] Fix duplicate error.type tags in metrics Apr 22, 2024
@JamesNK JamesNK added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Apr 23, 2024
@JamesNK
Copy link
Member Author

JamesNK commented Apr 23, 2024

Email approval

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Apr 30, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label May 2, 2024
@wtgodbe wtgodbe merged commit 1cb85b6 into release/8.0 May 2, 2024
23 of 25 checks passed
@wtgodbe wtgodbe deleted the jamesnk/metrics-exceptiontags-8.0 branch May 2, 2024 16:20
@dotnet-policy-service dotnet-policy-service bot modified the milestones: 8.0.x, 8.0.6 May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-hosting Includes Hosting Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants