✨ [RUM-12133] Track GraphQl Response #3921
Conversation
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage 🔗 Commit SHA: 84bfaf0 | Docs | Was this helpful? Give us feedback! |
| res.header('Content-Type', 'application/json') | ||
| res.json({ data: { result: 'success' } }) | ||
|
|
||
| const scenario = req.query.scenario as string | undefined |
packages/rum-core/src/domain/configuration/configuration.spec.ts
Outdated
Show resolved
Hide resolved
| function waitForResponseToComplete( | ||
| context: RumFetchResolveContext, | ||
| configuration: RumConfiguration, | ||
| callback: (duration: Duration, graphqlErrorsCount?: number, graphqlErrors?: GraphQlError[]) => void | ||
| ) { | ||
| const clonedResponse = context.response && tryToClone(context.response) | ||
| if (!clonedResponse || !clonedResponse.body) { | ||
| // do not try to wait for the response if the clone failed, fetch error or null body | ||
| callback(elapsed(context.startClocks.timeStamp, timeStampNow())) | ||
| } else { | ||
| readBytesFromStream( | ||
| clonedResponse.body, | ||
| () => { | ||
| callback(elapsed(context.startClocks.timeStamp, timeStampNow())) | ||
| }, | ||
| { | ||
| bytesLimit: Number.POSITIVE_INFINITY, | ||
| collectStreamBody: false, | ||
| } | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| const graphQlConfig = findGraphQlConfiguration(context.url, configuration) | ||
| const shouldExtractGraphQlErrors = graphQlConfig?.trackResponseErrors | ||
|
|
||
| readBytesFromStream( |
There was a problem hiding this comment.
💬 suggestion: that indeed seems to be the right spot to add the graphql response error logic, however now this function responsibility is not solely to wait for the response to complete, to compute a more accurate duration.
So it could be interesting to rename this function accordingly and see if the different responsibilities of this function could better be reflected in the implementation.
There was a problem hiding this comment.
Agree I renamed but I am not sure of the name, let me know what you think. Related to the other comment, graphql properties are now part of the context
| const metadata = extractGraphQlRequestMetadata(request.body, graphQlConfig.trackPayload) | ||
| if (!metadata) { | ||
| return | ||
| } |
There was a problem hiding this comment.
❓ question: Does it mean that we can't track error response if we don't track the payload?
There was a problem hiding this comment.
Yes we can, payload is just undefined in this case but the metadata object is still here with operationType, operationName and variables 👍
| const graphqlError: GraphQlError = { | ||
| message: typeof errorObj.message === 'string' ? errorObj.message : 'Unknown GraphQL error', | ||
| } |
There was a problem hiding this comment.
💬 suggestion: If there is no message, why not just leave this empty?
There was a problem hiding this comment.
it’s more about a malformed error type, since we’re expecting a string in the response format. So I instead of not reporting I thought it would be clearer to inform with an unknown error message
| return | ||
| } | ||
|
|
||
| const errors = (response.errors as unknown[]).map((error: unknown) => { |
There was a problem hiding this comment.
💬 suggestion: Do we need to go through all those validation? Can't we just have const errors = response.errors?
There was a problem hiding this comment.
well, don’t we need to have a validation for the expected type ? Furthermore the extensions code need to be set at top level
Motivation
This PR extends the existing GraphQL tracking to capture GraphQL response errors, allowing developers to monitor server-side errors in their GraphQL queries
See spec for more informations : Graphql Spec
Changes
Test instructions
Checklist