Closed mikehale closed 7 years ago
Nice find @mikehale :)
bump. anything blocking this from being merged?
@mikehale sorry for the delay. Is there any reason why we simply don't check for a .is_a(Hash)
? Is the Rollbar gem expecting rack.input
to be present? Maybe the better fix is to change the signature of ErrorReporters.notify
so it takes in nil
instead. That way we can skip this extraction in a saner way.
Is the Rollbar gem expecting
rack.input
to be present?
Yes, extract_request_data_from_rack
expects rack.input
to be an IO.
I tested reporting an exception with a nil
rack_env
, but the rollbar gem does not handle that either:
irb(main):016:0> report_exception_to_rollbar(nil, RuntimeError.new)
D, [2017-01-04T11:47:57.596340 #42833] DEBUG -- : [Rollbar] Reporting exception: RuntimeError
I, [2017-01-04T11:47:57.596896 #42833] INFO -- : [Rollbar] Scheduling item
I, [2017-01-04T11:47:57.596926 #42833] INFO -- : [Rollbar] Sending item
I, [2017-01-04T11:47:58.073781 #42833] INFO -- : [Rollbar] Success
I, [2017-01-04T11:47:58.073930 #42833] INFO -- : [Rollbar] Details: https://rollbar.com/instance/uuid?uuid=b068d0f6-fe22-4ab9-9454-cd16b08eaf49 (only available if report was successful)
W, [2017-01-04T11:47:58.075205 #42833] WARN -- : [Rollbar] Exception while reporting exception to Rollbar: undefined method `[]=' for nil:NilClass
=> true
irb(main):017:0> report_exception_to_rollbar({}, RuntimeError.new)
D, [2017-01-04T11:48:12.218743 #42833] DEBUG -- : [Rollbar] Reporting exception: RuntimeError
I, [2017-01-04T11:48:12.219101 #42833] INFO -- : [Rollbar] Scheduling item
I, [2017-01-04T11:48:12.219126 #42833] INFO -- : [Rollbar] Sending item
I, [2017-01-04T11:48:12.682914 #42833] INFO -- : [Rollbar] Success
I, [2017-01-04T11:48:12.682982 #42833] INFO -- : [Rollbar] Details: https://rollbar.com/instance/uuid?uuid=e6bcc9a0-c688-4075-87b7-b55839d5e296 (only available if report was successful)
D, [2017-01-04T11:48:12.683011 #42833] DEBUG -- : [Rollbar] Exception uuid saved in env: e6bcc9a0-c688-4075-87b7-b55839d5e296
I could change the check to rack_env.empty?
which would be more generic, not tied to the rollbar interface as much, but still should work in the case where a rack_env
is not actually populated, which I think was the intent of #291.
LGTM 👍
👍
thanks!
@mikehale thank you! 🙂
A rack env is a simple hash and as such will never respond to the method
body
. It does however have a key ofrack.input
. Maybe there is some confusion around what is being passed intonotify
...env
vsRack::Request.new(env)
?/cc @appleton ref https://github.com/interagent/pliny/pull/291