Send access token from payload

[jondeandres]

It seems that the payload already have the token of the project that generated the exception. We can see that if we are using eventmachine this header is no sent, so I supose it's not really needed.

This fix issues when the project sending the payload is not the project that generated the exception. This is the case when we enqueue a job to send the report but the job is processed by a resque process in other project.

[brianr]

Thanks for the PR...

It's not currently required, but we're planning to make it required eventually (for performance reasons on our end). Instead of removing the header, I think what we need to do is pass the access token into send_payload as a separate param from the payload, and then use that value (instead of configuration.access_token) for the access token header.

[jondeandres]

Brian, I've made some changes. Tests are not passing but I think this fit your needs.

With these changes the header is always sent, also for EventMachine requests, and it's taken from the payload. I've extracted the logic to generate the JSON string in #dump_payload and it's used in #send_payload and #send_payload_using_eventmachine.

If this is ok for you I can fix the tests and complete the PR.

[jondeandres]

Well, the problem I see is that the interface of #send_payload changes, receiving a Hash instead of a String, this could be changed and then add a second argument just with the access_token.

[jondeandres]

Mmmm, it's a private method so...perhaps the interface could change?

[brianr]

Hm, could that cause any problems when deploying this change? i.e. one process expecting sending the new interface, and another process with expecting the old interface.

[brianr]

Thanks for all the work on this so far. I'll take a deeper look later today.

[jondeandres]

Brian,

you can have problems with the deploy, yes. I'm right now with the mobile.
I can explain you better later and the solution for the problem.
On Aug 28, 2014 8:13 PM, "Brian Rue" [email protected] wrote:

Thanks for all the work on this so far. I'll take a deeper look later
today.

Reply to this email directly or view it on GitHub
#128 (comment).

[jondeandres]

Hi,

really the interface of the public methods has changed with my fix, so users can have crashes in the gem if after a deploy there are still some Resque jobs (for example) queued before the deploy. The queued payloads would be a dump version of a String and not a dump version of a Hash.

The gem should not crash the users applications so it needs to be able to send payloads generated with previous versions.

The change could be something like this:

def send_payload(payload)
  log_info '[Rollbar] Sending payload'
  payload = MultiJson.load(payload) if payload.is_a?(String)
  # ...

With this change we can get again a Hash from the serialized JSON. The problem here is that you are accessing the payload using symbols: headers = { 'X-Rollbar-Access-Token' => payload[:access_token] }. So probably is better to change the #build_payload method to use strings:

def build_payload(data)
  payload = {
    'access_token' => configuration.access_token,
    'data' => data
  }

It seems they are many changes, but you should take account that the input of Rollbar methods can be a serialized version of a Hash. This is what usually happens when you use Resque, Sidekiq, etc.. so it doesn't use symbols but strings:

irb(main):002:0> hash = { foo: 'bar' }                                                                                                                 
=> {:foo=>"bar"}
irb(main):003:0> MultiJson.load(MultiJson.dump(hash))                                                                                        
=> {"foo"=>"bar"}

What could happen just after a deploy with queued payloads from a previous gem version is the next:

irb(main):046:0> payload = MultiJson.dump({access_token: 'foo'})
=> "{\"access_token\":\"foo\"}"
irb(main):047:0> # Resque encodes the data to write in Redis
irb(main):048:0* redis_value = MultiJson.dump(class: 'MyJob', args: [payload])
=> "{\"class\":\"MyJob\",\"args\":[\"{\\\"access_token\\\":\\\"foo\\\"}\"]}"
irb(main):049:0> # Resque decodes the args when pops the job
irb(main):050:0* object_in_job = MultiJson.load(redis_value)                                                                                           
=> {"class"=>"MyJob", "args"=>["{\"access_token\":\"foo\"}"]}
irb(main):051:0> args = object_in_job['args']
=> ["{\"access_token\":\"foo\"}"]
irb(main):052:0> payload = args[0]
=> "{\"access_token\":\"foo\"}"

So finally we have an string, as it happens right now, but with my changes we hope to receive a Hash, not a String. So we can use MultiJson.load as I've commented above:

irb(main):053:0> MultiJson.load(payload)
=> {"access_token"=>"foo"}

But there it's the problem, we have lost the symbols and we have strings.

If you agree with my solution I can finish the PRs and fix the tests. If not, no problem. However I'll have to do some monkey patch to make Rollbar working in the 8 projects I have in production.

[jondeandres]

Brian,

I've made the changes I told before. It's has been needed to make some changes in the specs, cause now the payload is a Hash instance until it's sent to your API. So many specs don't need to parse a String with MultiJson but it's already a Hash.

You have a smelly test doing some changes in Rollbar::MAX_PAYLOAD_SIZE :-P, and I had to do some magic there with stubs. Normally I prefer to not redefine a constant value, but it's not my project so I just followed your style.

I've tried the changes with the Rails app in spec/dummyapp and the rollbar:test rake task, the results are in your demo app:

https://rollbar.com/Rollbar/demo/items/257/occurrences/2479670635/
https://rollbar.com/Rollbar/demo/items/257/occurrences/2479658909/

I hope this is enough to fix the problem. If not just tell me and I can improve the fix.

[brianr]

@jondeandres - this is awesome, thanks for all of this.

The only concern I have with this is that there could be serialization problems when sending the original payload hash (instead of a simple string) through to resque/etc. In theory that should be fine though, hopefully that serialization code is already robust.

Are you using this in production now? If so, my inclination is to give this a few days of production use before merging into master, in case any unforseen issues crop up.

[jondeandres]

Hey!

I've been checking the Resque and Sidekiq code.

Sidekiq uses JSON to dump and parse the arguments for the jobs, so we don't have problems here:

https://github.com/mperham/sidekiq/blob/27fc88504b15ea5b57929845c7118de66c3120ea/lib/sidekiq/client.rb#L188
https://github.com/mperham/sidekiq/blob/27fc88504b15ea5b57929845c7118de66c3120ea/lib/sidekiq.rb#L112

Resque used to use MultiJson to serialize and parse the arguments of the jobs, so no problem here. However in the last versions Resque uses Marshal to generate a binary version of the arguments.

https://github.com/resque/resque/blob/master/lib/resque/queue.rb#L21
https://github.com/resque/resque/blob/master/lib/resque/queue.rb#L122

With Marshal you can .dump and .load so you get what you got:

irb(main):021:0> payload = { 'data' => { :foo => 'bar' }, 'access_token' => 'my-access-token' }
=> {"data"=>{:foo=>"bar"}, "access_token"=>"my-access-token"}
irb(main):022:0> binary_payload = Marshal.dump(payload)
=> "\x04\b{\aI\"\tdata\x06:\x06ET{\x06:\bfooI\"\bbar\x06;\x00TI\"\x11access_token\x06;\x00TI\"\x14my-access-token\x06;\x00T"
irb(main):023:0> restored_payload = Marshal.load(binary_payload)
=> {"data"=>{:foo=>"bar"}, "access_token"=>"my-access-token"}

So...any problem here. The users will get again the generated payload by the gem. Just the object returned by Rollbar.dump_payload, https://github.com/jondeandres/rollbar-gem/blob/dont-send-rollbar-header/lib/rollbar.rb#L461.

I can switch to my branch in our staging environments and tell you something, although it seems to be tested enough.

[jondeandres]

@brianr I've been trying this branch in our staging servers and it seems to be working perfectly. I've mixed projects reporting exceptions with the official gem version and projects sending the payload with this branch. I've also tried building and sending the payload with this branch.

For all cases this worked perfect.

BTW, I've been reading more code in this gem and I've seen some strange things you are doing to add the Rollbar functionality to Rails and Rack. Just things like this:

lib/rollbar/middleware/rails/show_exceptions.rb and lib/rollbar/middleware/rack/builder.rb.

You are including this modules into ActionDispatch or Rack::Builder doing some magic and changing the behaviour of the call method.

It's cool cause the users don't have to do anything else, but it's a bit tricky and can cause some problems, depending on the app.

Otherwise, this is not working fine with Sinatra, cause Sinatra rescues the exceptions and they don't arrive to the middleware stack.

I've fixed this in my own Sinatra services. I have some proposals for you so I'll do a PR soon, ok?

Cheers.

[brianr]

Sounds great. I'll merge this PR and look forward to the next one. Thanks!