-
Notifications
You must be signed in to change notification settings - Fork 464
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
Add GraphQL client integration #2368
Add GraphQL client integration #2368
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found a handful of things to check/change, but looks great altogether! 🎖️
sentry_sdk/integrations/gql.py
Outdated
event["request"] = { | ||
"data": _data_from_document(document), | ||
"api_target": "graphql", | ||
**_request_info_from_transport(self.transport), | ||
} | ||
|
||
event["contexts"] = { | ||
"response": {"data": {"errors": e.errors}, "type": "response"} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple things here.
-
This looks like an
EventProcessor
usecase. You define one and add it on the scope and then any event that is reported when that scope is active gets this extra info. Look e.g. here for inspiration, or search forEventProcessor
. -
We should also make adding the request/response dependent on whether send_default_pii is on to avoid capturing potentially sensitive data. Please also add tests for this.
-
We should make sure we're not potentially rewriting stuff on the event if there's something in
event["request"]
orevent["contexts"]
already. I'm not sure if there can be anything set at this point already, but if we move this to an event processor, then it'll be an issue since other event processors will be setting things on the event. -
We're constructing the
response
here ourselves, which means it might be different from what is actually returned by the server. Do we have a way of capturing the actual response? E.g. somehow inspecting whatreal_execute
returns? -
Same for the request, we're constructing it from the parsed query -- is there a way to get the actual payload as it'll be sent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Done
-
Done
-
I am preserving as much of the
event["request"]
andevent["context"]
as possible, but if there is anything in the specific fields that I am trying to set (e.g. if the request already has a different URL set), this will still get overwritten. Is this okay, or should I rewrite so that we never overwrite anything (which would probably necessarily mean that we lose information captured by the GQL integration)? -
Getting this information would probably be quite complicated and would probably require significantly changing the code I have written so far due to the way GQL works. This is because the actual HTTP request is made from the GQL transport, of which there are several different types (including some that use WebSockets and other non-HTTP technologies), instead of being made directly from the
Client.execute
function that we are patching here. So, we would likely need to write separate integrations for each GQL transport. And, I think what we are currently capturing should already be sufficient for most use cases, since we are displaying to users the GraphQL query and response, as well as the URL.
Also, sincereal_execute
raises the exception, there is no return value for us to be able to inspect here. We have to extract the information we can from the parameters that are passed toreal_execute
and from the error it raises. -
Same as #4 above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Overwriting single fields with more specific info is fine -- I was more worried about potentially rewriting unrelated fields. E.g. with
event["contexts"] = {
"response": {"data": {"errors": e.errors}, "type": "response"}
}
as it was previously, we'd erase any other contexts already set in event["contexts"]
. But this should not be the case anymore.
4+5. Ideally we would capture the request and response as they were sent and received by the client, but understood. As long as our reconstructed requests/responses are equivalent this is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor things, see comments.
sentry_sdk/integrations/gql.py
Outdated
event["request"] = { | ||
"data": _data_from_document(document), | ||
"api_target": "graphql", | ||
**_request_info_from_transport(self.transport), | ||
} | ||
|
||
event["contexts"] = { | ||
"response": {"data": {"errors": e.errors}, "type": "response"} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Overwriting single fields with more specific info is fine -- I was more worried about potentially rewriting unrelated fields. E.g. with
event["contexts"] = {
"response": {"data": {"errors": e.errors}, "type": "response"}
}
as it was previously, we'd erase any other contexts already set in event["contexts"]
. But this should not be the case anymore.
4+5. Ideally we would capture the request and response as they were sent and received by the client, but understood. As long as our reconstructed requests/responses are equivalent this is fine.
Co-authored-by: Ivana Kellyerova <ivana.kellyerova@sentry.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!
Please use TYPE_CHECKING
from sentry_sdk._types
, but other than that LGTM!
Co-authored-by: Ivana Kellyerova <ivana.kellyerova@sentry.io>
Closes #2337