Fix high cardinality issue with "graphql.operation"

Prior to this commit, the GraphQL observability instrumentation would
add a "graphql.operation" low cardinality keyvalue to the request
observations. This value is extracted from the `ExecutionInput`.
But this value can be effectively high cardinality as it is driven by
user input and clients can set any name there.

This commit fixes this cardinality issue by:

* renaming "graphql.operation" to "graphql.operation.name" and making it
  high cardinality.
* adding a new "graphql.operation.type" low cardinality keyvalue that is
  extracted from the `ExecutionContext`, after the request document has
  been parses and validated. This information is now used for the trace
  context name.

Fixes gh-1322

Author: bclozel

spring-graphql-docs/modules/ROOT/pages/observability.adoc

@@ -27,11 +27,12 @@ By default, the following KeyValues are created:
 [cols="a,a"]
 |===
 |Name | Description
-|`graphql.operation` _(required)_|GraphQL Operation name.
+|`graphql.operation.type` _(required)_|GraphQL Operation type.
 |`graphql.outcome` _(required)_|Outcome of the GraphQL request.
 |===
 
-The `graphql.operation` KeyValue will use the custom name of the provided query, or http://spec.graphql.org/draft/#sec-Language.Operations[the standard name for the operation] if none (`"query"`, `"mutation"` or `"subscription"`).
+The `graphql.operation.type` KeyValue will use the http://spec.graphql.org/draft/#sec-Language.Operations[the standard name for the operation] (`"query"`, `"mutation"` or `"subscription"`) or `"operation"` if the request document could not be parsed.
+
 The `graphql.outcome` KeyValue will be:
 
 * `"SUCCESS"` if a valid GraphQL response has been sent and it contains no errors
@@ -43,8 +44,11 @@ The `graphql.outcome` KeyValue will be:
 |===
 |Name | Description
 |`graphql.execution.id` _(required)_|`graphql.execution.ExecutionId` of the GraphQL request.
+|`graphql.operation.name` _(required)_|GraphQL Operation name.
 |===
 
+The `graphql.operation.name` KeyValue will be similar to `graphql.operation.name` will use the operation name provided by the client.
+
 Spring for GraphQL also contributes Events for Server Request Observations.
 https://docs.micrometer.io/micrometer/reference/observation/components.html#micrometer-observation-events[Micrometer Observation Events] are usually handled as span annotations in traces.
 This instrumentation records errors listed in the GraphQL response as events.

spring-graphql/src/main/java/org/springframework/graphql/observation/DefaultExecutionRequestObservationConvention.java

@@ -16,6 +16,9 @@
 
 package org.springframework.graphql.observation;
 
+import java.util.Locale;
+
+import graphql.language.OperationDefinition;
 import io.micrometer.common.KeyValue;
 import io.micrometer.common.KeyValues;
 
@@ -42,7 +45,9 @@ public class DefaultExecutionRequestObservationConvention implements ExecutionRe
 
 	private static final KeyValue OUTCOME_INTERNAL_ERROR = KeyValue.of(ExecutionRequestLowCardinalityKeyNames.OUTCOME, "INTERNAL_ERROR");
 
-	private static final KeyValue OPERATION_QUERY = KeyValue.of(ExecutionRequestLowCardinalityKeyNames.OPERATION, "query");
+	private static final KeyValue OPERATION_NAME_QUERY = KeyValue.of(ExecutionRequestHighCardinalityKeyNames.OPERATION_NAME, "query");
+
+	private static final KeyValue OPERATION_TYPE_DEFAULT = KeyValue.of(ExecutionRequestLowCardinalityKeyNames.OPERATION_TYPE, "operation");
 
 	private final String name;
 
@@ -61,13 +66,12 @@ public String getName() {
 
 	@Override
 	public String getContextualName(ExecutionRequestObservationContext context) {
-		String operationName = (context.getExecutionInput().getOperationName() != null) ? context.getExecutionInput().getOperationName() : "query";
-		return BASE_CONTEXTUAL_NAME + operationName;
+		return BASE_CONTEXTUAL_NAME + operationType(context).getValue();
 	}
 
 	@Override
 	public KeyValues getLowCardinalityKeyValues(ExecutionRequestObservationContext context) {
-		return KeyValues.of(outcome(context), operation(context));
+		return KeyValues.of(outcome(context), operationType(context));
 	}
 
 	protected KeyValue outcome(ExecutionRequestObservationContext context) {
@@ -83,21 +87,29 @@ else if (!context.getExecutionResult().getErrors().isEmpty()) {
 		return OUTCOME_SUCCESS;
 	}
 
-	protected KeyValue operation(ExecutionRequestObservationContext context) {
-		String operationName = context.getExecutionInput().getOperationName();
-		if (operationName != null) {
-			return KeyValue.of(ExecutionRequestLowCardinalityKeyNames.OPERATION, operationName);
+	protected KeyValue operationType(ExecutionRequestObservationContext context) {
+		if (context.getExecutionContext() != null) {
+			OperationDefinition.Operation operation = context.getExecutionContext().getOperationDefinition().getOperation();
+			return KeyValue.of(ExecutionRequestLowCardinalityKeyNames.OPERATION_TYPE, operation.name().toLowerCase(Locale.ROOT));
 		}
-		return OPERATION_QUERY;
+		return OPERATION_TYPE_DEFAULT;
 	}
 
 	@Override
 	public KeyValues getHighCardinalityKeyValues(ExecutionRequestObservationContext context) {
-		return KeyValues.of(executionId(context));
+		return KeyValues.of(executionId(context), operationName(context));
 	}
 
 	protected KeyValue executionId(ExecutionRequestObservationContext context) {
 		return KeyValue.of(ExecutionRequestHighCardinalityKeyNames.EXECUTION_ID, context.getExecutionInput().getExecutionId().toString());
 	}
 
+	protected KeyValue operationName(ExecutionRequestObservationContext context) {
+		String operationName = context.getExecutionInput().getOperationName();
+		if (operationName != null) {
+			return KeyValue.of(ExecutionRequestHighCardinalityKeyNames.OPERATION_NAME, operationName);
+		}
+		return OPERATION_NAME_QUERY;
+	}
+
 }

spring-graphql/src/main/java/org/springframework/graphql/observation/ExecutionRequestObservationContext.java

@@ -18,6 +18,7 @@
 
 import graphql.ExecutionInput;
 import graphql.ExecutionResult;
+import graphql.execution.ExecutionContext;
 import io.micrometer.observation.Observation;
 
 import org.springframework.lang.Nullable;
@@ -33,6 +34,8 @@ public class ExecutionRequestObservationContext extends Observation.Context {
 
 	private final ExecutionInput executionInput;
 
+	private ExecutionContext executionContext;
+
 	@Nullable
 	private ExecutionResult executionResult;
 
@@ -48,6 +51,24 @@ public ExecutionInput getExecutionInput() {
 		return this.executionInput;
 	}
 
+	/**
+	 * Return the {@link ExecutionContext context} for the request execution.
+	 * @since 1.3.7
+	 */
+	@Nullable
+	public ExecutionContext getExecutionContext() {
+		return this.executionContext;
+	}
+
+	/**
+	 * Set the {@link ExecutionContext context} for the request execution.
+	 * @param executionContext the execution context
+	 * @since 1.3.7
+	 */
+	public void setExecutionContext(ExecutionContext executionContext) {
+		this.executionContext = executionContext;
+	}
+
 	/**
 	 * Return the {@link ExecutionResult result} for the request execution.
 	 * @since 1.1.4

spring-graphql/src/main/java/org/springframework/graphql/observation/GraphQlObservationDocumentation.java

@@ -122,18 +122,28 @@ public String asString() {
 		},
 
 		/**
-		 * GraphQL Operation name.
+		 * GraphQL {@link graphql.language.OperationDefinition.Operation Operation type}.
 		 */
-		OPERATION {
+		OPERATION_TYPE {
 			@Override
 			public String asString() {
-				return "graphql.operation";
+				return "graphql.operation.type";
 			}
 		}
 	}
 
 	public enum ExecutionRequestHighCardinalityKeyNames implements KeyName {
 
+		/**
+		 * GraphQL Operation name.
+		 */
+		OPERATION_NAME {
+			@Override
+			public String asString() {
+				return "graphql.operation.name";
+			}
+		},
+
 		/**
 		 * {@link graphql.execution.ExecutionId} of the GraphQL request.
 		 */

spring-graphql/src/main/java/org/springframework/graphql/observation/GraphQlObservationInstrumentation.java

@@ -29,6 +29,7 @@
 import graphql.execution.instrumentation.SimpleInstrumentationContext;
 import graphql.execution.instrumentation.SimplePerformantInstrumentation;
 import graphql.execution.instrumentation.parameters.InstrumentationCreateStateParameters;
+import graphql.execution.instrumentation.parameters.InstrumentationExecuteOperationParameters;
 import graphql.execution.instrumentation.parameters.InstrumentationExecutionParameters;
 import graphql.execution.instrumentation.parameters.InstrumentationFieldFetchParameters;
 import graphql.schema.DataFetcher;
@@ -144,14 +145,15 @@ public GraphQlObservationInstrumentation(ObservationRegistry observationRegistry
 
 	@Override
 	public CompletableFuture<InstrumentationState> createStateAsync(InstrumentationCreateStateParameters parameters) {
-		return CompletableFuture.completedFuture(RequestObservationInstrumentationState.INSTANCE);
+		ExecutionRequestObservationContext requestObservationContext = new ExecutionRequestObservationContext(parameters.getExecutionInput());
+		return CompletableFuture.completedFuture(new RequestObservationInstrumentationState(requestObservationContext));
 	}
 
 	@Override
 	public InstrumentationContext<ExecutionResult> beginExecution(InstrumentationExecutionParameters parameters,
 			InstrumentationState state) {
-		if (state == RequestObservationInstrumentationState.INSTANCE) {
-			ExecutionRequestObservationContext observationContext = new ExecutionRequestObservationContext(parameters.getExecutionInput());
+		if (state instanceof RequestObservationInstrumentationState observationState) {
+			ExecutionRequestObservationContext observationContext = observationState.requestObservationContext;
 			Observation requestObservation = GraphQlObservationDocumentation.EXECUTION_REQUEST.observation(this.requestObservationConvention,
 					DEFAULT_REQUEST_CONVENTION, () -> observationContext, this.observationRegistry);
 			setCurrentObservation(requestObservation, parameters.getGraphQLContext());
@@ -180,11 +182,19 @@ private static void setCurrentObservation(Observation currentObservation, GraphQ
 		graphQlContext.put(ObservationThreadLocalAccessor.KEY, currentObservation);
 	}
 
+	@Override
+	public InstrumentationContext<ExecutionResult> beginExecuteOperation(InstrumentationExecuteOperationParameters parameters, InstrumentationState state) {
+		if (state instanceof RequestObservationInstrumentationState observationState) {
+			observationState.requestObservationContext.setExecutionContext(parameters.getExecutionContext());
+		}
+		return super.beginExecuteOperation(parameters, state);
+	}
+
 	@Override
 	public DataFetcher<?> instrumentDataFetcher(DataFetcher<?> dataFetcher,
 			InstrumentationFieldFetchParameters parameters, InstrumentationState state) {
 		if (!parameters.isTrivialDataFetcher()
-				&& state == RequestObservationInstrumentationState.INSTANCE) {
+				&& state instanceof RequestObservationInstrumentationState) {
 			// skip batch loading operations, already instrumented at the dataloader level
 			if (dataFetcher instanceof SelfDescribingDataFetcher<?> selfDescribingDataFetcher
 					&& selfDescribingDataFetcher.usesDataLoader()) {
@@ -266,7 +276,11 @@ private static DataFetchingEnvironment wrapDataFetchingEnvironment(DataFetchingE
 
 	static class RequestObservationInstrumentationState implements InstrumentationState {
 
-		static final RequestObservationInstrumentationState INSTANCE = new RequestObservationInstrumentationState();
+		final ExecutionRequestObservationContext requestObservationContext;
+
+		RequestObservationInstrumentationState(ExecutionRequestObservationContext requestObservationContext) {
+			this.requestObservationContext = requestObservationContext;
+		}
 
 	}
 

spring-graphql/src/test/java/org/springframework/graphql/observation/DefaultExecutionRequestObservationConventionTests.java

@@ -22,7 +22,10 @@
 import graphql.ExecutionResult;
 import graphql.ExecutionResultImpl;
 import graphql.GraphQLError;
+import graphql.execution.ExecutionContext;
+import graphql.execution.ExecutionContextBuilder;
 import graphql.execution.ExecutionId;
+import graphql.language.OperationDefinition;
 import graphql.schema.idl.errors.QueryOperationMissingError;
 import io.micrometer.common.KeyValue;
 import org.junit.jupiter.api.Test;
@@ -61,21 +64,19 @@ void nameCanBeCustomized() {
 	void hasContextualName() {
 		ExecutionInput input = ExecutionInput.newExecutionInput().query("{ greeting }")
 				.operationName("mutation").build();
-		ExecutionRequestObservationContext context = createObservationContext(input, builder -> { });
+		ExecutionRequestObservationContext context = createObservationContext(input);
 		assertThat(this.convention.getContextualName(context)).isEqualTo("graphql mutation");
 	}
 
 	@Test
 	void hasOperationKeyValueWhenSuccessfulOutput() {
-		ExecutionRequestObservationContext context = createObservationContext(this.input, builder -> {
-		});
-		assertThat(this.convention.getLowCardinalityKeyValues(context)).contains(KeyValue.of("graphql.operation", "query"));
+		ExecutionRequestObservationContext context = createObservationContext(this.input);
+		assertThat(this.convention.getHighCardinalityKeyValues(context)).contains(KeyValue.of("graphql.operation.name", "query"));
 	}
 
 	@Test
 	void hasOutcomeKeyValueWhenSuccessfulOutput() {
-		ExecutionRequestObservationContext context = createObservationContext(this.input, builder -> {
-		});
+		ExecutionRequestObservationContext context = createObservationContext(this.input);
 		assertThat(this.convention.getLowCardinalityKeyValues(context)).contains(KeyValue.of("graphql.outcome", "SUCCESS"));
 	}
 
@@ -88,26 +89,34 @@ void hasOutcomeKeyValueWhenErrorOutput() {
 
 	@Test
 	void hasOutcomeKeyValueWhenInternalError() {
-		ExecutionRequestObservationContext context = createObservationContext(this.input, builder -> {
-		});
+		ExecutionRequestObservationContext context = createObservationContext(this.input);
 		GraphQLError graphQLError = GraphQLError.newError().errorType(ErrorType.INTERNAL_ERROR).message(ErrorType.INTERNAL_ERROR + " for [executionId]").build();
 		context.setExecutionResult(ExecutionResult.newExecutionResult().addError(graphQLError).build());
 		assertThat(this.convention.getLowCardinalityKeyValues(context)).contains(KeyValue.of("graphql.outcome", "INTERNAL_ERROR"));
 	}
 
 	@Test
 	void hasExecutionIdKeyValue() {
-		ExecutionRequestObservationContext context = createObservationContext(this.input, builder -> {
-		});
+		ExecutionRequestObservationContext context = createObservationContext(this.input);
 		assertThat(this.convention.getHighCardinalityKeyValues(context)).contains(KeyValue.of("graphql.execution.id", "42"));
 	}
 
+	private ExecutionRequestObservationContext createObservationContext(ExecutionInput executionInput) {
+		return createObservationContext(executionInput, builder -> { });
+	}
 
 	private ExecutionRequestObservationContext createObservationContext(ExecutionInput executionInput, Consumer<ExecutionResultImpl.Builder> resultConsumer) {
 		ExecutionRequestObservationContext context = new ExecutionRequestObservationContext(executionInput);
 		ExecutionResultImpl.Builder builder = ExecutionResultImpl.newExecutionResult();
 		resultConsumer.accept(builder);
 		context.setExecutionResult(builder.build());
+
+		ExecutionContext executionContext = ExecutionContextBuilder.newExecutionContextBuilder()
+				.executionId(ExecutionId.from("123_456_789"))
+				.executionInput(executionInput)
+				.operationDefinition(OperationDefinition.newOperationDefinition().operation(OperationDefinition.Operation.MUTATION).build())
+				.build();
+		context.setExecutionContext(executionContext);
 		return context;
 	}