From fcd85362dd5d38f8b44c9ddfdfdb8820fb2bb849 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Thu, 18 Mar 2021 17:25:47 +0100 Subject: [PATCH] Reuses configuration from the given propagation type; fixes gh-1846 --- .../web/SkipPatternConfiguration.java | 2 + .../CompositePropagationFactoryTests.java | 80 ++++++++++ .../CompositePropagationFactorySupplier.java | 140 +++++++++++++++--- ...positePropagationFactorySupplierTests.java | 2 + .../BraveSpanFromContextRetrieverTests.java | 7 +- .../MessagingApplicationTests.java | 30 ++-- 6 files changed, 224 insertions(+), 37 deletions(-) create mode 100644 spring-cloud-sleuth-autoconfigure/src/test/java/org/springframework/cloud/sleuth/autoconfig/brave/baggage/CompositePropagationFactoryTests.java diff --git a/spring-cloud-sleuth-autoconfigure/src/main/java/org/springframework/cloud/sleuth/autoconfig/instrument/web/SkipPatternConfiguration.java b/spring-cloud-sleuth-autoconfigure/src/main/java/org/springframework/cloud/sleuth/autoconfig/instrument/web/SkipPatternConfiguration.java index f910c767c..49dc59cf6 100644 --- a/spring-cloud-sleuth-autoconfigure/src/main/java/org/springframework/cloud/sleuth/autoconfig/instrument/web/SkipPatternConfiguration.java +++ b/spring-cloud-sleuth-autoconfigure/src/main/java/org/springframework/cloud/sleuth/autoconfig/instrument/web/SkipPatternConfiguration.java @@ -200,6 +200,7 @@ class SkipPatternConfiguration { @Bean @ConditionalOnManagementPort(ManagementPortType.SAME) + @ConditionalOnBean(WebEndpointProperties.class) SingleSkipPattern skipPatternForActuatorEndpointsSamePort(Environment environment, final ServerProperties serverProperties, final WebEndpointProperties webEndpointProperties, final EndpointsSupplier endpointsSupplier) { @@ -211,6 +212,7 @@ class SkipPatternConfiguration { @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, diff --git a/spring-cloud-sleuth-autoconfigure/src/test/java/org/springframework/cloud/sleuth/autoconfig/brave/baggage/CompositePropagationFactoryTests.java b/spring-cloud-sleuth-autoconfigure/src/test/java/org/springframework/cloud/sleuth/autoconfig/brave/baggage/CompositePropagationFactoryTests.java new file mode 100644 index 000000000..08076e397 --- /dev/null +++ b/spring-cloud-sleuth-autoconfigure/src/test/java/org/springframework/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 KEY = TraceContext.class; + + @GetMapping("spanId") + public Mono spanId() { + return Mono.deferContextual(view -> traceContext(view)).map(c -> c.spanId()); + + } + + private Mono traceContext(ContextView contextView) { + return Mono.justOrEmpty(contextView.get(KEY)); + } + +} diff --git a/spring-cloud-sleuth-brave/src/main/java/org/springframework/cloud/sleuth/brave/bridge/CompositePropagationFactorySupplier.java b/spring-cloud-sleuth-brave/src/main/java/org/springframework/cloud/sleuth/brave/bridge/CompositePropagationFactorySupplier.java index 167ccce25..269dedef2 100644 --- a/spring-cloud-sleuth-brave/src/main/java/org/springframework/cloud/sleuth/brave/bridge/CompositePropagationFactorySupplier.java +++ b/spring-cloud-sleuth-brave/src/main/java/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,32 +67,43 @@ public class CompositePropagationFactorySupplier implements PropagationFactorySu class CompositePropagationFactory extends Propagation.Factory implements Propagation { - private final Map> mapping = new HashMap<>(); + private final Map>> mapping = new HashMap<>(); private final List types; CompositePropagationFactory(BeanFactory beanFactory, BraveBaggageManager braveBaggageManager, List localFields, List 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 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 TraceContext.Injector injector(Setter 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)); }; } @@ -99,7 +111,11 @@ class CompositePropagationFactory extends Propagation.Factory implements Propaga public TraceContext.Extractor extractor(Getter getter) { return request -> { for (PropagationType type : this.types) { - Propagation propagator = this.mapping.get(type); + Map.Entry> entry = this.mapping.get(type); + if (entry == null) { + continue; + } + Propagation propagator = entry.getValue(); if (propagator == null || propagator == NoOpPropagation.INSTANCE) { continue; } @@ -117,33 +133,112 @@ class CompositePropagationFactory extends Propagation.Factory implements Propaga 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> 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 LazyPropagation implements Propagation { + private static final class LazyPropagationFactory extends Propagation.Factory { - private final ObjectProvider delegate; + private final ObjectProvider delegate; - private LazyPropagation(ObjectProvider delegate) { + private volatile Propagation.Factory propagationFactory; + + private LazyPropagationFactory(ObjectProvider delegate) { this.delegate = delegate; } - @Override - public List keys() { - return this.delegate.getIfAvailable(() -> NoOpPropagation.INSTANCE).keys(); + private Propagation.Factory propagationFactory() { + if (this.propagationFactory == null) { + this.propagationFactory = this.delegate.getIfAvailable(() -> NoOpPropagation.INSTANCE); + } + return this.propagationFactory; } @Override - public TraceContext.Injector injector(Setter setter) { - return this.delegate.getIfAvailable(() -> NoOpPropagation.INSTANCE).injector(setter); + public Propagation create(KeyFactory keyFactory) { + return propagationFactory().create(keyFactory); } @Override - public TraceContext.Extractor extractor(Getter getter) { - return this.delegate.getIfAvailable(() -> NoOpPropagation.INSTANCE).extractor(getter); + public boolean supportsJoin() { + return propagationFactory().supportsJoin(); + } + + @Override + public boolean requires128BitTraceId() { + return propagationFactory().requires128BitTraceId(); + } + + @Override + public Propagation get() { + return new LazyPropagation(this); + } + + @Override + public TraceContext decorate(TraceContext context) { + return propagationFactory().decorate(context); } } - private static class NoOpPropagation implements Propagation { + @SuppressWarnings("unchecked") + private static final class LazyPropagation implements Propagation { + + private final LazyPropagationFactory delegate; + + private volatile Propagation propagation; + + private LazyPropagation(LazyPropagationFactory delegate) { + this.delegate = delegate; + } + + private Propagation propagation() { + if (this.propagation == null) { + this.propagation = this.delegate.propagationFactory().get(); + } + return this.propagation; + } + + @Override + public List keys() { + return propagation().keys(); + } + + @Override + public TraceContext.Injector injector(Setter setter) { + return propagation().injector(setter); + } + + @Override + public TraceContext.Extractor extractor(Getter getter) { + return propagation().extractor(getter); + } + + } + + private static class NoOpPropagation extends Propagation.Factory implements Propagation { static final NoOpPropagation INSTANCE = new NoOpPropagation(); @@ -164,6 +259,11 @@ class CompositePropagationFactory extends Propagation.Factory implements Propaga return request -> TraceContextOrSamplingFlags.EMPTY; } + @Override + public Propagation create(KeyFactory keyFactory) { + return StringPropagationAdapter.create(this, keyFactory); + } + } } diff --git a/spring-cloud-sleuth-brave/src/test/java/org/springframework/cloud/sleuth/brave/bridge/CompositePropagationFactorySupplierTests.java b/spring-cloud-sleuth-brave/src/test/java/org/springframework/cloud/sleuth/brave/bridge/CompositePropagationFactorySupplierTests.java index 1884a69a1..236bf90d7 100644 --- a/spring-cloud-sleuth-brave/src/test/java/org/springframework/cloud/sleuth/brave/bridge/CompositePropagationFactorySupplierTests.java +++ b/spring-cloud-sleuth-brave/src/test/java/org/springframework/cloud/sleuth/brave/bridge/CompositePropagationFactorySupplierTests.java @@ -41,6 +41,8 @@ class CompositePropagationFactorySupplierTests { 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/spring-cloud-sleuth-brave/src/test/java/org/springframework/cloud/sleuth/brave/instrument/web/BraveSpanFromContextRetrieverTests.java b/spring-cloud-sleuth-brave/src/test/java/org/springframework/cloud/sleuth/brave/instrument/web/BraveSpanFromContextRetrieverTests.java index c8bf0265d..5a5eea915 100644 --- a/spring-cloud-sleuth-brave/src/test/java/org/springframework/cloud/sleuth/brave/instrument/web/BraveSpanFromContextRetrieverTests.java +++ b/spring-cloud-sleuth-brave/src/test/java/org/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 @@ class BraveSpanFromContextRetrieverTests { then(BraveSpan.toBrave(retriever.findSpan(Context.of(TraceContext.class, span.context())))).isEqualTo(span); } -} \ No newline at end of file + +} diff --git a/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-messaging/src/test/java/integration/MessagingApplicationTests.java b/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-messaging/src/test/java/integration/MessagingApplicationTests.java index bbe5cc46c..15b06e31a 100644 --- a/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-messaging/src/test/java/integration/MessagingApplicationTests.java +++ b/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-messaging/src/test/java/integration/MessagingApplicationTests.java @@ -16,6 +16,7 @@ package integration; +import java.util.ArrayList; import java.util.List; import java.util.Optional; import java.util.stream.Collectors; @@ -107,16 +108,19 @@ public class MessagingApplicationTests extends AbstractIntegrationTest { } private void thenThereIsAtLeastOneTagWithKey(String key) { - then(this.testSpanHandler.spans.stream().map(MutableSpan::tags).flatMap(m -> m.keySet().stream()) - .anyMatch(b -> b.equals(key))).isTrue(); + then(spans().stream().map(MutableSpan::tags).flatMap(m -> m.keySet().stream()).anyMatch(b -> b.equals(key))) + .isTrue(); + } + + private List spans() { + return new ArrayList<>(this.testSpanHandler.spans); } private void thenAllSpansHaveTraceIdEqualTo(long traceId) { String traceIdHex = Long.toHexString(traceId); - log.info("Stored spans: [\n" - + this.testSpanHandler.spans.stream().map(MutableSpan::toString).collect(Collectors.joining("\n")) + log.info("Stored spans: [\n" + spans().stream().map(MutableSpan::toString).collect(Collectors.joining("\n")) + "\n]"); - then(this.testSpanHandler.spans.stream().filter(span -> !span.traceId().equals(SpanUtil.idToHex(traceId))) + then(spans().stream().filter(span -> !span.traceId().equals(SpanUtil.idToHex(traceId))) .collect(Collectors.toList())).describedAs("All spans have same trace id [" + traceIdHex + "]") .isEmpty(); } @@ -130,8 +134,8 @@ public class MessagingApplicationTests extends AbstractIntegrationTest { // "http:/parent/" -> "message:messages" -> "http:/foo" (CS + CR) -> "http:/foo" // (SS) thenAllSpansArePresent(firstHttpSpan, eventSpans, lastHttpSpansParent, eventSentSpan, producerSpan); - List spans = this.testSpanHandler.spans; - then(spans).as("There were 7 spans").hasSize(7); + List spans = spans(); + then(spans).as("There were 6 spans").hasSize(6); log.info("Checking the parent child structure"); List> parentChild = spans.stream().filter(span -> span.parentId() != null).map(span -> { Optional any = spans.stream().filter(span1 -> span1.id().equals(span.parentId())).findAny(); @@ -146,21 +150,20 @@ public class MessagingApplicationTests extends AbstractIntegrationTest { } private Optional findLastHttpSpansParent() { - return this.testSpanHandler.spans.stream().filter(span -> "GET /".equals(span.name()) && span.kind() != null) - .findFirst(); + return spans().stream().filter(span -> "GET /".equals(span.name()) && span.kind() != null).findFirst(); } private Optional findSpanWithKind(Span.Kind kind) { - return this.testSpanHandler.spans.stream().filter(span -> kind.equals(span.kind())).findFirst(); + return spans().stream().filter(span -> kind.equals(span.kind())).findFirst(); } private List findAllEventRelatedSpans() { - return this.testSpanHandler.spans.stream().filter(span -> "send".equals(span.name()) && span.parentId() != null) + return spans().stream().filter(span -> "send".equals(span.name()) && span.parentId() != null) .collect(Collectors.toList()); } private Optional findFirstHttpRequestSpan() { - return this.testSpanHandler.spans.stream() + return spans().stream() // home is the name of the method .filter(span -> span.tags().values().stream().anyMatch("home"::equals)).findFirst(); } @@ -174,8 +177,7 @@ public class MessagingApplicationTests extends AbstractIntegrationTest { log.info("Event sent span " + eventSentSpan); log.info("Event received span " + eventReceivedSpan); log.info("Last http span " + lastHttpSpan); - log.info("All found spans \n" - + this.testSpanHandler.spans.stream().map(MutableSpan::toString).collect(Collectors.joining("\n"))); + log.info("All found spans \n" + spans().stream().map(MutableSpan::toString).collect(Collectors.joining("\n"))); then(firstHttpSpan.isPresent()).isTrue(); then(eventSpans).isNotEmpty(); then(eventSentSpan.isPresent()).isTrue();