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

Cache request values when hosting metrics are enabled #55351

Merged
merged 2 commits into from
Apr 26, 2024

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Apr 25, 2024

Fixes #55280

Hosting metrics use data on HttpContext when writing tag values. For example, the HTTP method of a request is metadata when writing metrics.

However, the context is mutable, which means that someone can modify the values used by hosting metrics during the request pipeline. This has the worst impact on the active requests counter, which depends on tags being the same for the start and the end of the request. If tags don't match then you end up with two dimensions, one going up and the other going down:

http.server.active_requests{ http_request_method="GET", url_scheme="http"} = 100
http.server.active_requests{ http_request_method="GET", url_scheme="https"} = -100

Fix is to stash the initial values on the context (this PR puts them on the metrics tags feature which is on the context) when the request starts and then use the stashed values at the end of the request.

There is slightly more memory used per request when metrics are enabled (3 additional fields), and no impact when metrics are disabled.

Copy link
Member

@mgravell mgravell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@JamesNK JamesNK enabled auto-merge (squash) April 25, 2024 23:43
@JamesNK JamesNK merged commit c157c5c into main Apr 26, 2024
26 checks passed
@JamesNK JamesNK deleted the jamesnk/metrics-preservevalues branch April 26, 2024 01:36
@dotnet-policy-service dotnet-policy-service bot added this to the 9.0-preview5 milestone Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-hosting Includes Hosting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

http.server.active_requests metric incorrect when behind TLS-terminating load balancer
3 participants