Bad http.url
when Rails raises exceptions
#351
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While working on improvements for our exception handling support in Rails, I discovered that our
http.url
was sometimes being given bad URLs after Rails handled exceptions.At core of this issue, the Rack tracer needs a source of truth for the
http.url
value. Looking at theenv
hash, it has a few possible options which to derive this value:PATH_INFO
: Must be present on Rack requests. Starts as relative URL value, e.g./article/1?foo=bar
. It's often overwritten by Rails whenever an exception occurs, to the new path its rendering e.g./500
. At end of the trace, this value isn't very useful.REQUEST_URI
: Equal to relative URL value, e.g./article/1?foo=bar
. Appears to be provided depending on the web server, may not always be present (for example, when using IIS instead of WEBrick/Puma.)ORIGINAL_FULLPATH
: Rails only. Equal to relative URL value, e.g./article/1?foo=bar
. UnlikePATH_INFO
, it doesn't get mutated.The original logic was previously
env['REQUEST_URI'] || env['PATH_INFO']
. This logic works under the most common of cases, but provides a/500
value whenREQUEST_URI
isn't present, and an exception is raised in Rails.This pull request proposes the new value should simply be the original value of
PATH_INFO
, before the rest of the Rack stack runs. This should universally address all cases.