From c2f9e7ae552f904298272da62794bc1b8fd93186 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Fri, 17 Jun 2022 14:14:19 +0300 Subject: [PATCH] Update GraphQL instrumentation to match spec (#6179) * Update GraphQL instrumentation to match spec * add back removed method as deprecated * revert accidental change * change method order --- .../graphql-java-12.0/javaagent/README.md | 1 - .../javaagent/build.gradle.kts | 4 ---- .../graphql/GraphqlSingletons.java | 3 --- .../graphql/GraphQLTelemetry.java | 16 ++++---------- .../graphql/GraphQLTelemetryBuilder.java | 5 ++--- ...r.java => GraphqlAttributesExtractor.java} | 13 ++++++----- .../graphql/OpenTelemetryInstrumentation.java | 21 ++++++++---------- .../instrumentation/graphql/GraphqlTest.java | 5 +---- .../graphql/AbstractGraphqlTest.java | 22 +++++++++---------- 9 files changed, 34 insertions(+), 56 deletions(-) rename instrumentation/graphql-java-12.0/library/src/main/java/io/opentelemetry/instrumentation/graphql/{ExperimentalAttributesExtractor.java => GraphqlAttributesExtractor.java} (78%) diff --git a/instrumentation/graphql-java-12.0/javaagent/README.md b/instrumentation/graphql-java-12.0/javaagent/README.md index f7ff857903e5..7bf9addd2579 100644 --- a/instrumentation/graphql-java-12.0/javaagent/README.md +++ b/instrumentation/graphql-java-12.0/javaagent/README.md @@ -2,5 +2,4 @@ | System property | Type | Default | Description | |---|---|---------|--------------------------------------------------------------------------------------------| -| `otel.instrumentation.graphql.experimental-span-attributes` | Boolean | `false` | Enable the capture of experimental span attributes. | | `otel.instrumentation.graphql.query-sanitizer.enabled` | Boolean | `true` | Whether to remove sensitive information from query source that is added as span attribute. | diff --git a/instrumentation/graphql-java-12.0/javaagent/build.gradle.kts b/instrumentation/graphql-java-12.0/javaagent/build.gradle.kts index f2bf1b4407ab..699820cf67a6 100644 --- a/instrumentation/graphql-java-12.0/javaagent/build.gradle.kts +++ b/instrumentation/graphql-java-12.0/javaagent/build.gradle.kts @@ -19,7 +19,3 @@ dependencies { testImplementation(project(":instrumentation:graphql-java-12.0:testing")) } - -tasks.withType().configureEach { - jvmArgs("-Dotel.instrumentation.graphql.experimental-span-attributes=true") -} diff --git a/instrumentation/graphql-java-12.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/graphql/GraphqlSingletons.java b/instrumentation/graphql-java-12.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/graphql/GraphqlSingletons.java index bafbf5e85e14..3863a2a9112f 100644 --- a/instrumentation/graphql-java-12.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/graphql/GraphqlSingletons.java +++ b/instrumentation/graphql-java-12.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/graphql/GraphqlSingletons.java @@ -17,12 +17,9 @@ public final class GraphqlSingletons { private static final boolean QUERY_SANITIZATION_ENABLED = Config.get().getBoolean("otel.instrumentation.graphql.query-sanitizer.enabled", true); - private static final boolean CAPTURE_EXPERIMENTAL_SPAN_ATTRIBUTES = - Config.get().getBoolean("otel.instrumentation.graphql.experimental-span-attributes", false); private static final GraphQLTelemetry TELEMETRY = GraphQLTelemetry.builder(GlobalOpenTelemetry.get()) - .setCaptureExperimentalSpanAttributes(CAPTURE_EXPERIMENTAL_SPAN_ATTRIBUTES) .setSanitizeQuery(QUERY_SANITIZATION_ENABLED) .build(); diff --git a/instrumentation/graphql-java-12.0/library/src/main/java/io/opentelemetry/instrumentation/graphql/GraphQLTelemetry.java b/instrumentation/graphql-java-12.0/library/src/main/java/io/opentelemetry/instrumentation/graphql/GraphQLTelemetry.java index 926c1c3326a2..b51a07940c76 100644 --- a/instrumentation/graphql-java-12.0/library/src/main/java/io/opentelemetry/instrumentation/graphql/GraphQLTelemetry.java +++ b/instrumentation/graphql-java-12.0/library/src/main/java/io/opentelemetry/instrumentation/graphql/GraphQLTelemetry.java @@ -31,16 +31,12 @@ public static GraphQLTelemetryBuilder builder(OpenTelemetry openTelemetry) { } private final Instrumenter instrumenter; - private final boolean captureExperimentalSpanAttributes; private final boolean sanitizeQuery; - GraphQLTelemetry( - OpenTelemetry openTelemetry, - boolean captureExperimentalSpanAttributes, - boolean sanitizeQuery) { + GraphQLTelemetry(OpenTelemetry openTelemetry, boolean sanitizeQuery) { InstrumenterBuilder builder = Instrumenter.builder( - openTelemetry, INSTRUMENTATION_NAME, ignored -> "GraphQL Query") + openTelemetry, INSTRUMENTATION_NAME, ignored -> "GraphQL Operation") .setSpanStatusExtractor( (spanStatusBuilder, instrumentationExecutionParameters, executionResult, error) -> { if (!executionResult.getErrors().isEmpty()) { @@ -54,12 +50,9 @@ public static GraphQLTelemetryBuilder builder(OpenTelemetry openTelemetry) { error); } }); - if (captureExperimentalSpanAttributes) { - builder.addAttributesExtractor(new ExperimentalAttributesExtractor()); - } + builder.addAttributesExtractor(new GraphqlAttributesExtractor()); this.instrumenter = builder.newInstrumenter(); - this.captureExperimentalSpanAttributes = captureExperimentalSpanAttributes; this.sanitizeQuery = sanitizeQuery; } @@ -67,7 +60,6 @@ public static GraphQLTelemetryBuilder builder(OpenTelemetry openTelemetry) { * Returns a new {@link Instrumentation} that generates telemetry for received GraphQL requests. */ public Instrumentation newInstrumentation() { - return new OpenTelemetryInstrumentation( - instrumenter, captureExperimentalSpanAttributes, sanitizeQuery); + return new OpenTelemetryInstrumentation(instrumenter, sanitizeQuery); } } diff --git a/instrumentation/graphql-java-12.0/library/src/main/java/io/opentelemetry/instrumentation/graphql/GraphQLTelemetryBuilder.java b/instrumentation/graphql-java-12.0/library/src/main/java/io/opentelemetry/instrumentation/graphql/GraphQLTelemetryBuilder.java index 88a9c2cab023..f0ba043045c8 100644 --- a/instrumentation/graphql-java-12.0/library/src/main/java/io/opentelemetry/instrumentation/graphql/GraphQLTelemetryBuilder.java +++ b/instrumentation/graphql-java-12.0/library/src/main/java/io/opentelemetry/instrumentation/graphql/GraphQLTelemetryBuilder.java @@ -13,7 +13,6 @@ public final class GraphQLTelemetryBuilder { private final OpenTelemetry openTelemetry; - private boolean captureExperimentalSpanAttributes; private boolean sanitizeQuery = true; GraphQLTelemetryBuilder(OpenTelemetry openTelemetry) { @@ -25,9 +24,9 @@ public final class GraphQLTelemetryBuilder { * removed in the future, so only enable this if you know you do not require attributes filled by * this instrumentation to be stable across versions. */ + @Deprecated public GraphQLTelemetryBuilder setCaptureExperimentalSpanAttributes( boolean captureExperimentalSpanAttributes) { - this.captureExperimentalSpanAttributes = captureExperimentalSpanAttributes; return this; } @@ -42,6 +41,6 @@ public GraphQLTelemetryBuilder setSanitizeQuery(boolean sanitizeQuery) { * GraphQLTelemetryBuilder}. */ public GraphQLTelemetry build() { - return new GraphQLTelemetry(openTelemetry, captureExperimentalSpanAttributes, sanitizeQuery); + return new GraphQLTelemetry(openTelemetry, sanitizeQuery); } } diff --git a/instrumentation/graphql-java-12.0/library/src/main/java/io/opentelemetry/instrumentation/graphql/ExperimentalAttributesExtractor.java b/instrumentation/graphql-java-12.0/library/src/main/java/io/opentelemetry/instrumentation/graphql/GraphqlAttributesExtractor.java similarity index 78% rename from instrumentation/graphql-java-12.0/library/src/main/java/io/opentelemetry/instrumentation/graphql/ExperimentalAttributesExtractor.java rename to instrumentation/graphql-java-12.0/library/src/main/java/io/opentelemetry/instrumentation/graphql/GraphqlAttributesExtractor.java index aa03b5d4e77b..2dfbf3034dda 100644 --- a/instrumentation/graphql-java-12.0/library/src/main/java/io/opentelemetry/instrumentation/graphql/ExperimentalAttributesExtractor.java +++ b/instrumentation/graphql-java-12.0/library/src/main/java/io/opentelemetry/instrumentation/graphql/GraphqlAttributesExtractor.java @@ -11,17 +11,18 @@ import io.opentelemetry.api.common.AttributesBuilder; import io.opentelemetry.context.Context; import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor; +import java.util.Locale; import javax.annotation.Nullable; -final class ExperimentalAttributesExtractor +final class GraphqlAttributesExtractor implements AttributesExtractor { - // https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-graphql/src/enums/AttributeNames.ts + // https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/instrumentation/graphql.md private static final AttributeKey OPERATION_NAME = AttributeKey.stringKey("graphql.operation.name"); private static final AttributeKey OPERATION_TYPE = AttributeKey.stringKey("graphql.operation.type"); - private static final AttributeKey GRAPHQL_SOURCE = - AttributeKey.stringKey("graphql.source"); + private static final AttributeKey GRAPHQL_DOCUMENT = + AttributeKey.stringKey("graphql.document"); @Override public void onStart( @@ -39,8 +40,8 @@ public void onEnd( OpenTelemetryInstrumentationState state = request.getInstrumentationState(); attributes.put(OPERATION_NAME, state.getOperationName()); if (state.getOperation() != null) { - attributes.put(OPERATION_TYPE, state.getOperation().name()); + attributes.put(OPERATION_TYPE, state.getOperation().name().toLowerCase(Locale.ROOT)); } - attributes.put(GRAPHQL_SOURCE, state.getQuery()); + attributes.put(GRAPHQL_DOCUMENT, state.getQuery()); } } diff --git a/instrumentation/graphql-java-12.0/library/src/main/java/io/opentelemetry/instrumentation/graphql/OpenTelemetryInstrumentation.java b/instrumentation/graphql-java-12.0/library/src/main/java/io/opentelemetry/instrumentation/graphql/OpenTelemetryInstrumentation.java index 9474177339ba..cb72a50ca389 100644 --- a/instrumentation/graphql-java-12.0/library/src/main/java/io/opentelemetry/instrumentation/graphql/OpenTelemetryInstrumentation.java +++ b/instrumentation/graphql-java-12.0/library/src/main/java/io/opentelemetry/instrumentation/graphql/OpenTelemetryInstrumentation.java @@ -37,21 +37,19 @@ import io.opentelemetry.context.Scope; import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; import io.opentelemetry.semconv.trace.attributes.SemanticAttributes; +import java.util.Locale; final class OpenTelemetryInstrumentation extends SimpleInstrumentation { private static final NodeVisitor sanitizingVisitor = new SanitizingVisitor(); private static final AstTransformer astTransformer = new AstTransformer(); private final Instrumenter instrumenter; - private final boolean captureExperimentalSpanAttributes; private final boolean sanitizeQuery; OpenTelemetryInstrumentation( Instrumenter instrumenter, - boolean captureExperimentalSpanAttributes, boolean sanitizeQuery) { this.instrumenter = instrumenter; - this.captureExperimentalSpanAttributes = captureExperimentalSpanAttributes; this.sanitizeQuery = sanitizeQuery; } @@ -98,24 +96,23 @@ public InstrumentationContext beginExecuteOperation( OperationDefinition operationDefinition = parameters.getExecutionContext().getOperationDefinition(); Operation operation = operationDefinition.getOperation(); + String operationType = operation.name().toLowerCase(Locale.ROOT); String operationName = operationDefinition.getName(); - String spanName = operation.name(); + String spanName = operationType; if (operationName != null && !operationName.isEmpty()) { spanName += " " + operationName; } span.updateName(spanName); - if (captureExperimentalSpanAttributes) { - state.setOperation(operation); - state.setOperationName(operationName); + state.setOperation(operation); + state.setOperationName(operationName); - Node node = operationDefinition; - if (sanitizeQuery) { - node = sanitize(node); - } - state.setQuery(AstPrinter.printAst(node)); + Node node = operationDefinition; + if (sanitizeQuery) { + node = sanitize(node); } + state.setQuery(AstPrinter.printAst(node)); return SimpleInstrumentationContext.noOp(); } diff --git a/instrumentation/graphql-java-12.0/library/src/test/java/io/opentelemetry/instrumentation/graphql/GraphqlTest.java b/instrumentation/graphql-java-12.0/library/src/test/java/io/opentelemetry/instrumentation/graphql/GraphqlTest.java index d4a80cf93dc0..0579613a0d5b 100644 --- a/instrumentation/graphql-java-12.0/library/src/test/java/io/opentelemetry/instrumentation/graphql/GraphqlTest.java +++ b/instrumentation/graphql-java-12.0/library/src/test/java/io/opentelemetry/instrumentation/graphql/GraphqlTest.java @@ -22,10 +22,7 @@ protected InstrumentationExtension getTesting() { @Override protected void configure(GraphQL.Builder builder) { - GraphQLTelemetry telemetry = - GraphQLTelemetry.builder(testing.getOpenTelemetry()) - .setCaptureExperimentalSpanAttributes(true) - .build(); + GraphQLTelemetry telemetry = GraphQLTelemetry.builder(testing.getOpenTelemetry()).build(); builder.instrumentation(telemetry.newInstrumentation()); } } diff --git a/instrumentation/graphql-java-12.0/testing/src/main/java/io/opentelemetry/instrumentation/graphql/AbstractGraphqlTest.java b/instrumentation/graphql-java-12.0/testing/src/main/java/io/opentelemetry/instrumentation/graphql/AbstractGraphqlTest.java index 140d8adc273c..4ec8189b35d5 100644 --- a/instrumentation/graphql-java-12.0/testing/src/main/java/io/opentelemetry/instrumentation/graphql/AbstractGraphqlTest.java +++ b/instrumentation/graphql-java-12.0/testing/src/main/java/io/opentelemetry/instrumentation/graphql/AbstractGraphqlTest.java @@ -140,16 +140,16 @@ void successfulQuery() { trace -> trace.hasSpansSatisfyingExactly( span -> - span.hasName("QUERY findBookById") + span.hasName("query findBookById") .hasKind(SpanKind.INTERNAL) .hasNoParent() .hasAttributesSatisfyingExactly( equalTo( AttributeKey.stringKey("graphql.operation.name"), "findBookById"), - equalTo(AttributeKey.stringKey("graphql.operation.type"), "QUERY"), + equalTo(AttributeKey.stringKey("graphql.operation.type"), "query"), normalizedQueryEqualsTo( - AttributeKey.stringKey("graphql.source"), + AttributeKey.stringKey("graphql.document"), "query findBookById { bookById(id: ?) { name } }")), span -> span.hasName("fetchBookById").hasParent(trace.getSpan(0)))); } @@ -172,13 +172,13 @@ void successfulQueryWithoutName() { trace -> trace.hasSpansSatisfyingExactly( span -> - span.hasName("QUERY") + span.hasName("query") .hasKind(SpanKind.INTERNAL) .hasNoParent() .hasAttributesSatisfyingExactly( - equalTo(AttributeKey.stringKey("graphql.operation.type"), "QUERY"), + equalTo(AttributeKey.stringKey("graphql.operation.type"), "query"), normalizedQueryEqualsTo( - AttributeKey.stringKey("graphql.source"), + AttributeKey.stringKey("graphql.document"), "query { bookById(id: ?) { name } }")), span -> span.hasName("fetchBookById").hasParent(trace.getSpan(0)))); } @@ -195,7 +195,7 @@ void parseError() { trace -> trace.hasSpansSatisfyingExactly( span -> - span.hasName("GraphQL Query") + span.hasName("GraphQL Operation") .hasKind(SpanKind.INTERNAL) .hasNoParent() .hasAttributesSatisfying(Attributes::isEmpty) @@ -241,7 +241,7 @@ void validationError() { trace -> trace.hasSpansSatisfyingExactly( span -> - span.hasName("GraphQL Query") + span.hasName("GraphQL Operation") .hasKind(SpanKind.INTERNAL) .hasNoParent() .hasAttributesSatisfying(Attributes::isEmpty) @@ -286,16 +286,16 @@ void successfulMutation() { trace -> trace.hasSpansSatisfyingExactly( span -> - span.hasName("MUTATION addNewBook") + span.hasName("mutation addNewBook") .hasKind(SpanKind.INTERNAL) .hasNoParent() .hasAttributesSatisfyingExactly( equalTo( AttributeKey.stringKey("graphql.operation.name"), "addNewBook"), equalTo( - AttributeKey.stringKey("graphql.operation.type"), "MUTATION"), + AttributeKey.stringKey("graphql.operation.type"), "mutation"), normalizedQueryEqualsTo( - AttributeKey.stringKey("graphql.source"), + AttributeKey.stringKey("graphql.document"), "mutation addNewBook { addBook(id: ?, name: ?, author: ?) { id } }")))); }