Add Strawberry GraphQL integration#2393
Conversation
|
Hey @patrick91, here's a shot at porting the 🍓 Sentry tracing extension to the Sentry SDK and adding error monitoring. Any feedback is welcome! Some notes:
|
|
we spoke on discord about this, I'm very excited to see this landing! From our side we'll add some code to make it easier to hook into the process result code (so users can still override it) I'll also make sure to have an integration test on our side once this lands, so we don't break the integration in future |
|
Made the PR here: strawberry-graphql/strawberry#3127 @sentrivana let me know if that looks ok to you! |
|
Thanks @patrick91 for being awesome and helping us integrate with Strawberry better! This PR now utilizes a new hook released with Strawberry 0.209.5. |
szokeasaurusrex
left a comment
There was a problem hiding this comment.
I have suggested a few changes. But, generally looks good!
| elif event.get("request", {}).get("data"): | ||
| del event["request"]["data"] |
There was a problem hiding this comment.
Maybe we could wrap it in a try/except block like so? Your choice, I think either way is good
| elif event.get("request", {}).get("data"): | |
| del event["request"]["data"] | |
| else: | |
| try: | |
| del event["request"]["data"] | |
| except KeyError: | |
| pass |
There was a problem hiding this comment.
I know that PEP8 prefers what you suggested here but from a personal taste point of view I prefer the original because I find it more concise and there is less visual clutter. So if you're fine with it I'd keep the original.
There was a problem hiding this comment.
Made one slight change to it though: 7831520 So that, if for some reason event["request"] is there and is None, we don't get an AttributeError.
There was a problem hiding this comment.
I have thought about this a bit more, and I think I would really prefer to switch to the try/except syntax, since I think it is much more readable for people looking at this code for the first time.
With the try/except block, the code reads in plain English as "If we want to send PII, send it, otherwise ensure we don't send any request data by deleting it if it's there." It is immediately obvious what the code does.
With the elif statement, the code reads in plain English as "If we want to send PII, send it, otherwise check whether the event has a request (treat the request as an empty dictionary if it doesn't) and check whether the event's request has any data, and if it does, delete the event request data." In this situation, the code's objective is less obvious; only after thoroughly reading the contents of the elif condition and the statement it contains is it clear that the code here is designed to ensure the request has no data when we send the event.
Although the elif version is more concise, this concision comes at the expense of readability and understandability, so I think the try/except version would be preferable here.
There was a problem hiding this comment.
Ok, changed: 2ccd1eb
(The TypeError is there for the event["request"] == None case -- it'd throw that instead of the AttributeError I mentioned above.)
Co-authored-by: Daniel Szoke <szokeasaurusrex@users.noreply.github.com>
Co-authored-by: Daniel Szoke <szokeasaurusrex@users.noreply.github.com>
Co-authored-by: Daniel Szoke <szokeasaurusrex@users.noreply.github.com>
|
Hope I've addressed everything @szokeasaurusrex -- please have another look when you have time. |
This reverts commit aed4cc9. Will do this in a new PR.
szokeasaurusrex
left a comment
There was a problem hiding this comment.
Added one comment to your tests, also please address my reply to our try/except thread. Then, the code looks good to me! @antonpirker should review before merge to double-check that I didn't miss anything
Capture GraphQL errors and spans when using Strawberry server side.
This is a port of the existing Sentry tracing extension in Strawberry with added error monitoring.
The integration has an option called
async_executionwhich controls whether to hook into Strawberry sync or async. If not provided, we try to guess based on whether an async web framework is installed.Example event in Sentry: https://sentry-sdks.sentry.io/issues/4507792283/events/c517f630fef54a378affa9602ccf3ff8/
Strawberry is one of three integrations for #2257, split from #2381.