-
-
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
Improve server side GraphQL support for spring-graphql and Nextflix DGS #2856
Conversation
|
if (result != null) { | ||
@NotNull Response response = new Response(); | ||
Map<String, Object> responseBody = result.toSpecification(); | ||
response.setData(responseBody); |
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.
Can you set all the information besides the data
?
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.
You mean statuscode, headers and cookies?
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 can probably attach some of it for Spring in an event processor but since the response isn't finished yet it might very well be wrong / incomplete. Would you still like me to add it?
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.
Can you add it after the request is made? This does not need to be GraphQL-specific since it makes sense for every integration.
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.
Can you add it after the request is made?
I don't understand. Do you mean after the response is finished? We'd have to delay sending an event to Sentry, probably put the response stuff on the scope and pick it up from there once ready. Sounds brittle and complicated.
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.
Yes, after the request is "finished".
I don't know the internals to tell how complicated is, but we do that with transactions and spans right, we collect all the information and "capture" at the end once everything is figured out, maybe we need something similar.
It's not a must but wishable.
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.
Can we do this in a follow up PR?
sentry-graphql/src/main/java/io/sentry/graphql/ExceptionReporter.java
Outdated
Show resolved
Hide resolved
sentry-graphql/src/main/java/io/sentry/graphql/ExceptionReporter.java
Outdated
Show resolved
Hide resolved
sentry-graphql/src/main/java/io/sentry/graphql/ExceptionReporter.java
Outdated
Show resolved
Hide resolved
sentry-spring/src/main/java/io/sentry/spring/graphql/SentrySpringSubscriptionHandler.java
Show resolved
Hide resolved
sentry-graphql/src/main/java/io/sentry/graphql/SentryInstrumentation.java
Outdated
Show resolved
Hide resolved
sentry-graphql/src/main/java/io/sentry/graphql/ExceptionReporter.java
Outdated
Show resolved
Hide resolved
sentry-graphql/src/main/java/io/sentry/graphql/SentryGraphqlExceptionHandler.java
Outdated
Show resolved
Hide resolved
sentry-graphql/src/main/java/io/sentry/graphql/ExceptionReporter.java
Outdated
Show resolved
Hide resolved
sentry-graphql/src/main/java/io/sentry/graphql/ExceptionReporter.java
Outdated
Show resolved
Hide resolved
import org.jetbrains.annotations.NotNull; | ||
import reactor.core.publisher.Flux; | ||
|
||
public final class SentryDgsSubscriptionHandler implements SentrySubscriptionHandler { |
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.
put this here as it requires reactor
. If we added reactor to sentry-graphql
we could rename it to something generic and put it there. Not sure we should do that tho.
sentry-graphql/src/main/java/io/sentry/graphql/NoOpSubscriptionHandler.java
Show resolved
Hide resolved
sentry-graphql/src/main/java/io/sentry/graphql/SentryGraphqlExceptionHandler.java
Outdated
Show resolved
Hide resolved
sentry-graphql/src/main/java/io/sentry/graphql/SentryInstrumentation.java
Outdated
Show resolved
Hide resolved
sentry-graphql/src/main/java/io/sentry/graphql/SentryInstrumentation.java
Show resolved
Hide resolved
sentry-graphql/src/main/java/io/sentry/graphql/SentryInstrumentation.java
Show resolved
Hide resolved
sentry-graphql/src/main/java/io/sentry/graphql/SentryInstrumentation.java
Show resolved
Hide resolved
...entry-samples-spring-boot/src/main/java/io/sentry/samples/spring/boot/ProjectController.java
Outdated
Show resolved
Hide resolved
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
3baa73f | 267.45 ms | 388.30 ms | 120.85 ms |
9246ed4 | 281.79 ms | 352.08 ms | 70.29 ms |
2718fc8 | 322.33 ms | 369.98 ms | 47.65 ms |
f274c79 | 313.96 ms | 355.96 ms | 42.00 ms |
4c237f8 | 319.84 ms | 354.47 ms | 34.63 ms |
4bf202b | 364.28 ms | 419.66 ms | 55.38 ms |
dc67004 | 273.86 ms | 346.37 ms | 72.51 ms |
496bdfd | 301.22 ms | 343.96 ms | 42.73 ms |
8820c5c | 330.60 ms | 416.86 ms | 86.26 ms |
f60279b | 324.60 ms | 345.33 ms | 20.73 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
3baa73f | 1.72 MiB | 2.29 MiB | 575.52 KiB |
9246ed4 | 1.72 MiB | 2.28 MiB | 572.22 KiB |
2718fc8 | 1.72 MiB | 2.29 MiB | 575.53 KiB |
f274c79 | 1.72 MiB | 2.29 MiB | 575.01 KiB |
4c237f8 | 1.72 MiB | 2.29 MiB | 575.58 KiB |
4bf202b | 1.72 MiB | 2.29 MiB | 575.54 KiB |
dc67004 | 1.72 MiB | 2.28 MiB | 573.45 KiB |
496bdfd | 1.72 MiB | 2.28 MiB | 571.82 KiB |
8820c5c | 1.72 MiB | 2.28 MiB | 571.82 KiB |
f60279b | 1.72 MiB | 2.29 MiB | 575.23 KiB |
Previous results on branch: feat/improved-graphql-server-support
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
7c99037 | 297.06 ms | 362.04 ms | 64.98 ms |
2702851 | 295.00 ms | 366.50 ms | 71.50 ms |
93f6eb8 | 293.06 ms | 327.16 ms | 34.10 ms |
1412898 | 344.34 ms | 382.63 ms | 38.29 ms |
dbb5dc4 | 331.08 ms | 352.98 ms | 21.90 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
7c99037 | 1.72 MiB | 2.29 MiB | 575.54 KiB |
2702851 | 1.72 MiB | 2.29 MiB | 575.54 KiB |
93f6eb8 | 1.72 MiB | 2.29 MiB | 575.54 KiB |
1412898 | 1.72 MiB | 2.29 MiB | 575.54 KiB |
dbb5dc4 | 1.72 MiB | 2.29 MiB | 575.54 KiB |
sentry-spring/src/main/java/io/sentry/spring/graphql/SentryBatchLoaderRegistry.java
Show resolved
Hide resolved
...spring/src/main/java/io/sentry/spring/graphql/SentryDataFetcherExceptionResolverAdapter.java
Show resolved
Hide resolved
...spring/src/main/java/io/sentry/spring/graphql/SentryDataFetcherExceptionResolverAdapter.java
Show resolved
Hide resolved
Just nitpick, for later, missing null annotations, and |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2856 +/- ##
============================================
- Coverage 81.39% 80.45% -0.95%
- Complexity 4652 4701 +49
============================================
Files 354 371 +17
Lines 17134 17556 +422
Branches 2314 2364 +50
============================================
+ Hits 13947 14125 +178
- Misses 2237 2454 +217
- Partials 950 977 +27
☔ 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.
@sentrivana I marked what I think makes sense to take a look at with 👀 for you. Feel free to review more if you feel like it ;-)
sentry-graphql/src/main/java/io/sentry/graphql/SentryGraphqlExceptionHandler.java
Show resolved
Hide resolved
sentry-graphql/src/main/java/io/sentry/graphql/SentryInstrumentation.java
Show resolved
Hide resolved
Thanks for prescreening things :) I will take a look later today or on Monday. |
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, except for some nitpicks :)
Also, extending existing READMEs or adding one on how to call the graphql endpoints in the samples might be nice
📜 Description
We already have basic support for
graphql-java
and Netflix DGS. This PR adds on top of that:ExecutionResult
(more specifically itserrors
)GraphQLContext
💡 Motivation and Context
Fixes #2790
💚 How did you test it?
graphiql
hosted by the sample at http://localhost:8080/graphiqlPlease take a look at the sample code to see that inputs cause which errors.
Queries used
Netflix DGS queries
Shows
New shows
Mutation
variables:
Subscription
variables:
Data loader
Spring queries
Greeting
Greeting with variables
Sample events:
crash
variables:
Project
Sample events:
status
via a separate data fetcher with slugstatuscrash
variables:
Mutation
Sample events:
addprojectcrash
variables:
Subscription
Sample events:
produceerror
crash
fluxerror
variables:
Data loader
Sample events:
Assignee
via data loader with slugcrash
variables:
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps