Skip params when unavailable #39
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The circle ci build error will be gone once #38 is merged |
eanakashima
approved these changes
Oct 9, 2019
There was a problem hiding this comment.
Looks good, but 😑 for the fact that this happens — looking at request params feels like a pretty standard thing to do in Rails. Maybe worth adding to the comment that twirp is the only reason we think we'd be likely to get into this state, just for future travelers.
|
yeah, lgtm, although without more comments inline I'd have to git blame and grovel around in history to find this conversation. |
|
Added additional information to the comment, I'll wait until #38 has been merged though so that I can test that it can push to ruby gems correctly when I release this |
martin308
force-pushed
the
martin308.skip-params
branch
from
184b81b to
11718f5
Compare
ajvondrak
added a commit
to ajvondrak/beeline-ruby
that referenced
this pull request
Feb 22, 2020
* Remove extraneous spans that were being generated due to the
ActiveSupport::Notifications subscriptions. There's no need to
actually configure these from the tests, since we already have the
active_support_spec.rb, and it's not like the config in the
rails_spec.rb even matched the one from the Rails generator or
anything. These just get in the way of testing the Rails integration,
since the custom fields we'd want to make assertions about only wind
up on the root span anyway.
* Factor out assertions into a set of shared examples, and actually
start making assertions about the `request.{action,controller,route}`
fields that the integration adds (which weren't even being tested for
previously 😱).
* Rework the "bad request" tests that reproduce honeycombio#49. These tests will
now break against the old implementation, since they assert that
routing information is still present (whereas honeycombio#50 wipes the fields out
with error messages). There are ways to get the routing information
without ever trying to parse the GET/POST parameters, so we can still
save this use case. Furthermore, the only Rails 5+ specific changes
are to whether or not Rails responds with an HTTP 400, so we needn't
wipe out the entire `describe` block for Rails < 5.
* Add tests to reproduce the issue described in honeycombio#62, where unrecognized
routes may cause the old implementation to erroneously fall back to
the GET/POST parameters for routing information.
* Add tests that reproduce honeycombio#31. The bug is actually in the twirp gem,
whose middleware fails to rewind the `rack.input` after reading it.
I've verified these regression tests against the old version of the
Rails integration that didn't check `if request.raw_post`, and they
fail appropriately. I wanted to have this test in here to have
confidence in changing the implementation of the Rails integration.
The `raw_post` workaround of honeycombio#39 is unnecessary if we actually never
look at the GET/POST parameters in the first place (no parsing, no
problem).
ajvondrak
added a commit
to ajvondrak/beeline-ruby
that referenced
this pull request
Feb 22, 2020
This refactoring avoids ever parsing GET/POST parameters to obtain the Rails routing information required for the `request.action`, `request.controller`, and `request.route` fields. This is accomplished in two ways: 1. Use `ActionDispatch::Request#path_parameters` instead of `ActionDispatch::Request#params`. The former is the subset of data we need for `request.action` + `request.controller`, and in fact are the values merged into the latter hash after `#params` parses the normal GET/POST parameters. So we can avoid the extraneous parsing by just getting the path parameters straight from the horse's mouth. 2. Call `ActionDispatch::Journey::Router#recognize` on a simplified request that *only* contains the necessary routing information. The `#recognize` method will yield the `ActionDispatch::Request#params` when it matches a route, which again triggers the GET/POST parsing. But we don't care about the HTTP parameters. So instead I copied the approach of `ActionDispatch::Routing::RouteSet#recognize_path`, which reconstructs a simplified request with just the information that it needs. (Unfortunately we can't use `#recognize_path` directly because instead of returning the route it returns the path parameters, which we can already get directly from `#path_parameters`.) This accomplishes several things: * Fixes honeycombio#62, since we don't get a chance to erroneously fall back to GET/POST parameters in the event that the routing information is missing (due to requests to unrecognized routes). * Fixes honeycombio#31, since the bug ultimately comes from the twirp gem causing POST parameter parsing to fail. But if we never parse the HTTP parameters, no problem. This is a more direct workaround than honeycombio#39, but ultimately is a problem that should be fixed upstream. * Fixes honeycombio#49, since the parameter encoding won't matter if we never try to parse them. Renders honeycombio#50 unnecessary, since we won't have a chance to trigger the exceptions in question. * Gives us more data in more situations. The new implementation of the `request.route` field now comes up non-nil in the event of errors because we use the `ActionDispatch::Request#original_fullpath` and not the `#path_info` that gets [rewritten by `ActionDispatch::ShowExceptions`](https://github.com/rails/rails/blob/758e4f8406e680a6cbf21b170749202c537a2576/actionpack/lib/action_dispatch/middleware/show_exceptions.rb#L49). * Gives us more data in more Rails versions. With the addition of actual tests for the field values, it became clear that `request.route` was missing in Rails 4, simply because of an interface change introduced in Rails 5.
This was referenced Feb 22, 2020
martin308
pushed a commit
that referenced
this pull request
Mar 9, 2020
This refactoring avoids ever parsing GET/POST parameters to obtain the Rails routing information required for the `request.action`, `request.controller`, and `request.route` fields. This is accomplished in two ways: 1. Use `ActionDispatch::Request#path_parameters` instead of `ActionDispatch::Request#params`. The former is the subset of data we need for `request.action` + `request.controller`, and in fact are the values merged into the latter hash after `#params` parses the normal GET/POST parameters. So we can avoid the extraneous parsing by just getting the path parameters straight from the horse's mouth. 2. Call `ActionDispatch::Journey::Router#recognize` on a simplified request that *only* contains the necessary routing information. The `#recognize` method will yield the `ActionDispatch::Request#params` when it matches a route, which again triggers the GET/POST parsing. But we don't care about the HTTP parameters. So instead I copied the approach of `ActionDispatch::Routing::RouteSet#recognize_path`, which reconstructs a simplified request with just the information that it needs. (Unfortunately we can't use `#recognize_path` directly because instead of returning the route it returns the path parameters, which we can already get directly from `#path_parameters`.) This accomplishes several things: * Fixes #62, since we don't get a chance to erroneously fall back to GET/POST parameters in the event that the routing information is missing (due to requests to unrecognized routes). * Fixes #31, since the bug ultimately comes from the twirp gem causing POST parameter parsing to fail. But if we never parse the HTTP parameters, no problem. This is a more direct workaround than #39, but ultimately is a problem that should be fixed upstream. * Fixes #49, since the parameter encoding won't matter if we never try to parse them. Renders #50 unnecessary, since we won't have a chance to trigger the exceptions in question. * Gives us more data in more situations. The new implementation of the `request.route` field now comes up non-nil in the event of errors because we use the `ActionDispatch::Request#original_fullpath` and not the `#path_info` that gets [rewritten by `ActionDispatch::ShowExceptions`](https://github.com/rails/rails/blob/758e4f8406e680a6cbf21b170749202c537a2576/actionpack/lib/action_dispatch/middleware/show_exceptions.rb#L49). * Gives us more data in more Rails versions. With the addition of actual tests for the field values, it became clear that `request.route` was missing in Rails 4, simply because of an interface change introduced in Rails 5. --- Revamp the Rails integration's tests * Remove extraneous spans that were being generated due to the ActiveSupport::Notifications subscriptions. There's no need to actually configure these from the tests, since we already have the active_support_spec.rb, and it's not like the config in the rails_spec.rb even matched the one from the Rails generator or anything. These just get in the way of testing the Rails integration, since the custom fields we'd want to make assertions about only wind up on the root span anyway. * Factor out assertions into a set of shared examples, and actually start making assertions about the `request.{action,controller,route}` fields that the integration adds (which weren't even being tested for previously 😱). * Rework the "bad request" tests that reproduce #49. These tests will now break against the old implementation, since they assert that routing information is still present (whereas #50 wipes the fields out with error messages). There are ways to get the routing information without ever trying to parse the GET/POST parameters, so we can still save this use case. Furthermore, the only Rails 5+ specific changes are to whether or not Rails responds with an HTTP 400, so we needn't wipe out the entire `describe` block for Rails < 5. * Add tests to reproduce the issue described in #62, where unrecognized routes may cause the old implementation to erroneously fall back to the GET/POST parameters for routing information. * Add tests that reproduce #31. The bug is actually in the twirp gem, whose middleware fails to rewind the `rack.input` after reading it. I've verified these regression tests against the old version of the Rails integration that didn't check `if request.raw_post`, and they fail appropriately. I wanted to have this test in here to have confidence in changing the implementation of the Rails integration. The `raw_post` workaround of #39 is unnecessary if we actually never look at the GET/POST parameters in the first place (no parsing, no problem).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #31.
Tracing this through, we call this method and
action_dispatch.request.parametersis nil. So we fall into this method which eventually ends up calling this lambda which raises an error whenraw_postis nil.I'm not entirely clear what twirp is doing to the request to end up in this state and have struggled to reproduce this in a test. This should prevent the issue though and doesn't seem to cause any other side effects that I could find.