Skip to content

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
@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.

@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
@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

@github-actions
Copy link
Contributor

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

@github-actions
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!

@github-actions
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

5 participants