Revisit request observation context type

Prior to this commit, the request execution observation would have a
context of type `RequestReplyReceiverContext` to directly deal with
tracing propagation at the transport level. This approach doesn't work
anymore as the parent observation is not properly set on the resulting
trace, even if it is manually set in the instrumentation.

This commit revisits the request observation setup and turns its context
into a regular `Observation.Context`. Tracing propagation should be
dealt with directly at the transport level by an underlying observation.
This is the case already for Spring Framework HTTP server observations.

As a result, the `PropagationWebGraphQlInterceptor` is deprecated with
no replacement and should not be used anymore.

Fixes gh-675

Author: bclozel

spring-graphql-docs/src/docs/asciidoc/includes/observability.adoc

@@ -13,7 +13,11 @@ If your application is using Spring Boot, contributing the custom convention as
 [[observability.server.request]]
 == Server Requests instrumentation
 
-GraphQL Server Requests observations are created with the name `"graphql.request"` for Servlet and Reactive applications and above all supported transports.
+GraphQL Server Requests observations are created with the name `"graphql.request"` for traditional and Reactive applications and above all supported transports.
+This instrumentation assumes that any parent observation must be set as the current one on the GraphQL context with the well-known `"micrometer.observation"` key.
+For trace propagation across network boundaries, a separate instrumentation at the transport level must be in charge.
+In the case of HTTP, Spring Framework {spring-framework-ref-docs}/integration.html#integration.observability.http-server[has dedicated instrumentation that takes care of trace propagation].
+
 Applications need to configure the `org.springframework.graphql.observation.GraphQlObservationInstrumentation` instrumentation in their application.
 It is using the `org.springframework.graphql.observation.DefaultExecutionRequestObservationConvention` by default, backed by the `ExecutionRequestObservationContext`.
 

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2020-2022 the original author or authors.
+ * Copyright 2020-2023 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -60,7 +60,7 @@ public String getName() {
 
 	@Override
 	public String getContextualName(ExecutionRequestObservationContext context) {
-		String operationName = (context.getCarrier().getOperationName() != null) ? context.getCarrier().getOperationName() : "query";
+		String operationName = (context.getExecutionInput().getOperationName() != null) ? context.getExecutionInput().getOperationName() : "query";
 		return BASE_CONTEXTUAL_NAME + operationName;
 	}
 
@@ -70,17 +70,17 @@ public KeyValues getLowCardinalityKeyValues(ExecutionRequestObservationContext c
 	}
 
 	protected KeyValue outcome(ExecutionRequestObservationContext context) {
-		if (context.getError() != null || context.getResponse() == null) {
+		if (context.getError() != null || context.getExecutionResult() == null) {
 			return OUTCOME_INTERNAL_ERROR;
 		}
-		else if (context.getResponse().getErrors().size() > 0) {
+		else if (context.getExecutionResult().getErrors().size() > 0) {
 			return OUTCOME_REQUEST_ERROR;
 		}
 		return OUTCOME_SUCCESS;
 	}
 
 	protected KeyValue operation(ExecutionRequestObservationContext context) {
-		String operationName = context.getCarrier().getOperationName();
+		String operationName = context.getExecutionInput().getOperationName();
 		if (operationName != null) {
 			return KeyValue.of(ExecutionRequestLowCardinalityKeyNames.OPERATION, operationName);
 		}
@@ -93,7 +93,7 @@ public KeyValues getHighCardinalityKeyValues(ExecutionRequestObservationContext
 	}
 
 	protected KeyValue executionId(ExecutionRequestObservationContext context) {
-		return KeyValue.of(ExecutionRequestHighCardinalityKeyNames.EXECUTION_ID, context.getCarrier().getExecutionId().toString());
+		return KeyValue.of(ExecutionRequestHighCardinalityKeyNames.EXECUTION_ID, context.getExecutionInput().getExecutionId().toString());
 	}
 
 }

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2020-2022 the original author or authors.
+ * Copyright 2020-2023 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -16,42 +16,72 @@
 
 package org.springframework.graphql.observation;
 
-import java.util.Map;
-
 import graphql.ExecutionInput;
 import graphql.ExecutionResult;
-import io.micrometer.observation.transport.RequestReplyReceiverContext;
+import io.micrometer.observation.Observation;
+import org.springframework.lang.Nullable;
 
 /**
  * Context that holds information for metadata collection during observations
  * for {@link GraphQlObservationDocumentation#EXECUTION_REQUEST GraphQL requests}.
- * <p>This context also extends {@link RequestReplyReceiverContext} for propagating
- * tracing information from the {@link graphql.GraphQLContext}
- * or the {@link ExecutionInput#getExtensions() input extensions}.
  *
  * @author Brian Clozel
  * @since 1.1.0
  */
-public class ExecutionRequestObservationContext extends RequestReplyReceiverContext<ExecutionInput, ExecutionResult> {
-
-	public ExecutionRequestObservationContext(ExecutionInput executionInput) {
-		super(ExecutionRequestObservationContext::getContextValue);
-		setCarrier(executionInput);
-	}
-
-	/**
-	 * Read propagation field from the {@link graphql.GraphQLContext},
-	 * or the {@link ExecutionInput#getExtensions() input extensions} as a fallback.
-	 */
-	private static String getContextValue(ExecutionInput executionInput, String key) {
-		String value = executionInput.getGraphQLContext().get(key);
-		if (value == null) {
-			Map<String, Object> extensions = executionInput.getExtensions();
-			if (extensions != null) {
-				value = (String) extensions.get(key);
-			}
-		}
-		return value;
-	}
+public class ExecutionRequestObservationContext extends Observation.Context {
+
+    private final ExecutionInput executionInput;
+
+    @Nullable
+    private ExecutionResult executionResult;
+
+    public ExecutionRequestObservationContext(ExecutionInput executionInput) {
+        this.executionInput = executionInput;
+    }
+
+    /**
+     * Return the {@link ExecutionInput input} for the request execution.
+     * @since 1.1.4
+     */
+    public ExecutionInput getExecutionInput() {
+        return this.executionInput;
+    }
+
+    /**
+     * Return the {@link ExecutionInput input} for the request execution.
+     * @deprecated since 1.1.4 in favor of {@link #getExecutionInput()}
+     */
+    @Deprecated(since = "1.1.4", forRemoval = true)
+    public ExecutionInput getCarrier() {
+        return this.executionInput;
+    }
+
+    /**
+     * Return the {@link ExecutionResult result} for the request execution.
+     * @since 1.1.4
+     */
+    @Nullable
+    public ExecutionResult getExecutionResult() {
+        return this.executionResult;
+    }
+
+    /**
+     * Set the {@link ExecutionResult result} for the request execution.
+     * @param executionResult the execution result
+     * @since 1.1.4
+     */
+    public void setExecutionResult(ExecutionResult executionResult) {
+        this.executionResult = executionResult;
+    }
+
+    /**
+     * Return the {@link ExecutionResult result} for the request execution.
+     * @deprecated since 1.1.4 in favor of {@link #getExecutionResult()}
+     */
+    @Nullable
+    @Deprecated(since = "1.1.4", forRemoval = true)
+    public ExecutionResult getResponse() {
+        return this.executionResult;
+    }
 
 }

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

@@ -110,7 +110,7 @@ public InstrumentationContext<ExecutionResult> beginExecution(InstrumentationExe
 			return new SimpleInstrumentationContext<>() {
 				@Override
 				public void onCompleted(ExecutionResult result, Throwable exc) {
-					observationContext.setResponse(result);
+					observationContext.setExecutionResult(result);
 					if (exc != null) {
 						requestObservation.error(exc);
 					}

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

@@ -32,7 +32,9 @@
  *
  * @author Brian Clozel
  * @since 1.1.1
+ * @deprecated since 1.1.4 with no replacement.
  */
+@Deprecated(since = "1.1.4", forRemoval = true)
 public class PropagationWebGraphQlInterceptor implements WebGraphQlInterceptor {
 
 	private final Propagator propagator;

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2020-2022 the original author or authors.
+ * Copyright 2020-2023 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -102,7 +102,7 @@ private ExecutionRequestObservationContext createObservationContext(ExecutionInp
 		ExecutionRequestObservationContext context = new ExecutionRequestObservationContext(executionInput);
 		ExecutionResultImpl.Builder builder = ExecutionResultImpl.newExecutionResult();
 		resultConsumer.accept(builder);
-		context.setResponse(builder.build());
+		context.setExecutionResult(builder.build());
 		return context;
 	}
 

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

@@ -16,23 +16,12 @@
 
 package org.springframework.graphql.observation;
 
-import java.util.List;
-import java.util.concurrent.CompletableFuture;
-
 import graphql.GraphqlErrorBuilder;
 import io.micrometer.observation.Observation;
 import io.micrometer.observation.ObservationRegistry;
+import io.micrometer.observation.contextpropagation.ObservationThreadLocalAccessor;
 import io.micrometer.observation.tck.TestObservationRegistry;
 import io.micrometer.observation.tck.TestObservationRegistryAssert;
-import io.micrometer.observation.transport.ReceiverContext;
-import io.micrometer.tracing.Span;
-import io.micrometer.tracing.TraceContext;
-import io.micrometer.tracing.handler.PropagatingReceiverTracingObservationHandler;
-import io.micrometer.tracing.handler.TracingObservationHandler;
-import io.micrometer.tracing.propagation.Propagator;
-import io.micrometer.tracing.test.simple.SimpleSpanBuilder;
-import io.micrometer.tracing.test.simple.SimpleTracer;
-import io.micrometer.tracing.test.simple.TracerAssert;
 import org.junit.jupiter.api.Test;
 import reactor.core.publisher.Mono;
 
@@ -45,7 +34,7 @@
 import org.springframework.graphql.execution.DataFetcherExceptionResolver;
 import org.springframework.graphql.execution.ErrorType;
 
-import static org.assertj.core.api.Assertions.assertThat;
+import java.util.concurrent.CompletableFuture;
 
 /**
  * Tests for {@link GraphQlObservationInstrumentation}.
@@ -134,7 +123,8 @@ void instrumentMultipleDataFetcherOperations() {
 				.that()
 				.hasLowCardinalityKeyValue("graphql.outcome", "SUCCESS")
 				.hasLowCardinalityKeyValue("graphql.field.name", "bookById")
-				.hasHighCardinalityKeyValue("graphql.field.path", "/bookById");
+				.hasHighCardinalityKeyValue("graphql.field.path", "/bookById")
+				.hasParentObservationContextMatching(context -> context instanceof ExecutionRequestObservationContext);
 
 		TestObservationRegistryAssert.assertThat(this.observationRegistry)
 				.hasAnObservationWithAKeyValue("graphql.field.name", "author")
@@ -213,7 +203,7 @@ void setIncomingObservationAsParent() {
 		ExecutionGraphQlRequest graphQlRequest = TestExecutionRequest.forDocument(document);
 		Observation incoming = Observation.start("incoming", ObservationRegistry.create());
 		graphQlRequest.configureExecutionInput((input, builder) ->
-				builder.graphQLContext(contextBuilder -> contextBuilder.of("micrometer.observation", incoming)).build());
+				builder.graphQLContext(contextBuilder -> contextBuilder.of(ObservationThreadLocalAccessor.KEY, incoming)).build());
 		Mono<ExecutionGraphQlResponse> responseMono = graphQlSetup
 				.queryFetcher("bookById", env -> BookSource.getBookWithoutAuthor(1L))
 				.toGraphQlService()
@@ -225,65 +215,4 @@ void setIncomingObservationAsParent() {
 		incoming.stop();
 	}
 
-	@Test
-	void inboundTracingInformationIsPropagated() {
-		SimpleTracer simpleTracer = new SimpleTracer();
-		String traceId = "traceId";
-		TracingObservationHandler<ReceiverContext> tracingHandler = new PropagatingReceiverTracingObservationHandler<>(simpleTracer, new TestPropagator(simpleTracer, traceId));
-		this.observationRegistry.observationConfig().observationHandler(tracingHandler);
-		String document = """
-				{
-					bookById(id: 1) {
-						name
-					}
-				}
-				""";
-		ExecutionGraphQlRequest executionRequest = TestExecutionRequest.forDocument(document);
-		executionRequest.configureExecutionInput((input, builder) ->
-				builder.graphQLContext(context -> context.of(TestPropagator.TRACING_HEADER_NAME, traceId)).build());
-		Mono<ExecutionGraphQlResponse> responseMono = graphQlSetup
-				.queryFetcher("bookById", env -> BookSource.getBookWithoutAuthor(1L))
-				.toGraphQlService()
-				.execute(executionRequest);
-		ResponseHelper response = ResponseHelper.forResponse(responseMono);
-
-		TracerAssert.assertThat(simpleTracer)
-				.onlySpan()
-				.hasNameEqualTo("graphql query")
-				.hasKindEqualTo(Span.Kind.SERVER)
-				.hasTag("graphql.operation", "query")
-				.hasTag("graphql.outcome", "SUCCESS")
-				.hasTagWithKey("graphql.execution.id");
-	}
-
-	static class TestPropagator implements Propagator {
-
-		public static String TRACING_HEADER_NAME = "X-Test-Tracing";
-
-		private final SimpleTracer tracer;
-
-		private final String traceId;
-
-		TestPropagator(SimpleTracer tracer, String traceId) {
-			this.tracer = tracer;
-			this.traceId = traceId;
-		}
-
-		@Override
-		public List<String> fields() {
-			return List.of(TRACING_HEADER_NAME);
-		}
-
-		@Override
-		public <C> void inject(TraceContext context, C carrier, Setter<C> setter) {
-			setter.set(carrier, TRACING_HEADER_NAME, "traceId");
-		}
-
-		@Override
-		public <C> Span.Builder extract(C carrier, Getter<C> getter) {
-			String foo = getter.get(carrier, TRACING_HEADER_NAME);
-			assertThat(foo).isEqualTo(this.traceId);
-			return new SimpleSpanBuilder(this.tracer);
-		}
-	}
 }

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

@@ -47,6 +47,7 @@
  *
  * @author Brian Clozel
  */
+@SuppressWarnings("removal")
 class PropagationWebGraphQlInterceptorTests {
 
 	PropagationWebGraphQlInterceptor interceptor = new PropagationWebGraphQlInterceptor(new TestPropagator());