Feat: Add graphql-java instrumentation.

[maciejwalkowiak]

📜 Description

Add graphql-java instrumentation.

• exception handler - SentryDataFetcherExceptionHandler reports exception to Sentry and invokes a delegate

Sample usage with Netflix DGS:

@Bean
SentryDataFetcherExceptionHandler sentryDataFetcherExceptionHandler() {
  // delegate to default Netflix DGS exception handler
  return new SentryDataFetcherExceptionHandler(new DefaultDataFetcherExceptionHandler());
}

• tracing - SentryInstrumentation creates spans around data fetchers. Since there can be more than a single query executed within a single HTTP request, the transaction name is POST /graphql and resolving each query/field is reported as separate span.

It is advised to turn on sentry.max-request-body-size reporting, 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-java directly.

💚 How did you test it?

Unit tests.

📝 Checklist

• I reviewed the submitted code
• I added tests to verify the changes
• I updated the docs if needed
• No breaking changes

🔮 Next steps

• Add error handler
• Create sample project

Optionally, once Spring GraphQL project reaches GA, add corresponding sample.

[codecov-commenter]

Codecov Report

Merging #1777 (8ec6a1f) into main (2d8a74a) will decrease coverage by 0.02%.
The diff coverage is 72.72%.

@@             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     

Impacted Files	Coverage Δ
.../java/io/sentry/graphql/SentryInstrumentation.java	72.34% <72.34%> (ø)
...try/graphql/SentryDataFetcherExceptionHandler.java	75.00% <75.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d8a74a...8ec6a1f. Read the comment docs.

[bruno-garcia]

Do we need to have the .java in the name?

[marandaneto]

still called sentry-graphql-java, should we rename it to sentry-graphql? we only add the android suffix to Android modules

[maciejwalkowiak]

the library we integrate with is called graphql-java - i do think it should stay as the module name.

[marandaneto]

@bruno-garcia are u fine with it? I recall you didn't want to add such sufixes

[bruno-garcia]

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

[marandaneto]

with or without fine with me, I'd rather keep it without -java if I'd have to vote.
@romtsn ?

[romtsn]

same maybe call it sentry-graphql-server? :D fine with everything

[maciejwalkowiak]

Few improvements to instrumentation:

• do not finish parent span
• handle exceptions thrown by data fetchers
• set parent span status if any of child spans fails

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 INTERNAL_ERROR.

[marandaneto]

missing the craft configuration, release-registry and docs later on :)

the module name is still with -java

[marandaneto]

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?

[marandaneto]

LGTM

[koenpunt]

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?

[marandaneto]

see the Docs PR getsentry/sentry-docs#4385

[koenpunt]

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?

[marandaneto]

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

[koenpunt]

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.

[marandaneto]

good catch, #1807
no priorities for now, but if you feel adding support via PR, happy to guide :)