Improve server side GraphQL support for spring-graphql and Nextflix DGS#2856
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.
Can you set all the information besides the data?
There was a problem hiding this comment.
You mean statuscode, headers and cookies?
There was a problem hiding this comment.
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.
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.
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.
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.
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
| import org.jetbrains.annotations.NotNull; | ||
| import reactor.core.publisher.Flux; | ||
|
|
||
| public final class SentryDgsSubscriptionHandler implements SentrySubscriptionHandler { |
There was a problem hiding this comment.
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. |
adinauer
left a comment
There was a problem hiding this comment.
@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. |
lbloder
left a comment
There was a problem hiding this comment.
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-javaand Netflix DGS. This PR adds on top of that:ExecutionResult(more specifically itserrors)GraphQLContext💡 Motivation and Context
Fixes #2790
💚 How did you test it?
graphiqlhosted by the sample at http://localhost:8080/graphiqlPlease take a look at the sample code to see that inputs cause which errors.
Queries used
Details
Netflix DGS queries
Shows
New shows
Mutation
variables:
Subscription
variables:
Data loader
Spring queries
Greeting
Greeting with variables
Sample events:
crashvariables:
Project
Sample events:
statusvia a separate data fetcher with slugstatuscrashvariables:
Mutation
Sample events:
addprojectcrashvariables:
Subscription
Sample events:
produceerrorcrashfluxerrorvariables:
Data loader
Sample events:
Assigneevia data loader with slugcrashvariables:
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps