jacobsvante/sentry-dramatiq

Should not use `event.request` for non-HTTP requests

Closed this issue · 8 comments

The request key is reserved for actual HTTP requests. I don't think it's a big deal to do what you're doing, but I wonder why this approach was taken?

Hi @untitaker,

The main reason is that I found it very useful to have the args/kwargs of the message available when tracking down dramatiq errors. Plus I get the ”request body” ignored for free whenever it goes above the size limit set through the request_bodies setting. Have you thought about generalizing the request concept in Sentry? I found it quite pleasing to use with this integration as well.

@jmagnusson the other integrations for celery and rq use extra to store this sort of information. You get trimming and size limitations for free in either case, although that might not have worked out too well for older sdk versions.

Another approach I would recommend exploring is to write a context:

{"contexts": {"dramatiq": {"args": ..., "kwargs": ...}}}

This might render nicer in the UI than either option. I don't really see the need to change the request interface.

@untitaker I didn't know that one could use context for other things than tags and extra, is this documented somewhere? Also where would it show up in the UI then?

@jmagnusson So the confusing thing here is that we have two "contexts":

Those two concepts are entirely unrelated.

contexts in the event schema looks like this:

Screenshot 2019-06-03 at 16 14 58

E.g. operating system looks like this in JSON:

{"contexts": {"os": {...}}}

If you write a custom key instead of os, it will still create its own section with the key as title. A better example is trace which right now has no custom handling on the serverside.

Ah that sounds perfect. I guess I could live with the message data residing a bit further down in the Sentry event page.

I should mention however that I'm very tight on time currently. Feel free to submit a PR and I'll be sure to take a look at it.

I'll try to make room for it. Thanks for your work on the integration!

I'll start working on this today.

@jmagnusson btw are you interested in joining the community slack channel for the SDK? getsentry/sentry-python#390

@untitaker Sounds like a good idea with a Slack channel, but with limited time on my hands I'm gonna have to decline for the time being.