Skip to content
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

Feat: Add graphql-java instrumentation. #1777

Merged
merged 25 commits into from
Nov 8, 2021
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
c1f11cb
Feat: Add `graphql-java` instrumentation.
maciejwalkowiak Oct 22, 2021
5e454a7
Changelog.
maciejwalkowiak Oct 22, 2021
f0f9027
Merge remote-tracking branch 'origin/main' into gh-1755
maciejwalkowiak Oct 25, 2021
3d27109
Reformat.
maciejwalkowiak Oct 25, 2021
e861e57
Add graphql exception handler.
maciejwalkowiak Oct 25, 2021
a43603b
Add Netflix DGS sample
maciejwalkowiak Oct 25, 2021
67265a2
Add missing files.
maciejwalkowiak Oct 25, 2021
4ef521d
Polish.
maciejwalkowiak Oct 28, 2021
ef4b780
Remove `java` package.
maciejwalkowiak Oct 28, 2021
60474bc
Polish.
maciejwalkowiak Oct 28, 2021
9245a43
Merge remote-tracking branch 'origin/main' into gh-1755
maciejwalkowiak Oct 29, 2021
f16a681
Merge branch 'main' into gh-1755
bruno-garcia Nov 2, 2021
a66aeed
Merge remote-tracking branch 'origin/main' into gh-1755
maciejwalkowiak Nov 2, 2021
e37ddd9
Merge branch 'gh-1755' of https://github.com/getsentry/sentry-java in…
maciejwalkowiak Nov 2, 2021
de5d9bd
Remove API check from Netflix DGS sample.
maciejwalkowiak Nov 2, 2021
542c6b6
Fix instrumentation:
maciejwalkowiak Nov 2, 2021
8180ec2
Update sentry-graphql-java/src/main/java/io/sentry/graphql/SentryInst…
maciejwalkowiak Nov 3, 2021
8ec6a1f
Update sentry-graphql-java/src/main/java/io/sentry/graphql/SentryInst…
maciejwalkowiak Nov 3, 2021
96c8e30
Pass parameters as a hint
maciejwalkowiak Nov 3, 2021
cabfa4a
Add BeforeSpan to SentryInstrumentation.
maciejwalkowiak Nov 3, 2021
b0fc86d
Merge branch 'gh-1755' of https://github.com/getsentry/sentry-java in…
maciejwalkowiak Nov 3, 2021
6c77fca
Add craft.
maciejwalkowiak Nov 3, 2021
9c55ee8
Do not set status on parent span.
maciejwalkowiak Nov 8, 2021
f42ef1f
Polish.
maciejwalkowiak Nov 8, 2021
10258a2
Set span status.
maciejwalkowiak Nov 8, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

## Unreleased

* Feat: Add `graphql-java` instrumentation (#1777)

## 5.3.0

* Feat: Add datasource tracing with P6Spy (#1784)
Expand Down
4 changes: 4 additions & 0 deletions buildSrc/src/main/java/Config.kt
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,10 @@ object Config {
val apolloCoroutines = "com.apollographql.apollo:apollo-coroutines-support:$apolloVersion"

val p6spy = "p6spy:p6spy:3.9.1"

val graphQlJava = "com.graphql-java:graphql-java:17.3"

val kotlinReflect = "org.jetbrains.kotlin:kotlin-reflect"
}

object AnnotationProcessors {
Expand Down
15 changes: 15 additions & 0 deletions sentry-graphql-java/api/sentry-graphql-java.api
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
public final class io/sentry/graphql/SentryDataFetcherExceptionHandler : graphql/execution/DataFetcherExceptionHandler {
public fun <init> (Lgraphql/execution/DataFetcherExceptionHandler;)V
public fun <init> (Lio/sentry/IHub;Lgraphql/execution/DataFetcherExceptionHandler;)V
public fun onException (Lgraphql/execution/DataFetcherExceptionHandlerParameters;)Lgraphql/execution/DataFetcherExceptionHandlerResult;
}

public final class io/sentry/graphql/SentryInstrumentation : graphql/execution/instrumentation/SimpleInstrumentation {
public fun <init> ()V
public fun <init> (Lio/sentry/IHub;)V
public fun beginExecution (Lgraphql/execution/instrumentation/parameters/InstrumentationExecutionParameters;)Lgraphql/execution/instrumentation/InstrumentationContext;
public fun createState ()Lgraphql/execution/instrumentation/InstrumentationState;
public fun instrumentDataFetcher (Lgraphql/schema/DataFetcher;Lgraphql/execution/instrumentation/parameters/InstrumentationFieldFetchParameters;)Lgraphql/schema/DataFetcher;
public fun instrumentExecutionResult (Lgraphql/ExecutionResult;Lgraphql/execution/instrumentation/parameters/InstrumentationExecutionParameters;)Ljava/util/concurrent/CompletableFuture;
}

76 changes: 76 additions & 0 deletions sentry-graphql-java/build.gradle.kts
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
import net.ltgt.gradle.errorprone.errorprone
import org.jetbrains.kotlin.gradle.tasks.KotlinCompile

plugins {
`java-library`
kotlin("jvm")
jacoco
id(Config.QualityPlugins.errorProne)
id(Config.QualityPlugins.gradleVersions)
}

configure<JavaPluginExtension> {
sourceCompatibility = JavaVersion.VERSION_1_8
targetCompatibility = JavaVersion.VERSION_1_8
}

tasks.withType<KotlinCompile>().configureEach {
kotlinOptions.jvmTarget = JavaVersion.VERSION_1_8.toString()
kotlinOptions.languageVersion = Config.springKotlinCompatibleLanguageVersion
}

dependencies {
api(projects.sentry)
implementation(Config.Libs.graphQlJava)

compileOnly(Config.CompileOnly.nopen)
errorprone(Config.CompileOnly.nopenChecker)
errorprone(Config.CompileOnly.errorprone)
errorprone(Config.CompileOnly.errorProneNullAway)
compileOnly(Config.CompileOnly.jetbrainsAnnotations)

// tests
testImplementation(projects.sentry)
testImplementation(projects.sentryTestSupport)
testImplementation(kotlin(Config.kotlinStdLib))
testImplementation(Config.TestLibs.kotlinTestJunit)
testImplementation(Config.TestLibs.mockitoKotlin)
testImplementation(Config.TestLibs.mockWebserver)
testImplementation(Config.Libs.okhttp)
}

configure<SourceSetContainer> {
test {
java.srcDir("src/test/java")
}
}

jacoco {
toolVersion = Config.QualityPlugins.Jacoco.version
}

tasks.jacocoTestReport {
reports {
xml.required.set(true)
html.required.set(false)
}
}

tasks {
jacocoTestCoverageVerification {
violationRules {
rule { limit { minimum = Config.QualityPlugins.Jacoco.minimumCoverage } }
}
}
check {
dependsOn(jacocoTestCoverageVerification)
dependsOn(jacocoTestReport)
}
}

tasks.withType<JavaCompile>().configureEach {
options.errorprone {
check("NullAway", net.ltgt.gradle.errorprone.CheckSeverity.ERROR)
option("NullAway:AnnotatedPackages", "io.sentry")
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package io.sentry.graphql;

import graphql.execution.DataFetcherExceptionHandler;
import graphql.execution.DataFetcherExceptionHandlerParameters;
import graphql.execution.DataFetcherExceptionHandlerResult;
import io.sentry.HubAdapter;
import io.sentry.IHub;
import io.sentry.util.Objects;
import org.jetbrains.annotations.NotNull;

/**
* Captures exceptions that occur during data fetching, passes them to Sentry and invokes a delegate
* exception handler.
*/
public final class SentryDataFetcherExceptionHandler implements DataFetcherExceptionHandler {
private final @NotNull IHub hub;
private final DataFetcherExceptionHandler delegate;
marandaneto marked this conversation as resolved.
Show resolved Hide resolved

public SentryDataFetcherExceptionHandler(
final @NotNull IHub hub, final @NotNull DataFetcherExceptionHandler delegate) {
this.hub = Objects.requireNonNull(hub, "hub is required");
this.delegate = Objects.requireNonNull(delegate, "delegate is required");
}

public SentryDataFetcherExceptionHandler(final @NotNull DataFetcherExceptionHandler delegate) {
this(HubAdapter.getInstance(), delegate);
}

@Override
@SuppressWarnings("deprecation")
public DataFetcherExceptionHandlerResult onException(
final @NotNull DataFetcherExceptionHandlerParameters handlerParameters) {
hub.captureException(handlerParameters.getException());
marandaneto marked this conversation as resolved.
Show resolved Hide resolved
return delegate.onException(handlerParameters);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
package io.sentry.graphql;

import graphql.ExecutionResult;
import graphql.execution.instrumentation.InstrumentationContext;
import graphql.execution.instrumentation.InstrumentationState;
import graphql.execution.instrumentation.SimpleInstrumentation;
import graphql.execution.instrumentation.parameters.InstrumentationExecutionParameters;
import graphql.execution.instrumentation.parameters.InstrumentationFieldFetchParameters;
import graphql.schema.DataFetcher;
import graphql.schema.GraphQLNonNull;
import graphql.schema.GraphQLObjectType;
import graphql.schema.GraphQLOutputType;
import io.sentry.HubAdapter;
import io.sentry.IHub;
import io.sentry.ISpan;
import io.sentry.util.Objects;
import java.util.concurrent.CompletableFuture;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

public final class SentryInstrumentation extends SimpleInstrumentation {
private final @NotNull IHub hub;

public SentryInstrumentation(final @NotNull IHub hub) {
this.hub = Objects.requireNonNull(hub, "hub is required");
}

public SentryInstrumentation() {
this(HubAdapter.getInstance());
}

@Override
public @NotNull InstrumentationState createState() {
return new TracingState();
}

@Override
public @NotNull InstrumentationContext<ExecutionResult> beginExecution(
final @NotNull InstrumentationExecutionParameters parameters) {
TracingState tracingState = parameters.getInstrumentationState();
maciejwalkowiak marked this conversation as resolved.
Show resolved Hide resolved
tracingState.setTransaction(hub.getSpan());
marandaneto marked this conversation as resolved.
Show resolved Hide resolved
return super.beginExecution(parameters);
}

@Override
@SuppressWarnings("FutureReturnValueIgnored")
public @NotNull DataFetcher<?> instrumentDataFetcher(
final @NotNull DataFetcher<?> dataFetcher,
final @NotNull InstrumentationFieldFetchParameters parameters) {
// We only care about user code
if (parameters.isTrivialDataFetcher()) {
return dataFetcher;
}

return environment -> {
final TracingState tracingState = parameters.getInstrumentationState();
final ISpan transaction = tracingState.getTransaction();
if (transaction != null) {
Comment on lines +66 to +67

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see the Docs PR getsentry/sentry-docs#4385

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

final ISpan span = transaction.startChild(findDataFetcherTag(parameters));
final Object result = dataFetcher.get(environment);
if (result instanceof CompletableFuture) {
((CompletableFuture<?>) result)
.whenComplete(
(r, ex) -> {
span.finish();
});
} else {
span.finish();
}
return result;
} else {
return dataFetcher.get(environment);
}
};
}

@Override
public @NotNull CompletableFuture<ExecutionResult> instrumentExecutionResult(
final @NotNull ExecutionResult executionResult,
final @NotNull InstrumentationExecutionParameters parameters) {
TracingState tracingState = parameters.getInstrumentationState();
maciejwalkowiak marked this conversation as resolved.
Show resolved Hide resolved
final ISpan transaction = tracingState.getTransaction();

if (transaction != null) {
transaction.finish();
}

return super.instrumentExecutionResult(executionResult, parameters);
}

private @NotNull String findDataFetcherTag(
final @NotNull InstrumentationFieldFetchParameters parameters) {
final GraphQLOutputType type = parameters.getExecutionStepInfo().getParent().getType();
GraphQLObjectType parent;
if (type instanceof GraphQLNonNull) {
parent = (GraphQLObjectType) ((GraphQLNonNull) type).getWrappedType();
} else {
parent = (GraphQLObjectType) type;
}

return parent.getName() + "." + parameters.getExecutionStepInfo().getPath().getSegmentName();
}

static final class TracingState implements InstrumentationState {
private @Nullable ISpan transaction;

public @Nullable ISpan getTransaction() {
return transaction;
}

public void setTransaction(final @Nullable ISpan transaction) {
this.transaction = transaction;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package io.sentry.graphql

import com.nhaarman.mockitokotlin2.mock
import com.nhaarman.mockitokotlin2.verify
import graphql.execution.DataFetcherExceptionHandler
import graphql.execution.DataFetcherExceptionHandlerParameters
import io.sentry.IHub
import kotlin.test.Test

class SentryDataFetcherExceptionHandlerTest {

@Test
fun `passes exception to Sentry and invokes delegate`() {
val hub = mock<IHub>()
val delegate = mock<DataFetcherExceptionHandler>()
val handler = SentryDataFetcherExceptionHandler(hub, delegate)

val exception = RuntimeException()
val parameters = DataFetcherExceptionHandlerParameters.newExceptionParameters().exception(exception).build()
handler.onException(parameters)

verify(hub).captureException(exception)
verify(delegate).onException(parameters)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
package io.sentry.graphql

import com.nhaarman.mockitokotlin2.any
import com.nhaarman.mockitokotlin2.mock
import com.nhaarman.mockitokotlin2.verify
import com.nhaarman.mockitokotlin2.verifyNoMoreInteractions
import com.nhaarman.mockitokotlin2.whenever
import graphql.GraphQL
import graphql.schema.idl.RuntimeWiring
import graphql.schema.idl.SchemaGenerator
import graphql.schema.idl.SchemaParser
import io.sentry.IHub
import io.sentry.ISpan
import kotlin.random.Random
import kotlin.test.Test
import kotlin.test.assertTrue

class SentryInstrumentationTest {

class Fixture {
val transaction = mock<ISpan>()
val innerSpan = mock<ISpan>()

fun getSut(isTransactionActive: Boolean = true): GraphQL {
val schema = """
type Query {
shows: [Show]
}

type Show {
id: Int
}
""".trimIndent()
val hub = mock<IHub>()

val graphQLSchema = SchemaGenerator().makeExecutableSchema(SchemaParser().parse(schema), buildRuntimeWiring())
val graphQL = GraphQL.newGraphQL(graphQLSchema)
.instrumentation(SentryInstrumentation(hub))
.build()

if (isTransactionActive) {
whenever(hub.span).thenReturn(transaction)
} else {
whenever(hub.span).thenReturn(null)
}
whenever(transaction.startChild(any())).thenReturn(innerSpan)

return graphQL
}

private fun buildRuntimeWiring() = RuntimeWiring.newRuntimeWiring()
.type("Query") {
it.dataFetcher("shows") {
listOf(Show(Random.nextInt()), Show(Random.nextInt()))
}
}.build()
}

private val fixture = Fixture()

@Test
fun `when transaction is active, creates inner spans`() {
val sut = fixture.getSut()

val result = sut.execute("{ shows { id } }")

assertTrue(result.errors.isEmpty())
verify(fixture.transaction).startChild("Query.shows")
verify(fixture.innerSpan).finish()
verify(fixture.transaction).finish()
}

@Test
fun `when transaction is not , does not create spans`() {
val sut = fixture.getSut(isTransactionActive = false)

val result = sut.execute("{ shows { id } }")

assertTrue(result.errors.isEmpty())
verifyNoMoreInteractions(fixture.transaction)
verifyNoMoreInteractions(fixture.innerSpan)
}

data class Show(val id: Int)
}
Loading