feat(browser): Add graphqlClientIntegration
#13783
Conversation
Added support for graphql query with `xhr` with tests. Signed-off-by: Kaung Zin Hein <[email protected]>
| @@ -357,6 +357,8 @@ export function xhrCallback( | |||
| return undefined; | |||
| } | |||
|
|
|||
| const requestBody = JSON.parse(sentryXhrData.body as string); | |||
There was a problem hiding this comment.
This is dangerous, any such operations we have to try-catch as bodies may not be JSON!
| @@ -374,6 +376,7 @@ export function xhrCallback( | |||
| 'server.address': host, | |||
| [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.browser', | |||
| [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'http.client', | |||
| body: requestBody, | |||
There was a problem hiding this comment.
h: We should not do this - this will attach the request bodies to all spans, always, which is a) large, b) potentially PII sensitive 😬
Instead of doing this in on('spanStart'), I think we'll need a hook that provides the request or body in some way. I am thinking of a new hook like:
client.on('outgoingRequestSpanStart', (span: Span, { body }: { body: unknown }) => {
// ...
});and emit this hook in this file after the span was started 🤔
There was a problem hiding this comment.
Noted. I implemented that hook and attached the req payload only to graphql spans.
Added test for fetch graphql. Created new utility functions and added tests. Updated `instrumentFetch` to collect fetch request payload. Signed-off-by: Kaung Zin Hein <[email protected]>
| const handlerData: HandlerDataFetch = { | ||
| args, | ||
| fetchData: { | ||
| method, | ||
| url, | ||
| body, |
There was a problem hiding this comment.
IMO collecting fetch req payload shouldn't be a problem as long as only the graphql requests are sampled. Because xhr instrumentation already collects the payload.
Signed-off-by: Kaung Zin Hein <[email protected]>
|
…r graphql requests Signed-off-by: Kaung Zin Hein <[email protected]>
| const requestBody = JSON.parse(payload); | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access | ||
| const isGraphQLRequest = !!requestBody['query']; |
There was a problem hiding this comment.
This may not be the most accurate and safest way to detect a graphql request?
| client.on('spanStart', span => { | ||
| client.emit('outgoingRequestSpanStart', span); | ||
| }); |
There was a problem hiding this comment.
This was maybe a bit confusingly explained by me - I would not do this, so I'd remove these lines here. I'll leave a comment further below where/how I would suggest to do this!
| client.on('spanStart', span => { | |
| client.emit('outgoingRequestSpanStart', span); | |
| }); |
| @@ -374,6 +377,7 @@ export function xhrCallback( | |||
| 'server.address': host, | |||
| [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.browser', | |||
| [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'http.client', | |||
| body: graphqlRequest, | |||
There was a problem hiding this comment.
Instead of adding this here, after the span was started, in this file do:
client.emit('outgoingRequestSpanStart', span, { body });So the hook receives the body as second argument, and then in the integration we can use the body and add it to the span or use it otherwise. This way, this is not added to all spans, as we do not want to generally capture this for every span.
There was a problem hiding this comment.
I get what you mean now! Will implement
… has started Signed-off-by: Kaung Zin Hein <[email protected]>
Resolves #13399
Todo:
Supportfetchspans and testsyarn lint) & (yarn test).