-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(browser): Add support for GraphQL persisted operations #18505
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
feat(browser): Add support for GraphQL persisted operations #18505
Conversation
The isPersistedRequest type guard now validates that sha256Hash and version properties exist in the persistedQuery object, preventing undefined values from being set as span attributes and breadcrumb data. Added tests for edge cases: - Empty persistedQuery object - Missing sha256Hash property - Missing version property 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
| span.setAttribute('graphql.persistedQuery.sha256Hash', graphqlBody.extensions.persistedQuery.sha256Hash); | ||
| span.setAttribute('graphql.persistedQuery.version', graphqlBody.extensions.persistedQuery.version); |
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.
h: OpenTelemetry semantic convention are snake_case, I think we should go with graphql.persisted_query.hash.sha256 and graphql.persisted_query.version to be more in line with semconvs.
| data['graphql.persistedQuery.sha256Hash'] = graphqlBody.extensions.persistedQuery.sha256Hash; | ||
| data['graphql.persistedQuery.version'] = graphqlBody.extensions.persistedQuery.version; |
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.
h: Same here, let's get snake_case going and prefix the hash
andreiborza
left a comment
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.
This looks good to me, I'll take over making the suggested changes and fixing the tests.
Thanks for the contribution!
|
Awesome, thanks a lot for the collaboration and velocity! You're quick haha. This helps a ton, so thanks for looking into this. |
e319da1 to
e5e38fa
Compare
andreiborza
left a comment
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.
Thanks again for the contribution!
Closes: #18499 --------- Co-authored-by: tbeeren <tbeeren@bol.com> Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Co-authored-by: Andrei Borza <andrei.borza@sentry.io>
Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
yarn lint) & (yarn test).Closes: #18499