Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In rollbar.report_message's extra_data kwarg, "body" being reserved leads to surprising behavior #309

Open
brettdh opened this issue Mar 14, 2019 · 2 comments
Assignees

Comments

@brettdh
Copy link

brettdh commented Mar 14, 2019

Per this comment on rollbar.report_message:

extra_data: dictionary of params to include with the message. 'body' is reserved.
the body key in the extra_data kwarg is "reserved". I discovered the hard way that, if you happen to pass a dict with a body key, the message you intended to report is replaced with that raw JSON. This was quite difficult to track down, as the behavior did not communicate the "reserved"ness of body - nor is it mentioned in the documentation; only in the docstring.

I'd like to request that this reserved key be changed to something that's harder to accidentally collide with - or at least for pyrollbar to raise an exception or print a warning or something letting me know what I did.

@brettdh brettdh changed the title "body" being reserved leads to surprising behavior In rollbar.report_message's extra_data kwarg, "body" being reserved leads to surprising behavior Mar 14, 2019
@brianr
Copy link
Member

brianr commented Mar 14, 2019

Thanks @brettdh; that does sound like quite the rabbit hole...

@rokob thoughts? I think this behavior comes from here:

data['body']['message'].update(extra_data)

A few ideas:

  1. We could change the API format so that the reserved 'body' field has a different name, i.e. '$body', or we could move it to a different part of the payload
  2. We could have extra_data go in a nested dict, i.e. data['body']['message']['extra'] instead of merging it into data['body']['message']
  3. We could rename the extra_data['body'] key so it doesn't override the reserved body

After typing this out, (2) seems like the best way to go. The downside of that is that it would make the key a bit longer when it appears in the rollbar UI, but that seems like an OK compromise to avoid the nasty in-band signaling problem we have right now.

@rokob
Copy link
Contributor

rokob commented Mar 14, 2019

I think 2 sounds reasonable. In other SDKs we don't merge extra data into data.body.message but into data.custom basically at the top level

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants