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

Fix metrics duration and http.route tag with exception handling #52652

Merged
merged 8 commits into from
Dec 13, 2023

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Dec 8, 2023

Fixes #52648

The http.server.request.duration counter records information about every HTTP request. One piece of recorded information is the http.route which is the matched route pattern. The route pattern is taken from HttpContext's Endpoint.

There is a bug that causes http.route to not be available when:

  • Incoming request to an app that has enabled metrics
  • There is an unhandled exception
  • App is using UseExceptionHandler
  • Response hasn't started or aborted
  • The exception handler clears the HttpContext and re-executes the middleware pipeline <- problem
  • Value is recorded to http.server.request.duration without http.route

The exception handler clears the endpoint from the context and re-executes the middleware pipeline. There is no longer an endpoint on the context when http.server.request.duration is recorded and the http.route value is lost.

The fix is to stash the original endpoint to a known location (a stable key in HttpContext.Items) and then falls back to getting the endpoint from that location if the endpoint isn't set. HttpContext.Items is used because IExceptionHandlerPathFeature (which has an Endpoint property with the original endpoint) isn't available at the hosting layer.

A magic string in a dictionary isn't ideal, but we do use this technique in some other places. It would be better to add a new feature - IOriginalEndpointFeature perhaps? - but it doesn't seem worth it for this scenario.

@JamesNK JamesNK added the area-hosting Includes Hosting label Dec 8, 2023
src/Shared/HttpExtensions.cs Outdated Show resolved Hide resolved
@JamesNK JamesNK force-pushed the jamesnk/metrics-exceptionhandling-route branch from 1bd3bd2 to a4bf039 Compare December 12, 2023 06:55
Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

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

LGTM -- dealing with issues that came as a result of the middleware pipeline being reinvoked is always a pain but I agree that stashing OriginalEndpoint in HttpContext.Items is probably the most sensible approach here.

src/Shared/HttpExtensions.cs Outdated Show resolved Hide resolved
@JamesNK JamesNK force-pushed the jamesnk/metrics-exceptionhandling-route branch from 4c28788 to ecd98eb Compare December 13, 2023 06:35
@JamesNK JamesNK enabled auto-merge (squash) December 13, 2023 08:11
@JamesNK JamesNK merged commit 6600fa8 into main Dec 13, 2023
26 checks passed
@JamesNK JamesNK deleted the jamesnk/metrics-exceptionhandling-route branch December 13, 2023 08:21
@ghost ghost added this to the 9.0-preview1 milestone Dec 13, 2023
@JamesNK
Copy link
Member Author

JamesNK commented Dec 13, 2023

/backport to release/8.0

Copy link
Contributor

Started backporting to release/8.0: https://github.com/dotnet/aspnetcore/actions/runs/7192610416

Copy link
Contributor

@JamesNK backporting to release/8.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Fix metrics duration and http.route tag with exception handling
Using index info to reconstruct a base tree...
M	src/Middleware/Diagnostics/test/UnitTests/ExceptionHandlerMiddlewareTest.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Middleware/Diagnostics/test/UnitTests/ExceptionHandlerMiddlewareTest.cs
Applying: Clean up
Using index info to reconstruct a base tree...
M	src/Middleware/Diagnostics/test/UnitTests/ExceptionHandlerMiddlewareTest.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Middleware/Diagnostics/test/UnitTests/ExceptionHandlerMiddlewareTest.cs
CONFLICT (content): Merge conflict in src/Middleware/Diagnostics/test/UnitTests/ExceptionHandlerMiddlewareTest.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 Clean up
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

Copy link
Contributor

@JamesNK an error occurred while backporting to release/8.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

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.

Investigate metrics endpoint behavior when exception middleware runs
4 participants