Feat: Add graphql-java instrumentation.#1777
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1777 +/- ##
============================================
- Coverage 75.75% 75.73% -0.03%
- Complexity 2180 2193 +13
============================================
Files 216 218 +2
Lines 7739 7794 +55
Branches 824 828 +4
============================================
+ Hits 5863 5903 +40
- Misses 1476 1488 +12
- Partials 400 403 +3
Continue to review full report at Codecov.
|
| @@ -0,0 +1,36 @@ | |||
| package io.sentry.graphql.java; | |||
There was a problem hiding this comment.
Do we need to have the .java in the name?
There was a problem hiding this comment.
still called sentry-graphql-java, should we rename it to sentry-graphql? we only add the android suffix to Android modules
There was a problem hiding this comment.
the library we integrate with is called graphql-java - i do think it should stay as the module name.
There was a problem hiding this comment.
@bruno-garcia are u fine with it? I recall you didn't want to add such sufixes
There was a problem hiding this comment.
I don't think it must match in all cases the package name. The proposal from manoel sounds better to me unless folks have strong options not to take that route
There was a problem hiding this comment.
with or without fine with me, I'd rather keep it without -java if I'd have to vote.
@romtsn ?
There was a problem hiding this comment.
same maybe call it sentry-graphql-server? :D fine with everything
- do not finish parent span - handle exceptions thrown by data fetchers - set parent span status if any of child spans fails
|
Few improvements to instrumentation:
So now, if any of the child spans fails - meaning that there are errors present in GraphQL response, we set the parent span status - which in most cases will be a transaction - to |
…rumentation.java Co-authored-by: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com>
…rumentation.java Co-authored-by: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com>
|
missing the craft configuration, release-registry and docs later on :) the module name is still with |
|
let's discuss https://github.com/getsentry/sentry-java/pull/1777/files#r742653695 today and approve/merge, nothing but this comment blocks me to approve, merge and release, thanks for doing this @maciejwalkowiak @romtsn wanna have a look? |
| final ISpan transaction = tracingState.getTransaction(); | ||
| if (transaction != null) { |
There was a problem hiding this comment.
I'm trying to use the SentryInstrumentation in our spring-graphql application (using Spring Boot), but the transaction here is always null. Is it necessary to manually start a transaction? Or how can I get this to work?
There was a problem hiding this comment.
Thanks, I didn't see that PR yet. But unfortunately it doesn't solve the issue; performance tracing is set up, but apparently a transaction isn't started before graphql execution.
For now I think I've mitigated it by adding a WebInterceptor (a spring-graphql concept), that starts a transaction;
class SentryWebInterceptor : WebInterceptor {
override fun intercept(webInput: WebInput, chain: WebInterceptorChain): Mono<WebOutput> {
val span = Sentry.getSpan() ?: Sentry.startTransaction("GraphQL", webInput.operationName ?: "none", true)
return chain.next(webInput).doFinally {
span.finish()
}
}
}According to the PR description "the transaction name is POST /graphql", but this doesn't seem to be defined explicitly, so could it be that the assumption is made that the the http/network layer already starts the transaction, but that somehow with spring-graphql this doesn't happen?
There was a problem hiding this comment.
the graphql integration only creates spans if there's an active transaction, so ideally you'd use it along with an integration that has the ability to create a transaction, eg https://docs.sentry.io/platforms/java/guides/spring-boot/performance/instrumentation/automatic-instrumentation/
or just create a transaction yourself
There was a problem hiding this comment.
Just found that out too, but unfortunately there's no auto instrumentation available for applications using webflux, so I guess I have to implement what's in SentryTracingFilter, but then reactive.
There was a problem hiding this comment.
good catch, #1807
no priorities for now, but if you feel adding support via PR, happy to guide :)
📜 Description
Add
graphql-javainstrumentation.SentryDataFetcherExceptionHandlerreports exception to Sentry and invokes a delegateSample usage with Netflix DGS:
SentryInstrumentationcreates spans around data fetchers. Since there can be more than a single query executed within a single HTTP request, the transaction name isPOST /graphqland resolving each query/field is reported as separate span.It is advised to turn on
sentry.max-request-body-sizereporting, to get a complete GraphQL query in request parameters.💡 Motivation and Context
Fixes #1755
Both Netflix DGS and Spring GraphQL are based on graphql-java. Changes introduced in this PR will work for both solutions as well as using
graphql-javadirectly.💚 How did you test it?
Unit tests.
📝 Checklist
🔮 Next steps
Optionally, once Spring GraphQL project reaches GA, add corresponding sample.