Reuses configuration from the given propagation type; fixes gh-1846

spring-cloud · Mar 18, 2021 · fcd8536 · fcd8536
1 parent 7ad006b
commit fcd8536
Show file tree

Hide file tree

Showing 6 changed files with 219 additions and 32 deletions.
diff --git a/.../org/springframework/cloud/sleuth/autoconfig/instrument/web/SkipPatternConfiguration.java b/.../org/springframework/cloud/sleuth/autoconfig/instrument/web/SkipPatternConfiguration.java
@@ -200,6 +200,7 @@ private static String getPathSuffix(String contextPath, String actuatorBasePath)
 
 		@Bean
 		@ConditionalOnManagementPort(ManagementPortType.SAME)
+		@ConditionalOnBean(WebEndpointProperties.class)
 		SingleSkipPattern skipPatternForActuatorEndpointsSamePort(Environment environment,
 				final ServerProperties serverProperties, final WebEndpointProperties webEndpointProperties,
 				final EndpointsSupplier<ExposableWebEndpoint> endpointsSupplier) {
@@ -211,6 +212,7 @@ SingleSkipPattern skipPatternForActuatorEndpointsSamePort(Environment environmen
 		@ConditionalOnManagementPort(ManagementPortType.DIFFERENT)
 		@ConditionalOnProperty(name = "management.server.servlet.context-path", havingValue = "/",
 				matchIfMissing = true)
+		@ConditionalOnBean(WebEndpointProperties.class)
 		SingleSkipPattern skipPatternForActuatorEndpointsDifferentPort(Environment environment,
 				final WebEndpointProperties webEndpointProperties,
 				ObjectProvider<ManagementServerProperties> managementServerProperties,

diff --git a/...ringframework/cloud/sleuth/autoconfig/brave/baggage/CompositePropagationFactoryTests.java b/...ringframework/cloud/sleuth/autoconfig/brave/baggage/CompositePropagationFactoryTests.java
@@ -0,0 +1,80 @@
+/*
+ * Copyright 2013-2021 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.
+ * You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.springframework.cloud.sleuth.autoconfig.brave.baggage;
+
+import org.junit.jupiter.api.Test;
+import reactor.core.publisher.Mono;
+import reactor.util.context.ContextView;
+
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.boot.autoconfigure.ImportAutoConfiguration;
+import org.springframework.boot.test.autoconfigure.web.reactive.WebFluxTest;
+import org.springframework.cloud.sleuth.DisableWebFluxSecurity;
+import org.springframework.cloud.sleuth.TraceContext;
+import org.springframework.cloud.sleuth.autoconfig.brave.BraveAutoConfiguration;
+import org.springframework.cloud.sleuth.autoconfig.instrument.reactor.TraceReactorAutoConfiguration;
+import org.springframework.cloud.sleuth.autoconfig.instrument.web.TraceWebAutoConfiguration;
+import org.springframework.context.annotation.Import;
+import org.springframework.test.web.reactive.server.WebTestClient;
+import org.springframework.web.bind.annotation.GetMapping;
+import org.springframework.web.bind.annotation.RequestMapping;
+import org.springframework.web.bind.annotation.RestController;
+
+import static org.hamcrest.core.IsEqual.equalTo;
+
+@WebFluxTest(controllers = TracingResource.class, properties = "spring.main.web-application-type=reactive")
+@ImportAutoConfiguration({ BraveAutoConfiguration.class, TraceWebAutoConfiguration.class,
+		TraceReactorAutoConfiguration.class })
+@Import(TracingResource.class)
+@DisableWebFluxSecurity
+public class CompositePropagationFactoryTests {
+
+	@Autowired
+	TracingResource tracingResource;
+
+	@Test
+	void should_delegate_configuration_to_propagation_factory(@Autowired WebTestClient webTestClient) {
+		// issue 1846 - without the fix supportJoin is assumed to be false
+		// because it's not taken from the configuration but from not overridden
+		// methods from CompositePropagationFactorySupplier
+		String spanId = "a2fb4a1d1a96d312";
+		webTestClient.get().uri("/api/tracing/spanId").header("X-B3-TraceId", "463ac35c9f6413ad48485a3953bb6124")
+				.header("X-B3-SpanId", spanId).header("X-B3-ParentSpanId", "0020000000000001").header("X-B3-Flags", "1")
+				.exchange().expectStatus().isOk().expectBody(String.class)
+				.value(returnedSpanId -> returnedSpanId, equalTo(spanId));
+
+	}
+
+}
+
+@RestController
+@RequestMapping("/api/tracing")
+class TracingResource {
+
+	private static final Class<TraceContext> KEY = TraceContext.class;
+
+	@GetMapping("spanId")
+	public Mono<String> spanId() {
+		return Mono.deferContextual(view -> traceContext(view)).map(c -> c.spanId());
+
+	}
+
+	private Mono<TraceContext> traceContext(ContextView contextView) {
+		return Mono.justOrEmpty(contextView.get(KEY));
+	}
+
+}
diff --git a/...va/org/springframework/cloud/sleuth/brave/bridge/CompositePropagationFactorySupplier.java b/...va/org/springframework/cloud/sleuth/brave/bridge/CompositePropagationFactorySupplier.java
@@ -16,6 +16,7 @@
 
 package org.springframework.cloud.sleuth.brave.bridge;
 
+import java.util.AbstractMap;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
@@ -66,40 +67,55 @@ public Propagation.Factory get() {
 
 class CompositePropagationFactory extends Propagation.Factory implements Propagation<String> {
 
-	private final Map<PropagationType, Propagation<String>> mapping = new HashMap<>();
+	private final Map<PropagationType, Map.Entry<Propagation.Factory, Propagation<String>>> mapping = new HashMap<>();
 
 	private final List<PropagationType> types;
 
 	CompositePropagationFactory(BeanFactory beanFactory, BraveBaggageManager braveBaggageManager,
 			List<String> localFields, List<PropagationType> types) {
 		this.types = types;
-		this.mapping.put(PropagationType.AWS, AWSPropagation.FACTORY.get());
+		this.mapping.put(PropagationType.AWS,
+				new AbstractMap.SimpleEntry<>(AWSPropagation.FACTORY, AWSPropagation.FACTORY.get()));
 		// Note: Versions <2.2.3 use injectFormat(MULTI) for non-remote (ex
 		// spring-messaging)
 		// See #1643
-		this.mapping.put(PropagationType.B3,
-				B3Propagation.newFactoryBuilder().injectFormat(B3Propagation.Format.SINGLE_NO_PARENT).build().get());
-		this.mapping.put(PropagationType.W3C, new W3CPropagation(braveBaggageManager, localFields));
-		this.mapping.put(PropagationType.CUSTOM, new LazyPropagation(beanFactory.getBeanProvider(Propagation.class)));
+		Factory b3Factory = b3Factory();
+		this.mapping.put(PropagationType.B3, new AbstractMap.SimpleEntry<>(b3Factory, b3Factory.get()));
+		W3CPropagation w3CPropagation = new W3CPropagation(braveBaggageManager, localFields);
+		this.mapping.put(PropagationType.W3C, new AbstractMap.SimpleEntry<>(w3CPropagation, w3CPropagation.get()));
+		LazyPropagationFactory lazyPropagationFactory = new LazyPropagationFactory(
+				beanFactory.getBeanProvider(Factory.class));
+		this.mapping.put(PropagationType.CUSTOM,
+				new AbstractMap.SimpleEntry<>(lazyPropagationFactory, lazyPropagationFactory.get()));
+	}
+
+	private Factory b3Factory() {
+		return B3Propagation.newFactoryBuilder().injectFormat(B3Propagation.Format.SINGLE_NO_PARENT).build();
 	}
 
 	@Override
 	public List<String> keys() {
-		return this.types.stream().map(this.mapping::get).flatMap(p -> p.keys().stream()).collect(Collectors.toList());
+		return this.types.stream().map(this.mapping::get).flatMap(p -> p.getValue().keys().stream())
+				.collect(Collectors.toList());
 	}
 
 	@Override
 	public <R> TraceContext.Injector<R> injector(Setter<R, String> setter) {
 		return (traceContext, request) -> {
-			this.types.stream().map(this.mapping::get).forEach(p -> p.injector(setter).inject(traceContext, request));
+			this.types.stream().map(this.mapping::get)
+					.forEach(p -> p.getValue().injector(setter).inject(traceContext, request));
 		};
 	}
 
 	@Override
 	public <R> TraceContext.Extractor<R> extractor(Getter<R, String> getter) {
 		return request -> {
 			for (PropagationType type : this.types) {
-				Propagation<String> propagator = this.mapping.get(type);
+				Map.Entry<Factory, Propagation<String>> entry = this.mapping.get(type);
+				if (entry == null) {
+					continue;
+				}
+				Propagation<String> propagator = entry.getValue();
 				if (propagator == null || propagator == NoOpPropagation.INSTANCE) {
 					continue;
 				}
@@ -117,33 +133,112 @@ public <K> Propagation<K> create(KeyFactory<K> keyFactory) {
 		return StringPropagationAdapter.create(this, keyFactory);
 	}
 
+	@Override
+	public boolean supportsJoin() {
+		return this.types.stream().map(this.mapping::get).allMatch(e -> e.getKey().supportsJoin());
+	}
+
+	@Override
+	public boolean requires128BitTraceId() {
+		return this.types.stream().map(this.mapping::get).allMatch(e -> e.getKey().requires128BitTraceId());
+	}
+
+	@Override
+	public TraceContext decorate(TraceContext context) {
+		for (PropagationType type : this.types) {
+			Map.Entry<Factory, Propagation<String>> entry = this.mapping.get(type);
+			if (entry == null) {
+				continue;
+			}
+			TraceContext decorate = entry.getKey().decorate(context);
+			if (decorate != context) {
+				return decorate;
+			}
+		}
+		return super.decorate(context);
+	}
+
+	@SuppressWarnings("unchecked")
+	private static final class LazyPropagationFactory extends Propagation.Factory {
+
+		private final ObjectProvider<Propagation.Factory> delegate;
+
+		private volatile Propagation.Factory propagationFactory;
+
+		private LazyPropagationFactory(ObjectProvider<Propagation.Factory> delegate) {
+			this.delegate = delegate;
+		}
+
+		private Propagation.Factory propagationFactory() {
+			if (this.propagationFactory == null) {
+				this.propagationFactory = this.delegate.getIfAvailable(() -> NoOpPropagation.INSTANCE);
+			}
+			return this.propagationFactory;
+		}
+
+		@Override
+		public <K> Propagation<K> create(KeyFactory<K> keyFactory) {
+			return propagationFactory().create(keyFactory);
+		}
+
+		@Override
+		public boolean supportsJoin() {
+			return propagationFactory().supportsJoin();
+		}
+
+		@Override
+		public boolean requires128BitTraceId() {
+			return propagationFactory().requires128BitTraceId();
+		}
+
+		@Override
+		public Propagation<String> get() {
+			return new LazyPropagation(this);
+		}
+
+		@Override
+		public TraceContext decorate(TraceContext context) {
+			return propagationFactory().decorate(context);
+		}
+
+	}
+
 	@SuppressWarnings("unchecked")
 	private static final class LazyPropagation implements Propagation<String> {
 
-		private final ObjectProvider<Propagation> delegate;
+		private final LazyPropagationFactory delegate;
+
+		private volatile Propagation<String> propagation;
 
-		private LazyPropagation(ObjectProvider<Propagation> delegate) {
+		private LazyPropagation(LazyPropagationFactory delegate) {
 			this.delegate = delegate;
 		}
 
+		private Propagation<String> propagation() {
+			if (this.propagation == null) {
+				this.propagation = this.delegate.propagationFactory().get();
+			}
+			return this.propagation;
+		}
+
 		@Override
 		public List<String> keys() {
-			return this.delegate.getIfAvailable(() -> NoOpPropagation.INSTANCE).keys();
+			return propagation().keys();
 		}
 
 		@Override
 		public <R> TraceContext.Injector<R> injector(Setter<R, String> setter) {
-			return this.delegate.getIfAvailable(() -> NoOpPropagation.INSTANCE).injector(setter);
+			return propagation().injector(setter);
 		}
 
 		@Override
 		public <R> TraceContext.Extractor<R> extractor(Getter<R, String> getter) {
-			return this.delegate.getIfAvailable(() -> NoOpPropagation.INSTANCE).extractor(getter);
+			return propagation().extractor(getter);
 		}
 
 	}
 
-	private static class NoOpPropagation implements Propagation<String> {
+	private static class NoOpPropagation extends Propagation.Factory implements Propagation<String> {
 
 		static final NoOpPropagation INSTANCE = new NoOpPropagation();
 
@@ -164,6 +259,11 @@ public <R> TraceContext.Extractor<R> extractor(Getter<R, String> getter) {
 			return request -> TraceContextOrSamplingFlags.EMPTY;
 		}
 
+		@Override
+		public <K> Propagation<K> create(KeyFactory<K> keyFactory) {
+			return StringPropagationAdapter.create(this, keyFactory);
+		}
+
 	}
 
 }
diff --git a/...g/springframework/cloud/sleuth/brave/bridge/CompositePropagationFactorySupplierTests.java b/...g/springframework/cloud/sleuth/brave/bridge/CompositePropagationFactorySupplierTests.java
@@ -41,6 +41,8 @@ void should_pick_custom_registered_propagation_when_custom_mode_picked() {
 		BeanFactory beanFactory = Mockito.mock(BeanFactory.class);
 		Mockito.when(beanFactory.getBeanProvider(BraveBaggageManager.class))
 				.thenReturn(new SimpleObjectProvider(new BraveBaggageManager()));
+		Mockito.when(beanFactory.getBeanProvider(Propagation.Factory.class))
+				.thenReturn(new SimpleObjectProvider(new CustomTracePropagation()));
 		Mockito.when(beanFactory.getBeanProvider(Propagation.class))
 				.thenReturn(new SimpleObjectProvider(new CustomTracePropagation()));
 

diff --git a/...springframework/cloud/sleuth/brave/instrument/web/BraveSpanFromContextRetrieverTests.java b/...springframework/cloud/sleuth/brave/instrument/web/BraveSpanFromContextRetrieverTests.java
@@ -35,8 +35,8 @@ class BraveSpanFromContextRetrieverTests {
 
 	StrictCurrentTraceContext traceContext = StrictCurrentTraceContext.create();
 
-	Tracing tracing = Tracing.newBuilder().currentTraceContext(this.traceContext)
-			.sampler(Sampler.ALWAYS_SAMPLE).addSpanHandler(this.spans).build();
+	Tracing tracing = Tracing.newBuilder().currentTraceContext(this.traceContext).sampler(Sampler.ALWAYS_SAMPLE)
+			.addSpanHandler(this.spans).build();
 
 	brave.Tracer tracer = this.tracing.tracer();
 
@@ -60,4 +60,5 @@ void should_return_span_when_brave_trace_context_present_in_context() {
 
 		then(BraveSpan.toBrave(retriever.findSpan(Context.of(TraceContext.class, span.context())))).isEqualTo(span);
 	}
-}
+
+}