Skip to content
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

fix: Invalid context on GraphQL request errors #4567

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jmccance
Copy link

@jmccance jmccance commented Nov 22, 2024

📜 Description

Fixes SentryNetworkTracker to add an object for GraphQL context instead of a string.

💡 Motivation and Context

Fixes #4560.

💚 How did you test it?

📝 Checklist

You have to check all boxes before merging:

  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
    • I don't think there are any docs for this that need updating.
  • I updated the wizard if needed.
    • This change shouldn't affect project setup
  • Review from the native team if needed.
    • I don't think so, but will wait for someone to tell me.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

Ideally I'd like to include the GraphQL operation name in the client tags. This would help us identify which actual calls are causing the most errors — currently we have one issue with tens of thousands of events in it and no good way to figure out which calls are the problem.

But for now I'm just making sure we're able to see the operation correctly on network errors.

@@ -2,40 +2,40 @@
"entries": {
"brew": {
"clang-format": {
"version": "19.1.3",
"version": "19.1.4",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following instructions from check-tooling-versions.sh:

clang-format version mismatch, expected: 19.1.3, but found: 19.1.4. Please run make init to update your local dev tools. This may actually upgrade to a newer version than what is currently recorded in the lockfile; if that happens, please commit the update to the lockfile as well.

Let me know if you don't want this change in this PR, but it was preventing me from committing with the pre-commit hook in place.

Comment on lines +460 to +461
context[@"graphql"] = graphql;
graphql[@"operation_name"] = [URLSessionTaskHelper getGraphQLOperationNameFrom:sessionTask];
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if there's anything special needed to get prettier names in the Sentry dashboard, or if that's necessary. Here's how this looks in a test event I generated with our app:

image

GraphQL and Operation Name would look more aligned with the Operating System context, but my hunch is these may be getting special treatment server-side since I don't see these strings in the sentry-cocoa project anywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enableGraphQLOperationTracking attaches invalid context to network errors
1 participant