-
-
Notifications
You must be signed in to change notification settings - Fork 435
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
Support for automatically capturing Failed GraphQL (Apollo 3) Client errors #2781
Conversation
@lbloder can you give some feedback on this PR since I changed a few things you added? |
sentry-apollo-3/src/main/java/io/sentry/apollo3/SentryApollo3HttpInterceptor.kt
Outdated
Show resolved
Hide resolved
sentry-apollo-3/src/main/java/io/sentry/apollo3/SentryApollo3Interceptor.kt
Show resolved
Hide resolved
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
44d22ce | 314.00 ms | 357.00 ms | 43.00 ms |
e3e3297 | 301.14 ms | 354.78 ms | 53.63 ms |
72c927d | 279.31 ms | 317.98 ms | 38.67 ms |
6b513d9 | 311.94 ms | 326.29 ms | 14.35 ms |
2483a35 | 293.88 ms | 331.54 ms | 37.66 ms |
641268f | 288.40 ms | 354.54 ms | 66.14 ms |
4d9c9a7 | 260.75 ms | 355.40 ms | 94.65 ms |
eac6dd1 | 305.90 ms | 378.14 ms | 72.24 ms |
efb1927 | 292.00 ms | 348.86 ms | 56.86 ms |
741ec32 | 248.92 ms | 348.82 ms | 99.90 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
44d22ce | 1.72 MiB | 2.28 MiB | 571.72 KiB |
e3e3297 | 1.72 MiB | 2.28 MiB | 571.65 KiB |
72c927d | 1.72 MiB | 2.28 MiB | 571.72 KiB |
6b513d9 | 1.72 MiB | 2.28 MiB | 571.72 KiB |
2483a35 | 1.72 MiB | 2.28 MiB | 571.72 KiB |
641268f | 1.72 MiB | 2.28 MiB | 571.72 KiB |
4d9c9a7 | 1.72 MiB | 2.28 MiB | 571.72 KiB |
eac6dd1 | 1.72 MiB | 2.28 MiB | 571.65 KiB |
efb1927 | 1.72 MiB | 2.28 MiB | 571.72 KiB |
741ec32 | 1.72 MiB | 2.28 MiB | 571.72 KiB |
@marandaneto Will do. Probably sometime tomorrow |
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.
Mostly LGTM apart from the body size that I commented on.
And the tests need to be adapted, extended, of course to cover the error handling as well.
Thanks @marandaneto
sentry-apollo-3/src/main/java/io/sentry/apollo3/SentryApollo3HttpInterceptor.kt
Show resolved
Hide resolved
sentry-apollo-3/src/main/java/io/sentry/apollo3/SentryApollo3HttpInterceptor.kt
Outdated
Show resolved
Hide resolved
sentry-apollo-3/src/main/java/io/sentry/apollo3/SentryApollo3Interceptor.kt
Show resolved
Hide resolved
sentry-apollo-3/src/main/java/io/sentry/apollo3/SentryApollo3HttpInterceptor.kt
Outdated
Show resolved
Hide resolved
…va into chore/graphql_errors
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2781 +/- ##
============================================
+ Coverage 81.11% 81.13% +0.02%
- Complexity 4479 4493 +14
============================================
Files 347 348 +1
Lines 16510 16659 +149
Branches 2238 2263 +25
============================================
+ Hits 13392 13517 +125
- Misses 2183 2197 +14
- Partials 935 945 +10
☔ View full report in Codecov by Sentry. |
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.
LGTM, looks dope 🚀
sentry-apollo-3/src/main/java/io/sentry/apollo3/SentryApollo3ClientException.kt
Show resolved
Hide resolved
sentry-apollo-3/src/main/java/io/sentry/apollo3/SentryApollo3HttpInterceptor.kt
Show resolved
Hide resolved
|
||
val breadcrumb = Breadcrumb.http(request.url, request.method.name, statusCode) | ||
|
||
request.body?.contentLength.ifHasValidLength { contentLength -> | ||
breadcrumb.setData("request_body_size", contentLength) | ||
} | ||
|
||
operationName?.let { |
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.
l
: should we prefix these with graphql.
?
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.
Actually what I want is to extend https://develop.sentry.dev/sdk/event-payloads/breadcrumbs/ adding a new type
and category
for crumbs specifically to graphql
, I am checking this on the FE first, we can easily decide to change it later since data
is not indexed anyway, if we do that, graph.$something
is redundant because the category
is already graphql
.
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.
I see, that makes sense 👍
@romtsn thanks for the review, fixed a few things and replied to some others, is it fine? just wanna double check after my last changes although it's approved :) |
yeah looks good 👍 |
📜 Description
Support GraphQL Client errors for Apollo v3
💡 Motivation and Context
Relates to getsentry/sentry#33723
💚 How did you test it?
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps
Docs and test with the FE bits.