Rack Integration Raises an Error During File Upload #62
Comments
|
You should use |
|
While the surface cause of the error is Honeycomb invoking
beeline-ruby/lib/honeycomb/integrations/rails.rb Lines 20 to 21 in 0285e18 What I'm guessing is that you have a form field somewhere named Does this sound plausible? Because otherwise I can't figure out how your parameter is getting into the beeline's span in the first place. A cursory search of the dev.to source shows this partial, which looks like it might be generating the parameter named based on dynamic data (?): <%= f.file_field field.attribute %>But that's just me taking a shot in the dark. The term "action" seems to crop up in your source code a lot, too; I don't know if there are any associated file uploads with that parameter name. As for a solution, the beeline could technically handle unexpected types better. Something simple like diff --git a/lib/honeycomb/integrations/rack.rb b/lib/honeycomb/integrations/rack.rb
index f3808d4..19b5188 100644
--- a/lib/honeycomb/integrations/rack.rb
+++ b/lib/honeycomb/integrations/rack.rb
@@ -28,9 +28,7 @@ module Honeycomb
hny = env["HTTP_X_HONEYCOMB_TRACE"]
client.start_span(name: "http_request", serialized_trace: hny) do |span|
add_field = lambda do |key, value|
- next unless value && !value.empty?
-
- span.add_field(key, value)
+ span.add_field(key, value) if valuable?(value)
end
extract_fields(env, RACK_FIELDS, &add_field)
@@ -58,6 +56,14 @@ module Honeycomb
end
end
+ def valuable?(value)
+ if value.respond_to?(:empty?)
+ !value.empty?
+ else
+ !value.nil?
+ end
+ end
+
# Rack middleware
class Middleware
include RackBut if this is the error you're getting, I'm afraid you'd just get nonsensical strings like Alternatively, I wonder if there's a way to get at Rails routing information without having to look through the fraught request parameters. 🤔 If so, the beeline should almost certainly prefer to use that method (if my theory is right that |
|
Good catch! That information about how you parse the controller actions is SUPER HELPFUL! I went back and looked at the Honeybadger involving the error and sure enough, the action param is getting overridden. I am going to dig further into this and figure out if we can prevent that overwritten action from happening. I do think preventing the hard errors from rising up and better handling the unexpected types would probably be a good idea. Then people aren't being thrown 500s but rather seeing the issue in unexpected data rather than unexpected errors. At least that is how I would prefer it. It might also be useful to log a warning or something if you run into an action that is unexpected to help people debug. |
|
The plot thickens... 🤔 I think a way we could fix this would be to use However, trying to write a failing unit test to the effect, I can't actually reproduce the issue! 😱 Just trying something simple like this passes no problem: diff --git a/spec/honeycomb/integrations/rails_spec.rb b/spec/honeycomb/integrations/rails_spec.rb
index 9a0ffc0..52a3913 100644
--- a/spec/honeycomb/integrations/rails_spec.rb
+++ b/spec/honeycomb/integrations/rails_spec.rb
@@ -37,6 +37,7 @@ if defined?(Honeycomb::Rails)
app.routes.draw do
get "/hello/:name" => "test#hello"
+ post "/goodbye" => "test#goodbye"
end
end
end
@@ -45,6 +46,10 @@ if defined?(Honeycomb::Rails)
def hello
render plain: "Hello World!"
end
+
+ def goodbye
+ render plain: "Goodbye World!"
+ end
end
describe "a standard request" do
@@ -68,6 +73,15 @@ if defined?(Honeycomb::Rails)
it_behaves_like "event data"
end
+ describe "a request that clobbers routing parameters" do
+ before { post "/goodbye", action: "wave" }
+
+ it "still sets the request.action field correctly" do
+ event = libhoney_client.events.last
+ expect(event.data["request.action"]).to eq "goodbye" # not "wave"
+ end
+ end
+
if VERSION >= Gem::Version.new("5")
describe "a bad request" do
before doIt seems that params = begin
request_parameters.merge(query_parameters)
rescue EOFError
query_parameters.dup
end
params.merge!(path_parameters)The dev.to code is complicated enough that I'm not really sure how to track down who's overriding the I'm not sure if this would happen any way other than a rogue middleware, because the beeline is even constructing a brand-spanking-new I look forward to you tracking down how this is happening, since it doesn't seem like Rails should be letting |
|
After some investigation, I am pretty sure this request is not originating from our Rails app. From looking at the Honeybadger, there is no referrer for the request, the url the user is making a POST to is Given that conclusion, I don't think there is really anything here to fix. My solution at this point is going to be to mark the Honeybadger as ignored. We can definitely close this issue since I really don't want to waste any more of your time. Once again, I am so sorry I didn't see those details sooner! |
|
Eureka! That was the clue we needed! 🎉 Rails can only set the
Absolutely not a waste of time. I'm so happy to finally figure out the problem, and it took your sleuthing to narrow everything down. Thanks so much for satisfying my sense of order, lol. Writing a PR to fix this issue now. ❤️ P.S.: I think this shift away from get/post parameter parsing will be a vast improvement over the workarounds of #39 & #50, too. 😍 |
* 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).
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 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).
I recently noticed that we have had a few errors raised in our production environment from the beeline gem when a user tries to upload a file. They seem pretty deep in the rack implementation so I figured I would share here to see if you all had any suggestions for a fix.
Here is the error and backtrace:
Our Honeycomb setup is very basic and we just use the default suggested tracking for Rails at this point.
Thanks in advance for the help and if there is anything else you need from me let me know!
The text was updated successfully, but these errors were encountered: