From 8b61684168be90e06b861c26205d25f61ab3a0a2 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Fri, 2 Jul 2021 17:14:05 +0200 Subject: [PATCH] Ensures that there are no issues with context setup when picking custom propagation type (#1989) fixes gh-1987 --- .../brave/BraveBaggageConfiguration.java | 7 +- .../CustomPropagationFactoryTests.java | 102 +++++++++++++++ .../CompositePropagationFactorySupplier.java | 8 +- .../brave/propagation/PropagationType.java | 3 +- ...positePropagationFactorySupplierTests.java | 120 ------------------ 5 files changed, 113 insertions(+), 127 deletions(-) create mode 100644 spring-cloud-sleuth-autoconfigure/src/test/java/org/springframework/cloud/sleuth/autoconfig/brave/baggage/CustomPropagationFactoryTests.java delete mode 100644 spring-cloud-sleuth-brave/src/test/java/org/springframework/cloud/sleuth/brave/bridge/CompositePropagationFactorySupplierTests.java diff --git a/spring-cloud-sleuth-autoconfigure/src/main/java/org/springframework/cloud/sleuth/autoconfig/brave/BraveBaggageConfiguration.java b/spring-cloud-sleuth-autoconfigure/src/main/java/org/springframework/cloud/sleuth/autoconfig/brave/BraveBaggageConfiguration.java index e307d2b3d..16d9e03df 100644 --- a/spring-cloud-sleuth-autoconfigure/src/main/java/org/springframework/cloud/sleuth/autoconfig/brave/BraveBaggageConfiguration.java +++ b/spring-cloud-sleuth-autoconfigure/src/main/java/org/springframework/cloud/sleuth/autoconfig/brave/BraveBaggageConfiguration.java @@ -105,7 +105,11 @@ class BraveBaggageConfiguration { // See #1643 @Bean @ConditionalOnMissingBean - PropagationFactorySupplier defaultPropagationFactorySupplier() { + PropagationFactorySupplier defaultPropagationFactorySupplier(SleuthPropagationProperties properties) { + if (properties.getType().contains(PropagationType.CUSTOM)) { + throw new IllegalStateException( + "Please register a bean with the following signature [extends Propagation.Factory implements Propagation] to override the default Sleuth behaviour or [implements PropagationFactorySupplier] to reuse it."); + } return () -> B3Propagation.newFactoryBuilder().injectFormat(B3Propagation.Format.SINGLE_NO_PARENT).build(); } @@ -131,7 +135,6 @@ class BraveBaggageConfiguration { @Qualifier(PROPAGATION_KEYS) List propagationKeys, SleuthBaggageProperties sleuthBaggageProperties, SleuthPropagationProperties sleuthPropagationProperties, PropagationFactorySupplier supplier, @Nullable List baggagePropagationCustomizers) { - Set localFields = redirectOldPropertyToNew(LOCAL_KEYS, localKeys, "spring.sleuth.baggage.local-fields", sleuthBaggageProperties.getLocalFields()); for (String fieldName : localFields) { diff --git a/spring-cloud-sleuth-autoconfigure/src/test/java/org/springframework/cloud/sleuth/autoconfig/brave/baggage/CustomPropagationFactoryTests.java b/spring-cloud-sleuth-autoconfigure/src/test/java/org/springframework/cloud/sleuth/autoconfig/brave/baggage/CustomPropagationFactoryTests.java new file mode 100644 index 000000000..ad08b8385 --- /dev/null +++ b/spring-cloud-sleuth-autoconfigure/src/test/java/org/springframework/cloud/sleuth/autoconfig/brave/baggage/CustomPropagationFactoryTests.java @@ -0,0 +1,102 @@ +/* + * 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 java.util.Collections; +import java.util.List; + +import brave.internal.propagation.StringPropagationAdapter; +import brave.propagation.Propagation; +import brave.propagation.TraceContext; +import brave.propagation.TraceContextOrSamplingFlags; +import org.assertj.core.api.BDDAssertions; +import org.junit.jupiter.api.Test; + +import org.springframework.boot.actuate.autoconfigure.security.servlet.ManagementWebSecurityAutoConfiguration; +import org.springframework.boot.autoconfigure.EnableAutoConfiguration; +import org.springframework.boot.autoconfigure.mongo.MongoAutoConfiguration; +import org.springframework.boot.autoconfigure.quartz.QuartzAutoConfiguration; +import org.springframework.boot.test.context.runner.ApplicationContextRunner; +import org.springframework.cloud.gateway.config.GatewayAutoConfiguration; +import org.springframework.cloud.gateway.config.GatewayClassPathWarningAutoConfiguration; +import org.springframework.cloud.gateway.config.GatewayMetricsAutoConfiguration; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; + +public class CustomPropagationFactoryTests { + + @Test + void should_fail_to_start_the_context_when_propagation_type_custom_and_no_custom_propagation_provided() { + new ApplicationContextRunner().withUserConfiguration(Config.class) + .withPropertyValues("spring.sleuth.propagation.type=custom") + .run(context -> BDDAssertions.then(context).hasFailed()); + } + + @Test + void should_start_the_context_when_propagation_type_custom_and_no_custom_propagation_provided() { + new ApplicationContextRunner().withUserConfiguration(CustomConfig.class) + .withPropertyValues("spring.sleuth.propagation.type=custom").run(context -> BDDAssertions.then(context) + .hasNotFailed().getBean(CustomConfig.CustomPropagation.class)); + } + + @Configuration(proxyBeanMethods = false) + @EnableAutoConfiguration(exclude = { GatewayClassPathWarningAutoConfiguration.class, GatewayAutoConfiguration.class, + GatewayMetricsAutoConfiguration.class, ManagementWebSecurityAutoConfiguration.class, + MongoAutoConfiguration.class, QuartzAutoConfiguration.class }) + static class Config { + + } + + @Configuration(proxyBeanMethods = false) + @EnableAutoConfiguration(exclude = { GatewayClassPathWarningAutoConfiguration.class, GatewayAutoConfiguration.class, + GatewayMetricsAutoConfiguration.class, ManagementWebSecurityAutoConfiguration.class, + MongoAutoConfiguration.class, QuartzAutoConfiguration.class }) + static class CustomConfig { + + @Bean + CustomPropagation customPropagation() { + return new CustomPropagation(); + } + + static class CustomPropagation extends Propagation.Factory implements Propagation { + + @Override + public List keys() { + return Collections.emptyList(); + } + + @Override + public TraceContext.Injector injector(Setter setter) { + return (traceContext, request) -> { + }; + } + + @Override + public TraceContext.Extractor extractor(Getter getter) { + return request -> TraceContextOrSamplingFlags.EMPTY; + } + + @Override + public Propagation create(KeyFactory keyFactory) { + return StringPropagationAdapter.create(this, keyFactory); + } + + } + + } + +} 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 269dedef2..3afdd8d3b 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 @@ -84,7 +84,7 @@ class CompositePropagationFactory extends Propagation.Factory implements Propaga 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)); + beanFactory.getBeanProvider(PropagationFactorySupplier.class)); this.mapping.put(PropagationType.CUSTOM, new AbstractMap.SimpleEntry<>(lazyPropagationFactory, lazyPropagationFactory.get())); } @@ -161,17 +161,17 @@ class CompositePropagationFactory extends Propagation.Factory implements Propaga @SuppressWarnings("unchecked") private static final class LazyPropagationFactory extends Propagation.Factory { - private final ObjectProvider delegate; + private final ObjectProvider delegate; private volatile Propagation.Factory propagationFactory; - private LazyPropagationFactory(ObjectProvider delegate) { + private LazyPropagationFactory(ObjectProvider delegate) { this.delegate = delegate; } private Propagation.Factory propagationFactory() { if (this.propagationFactory == null) { - this.propagationFactory = this.delegate.getIfAvailable(() -> NoOpPropagation.INSTANCE); + this.propagationFactory = this.delegate.getIfAvailable(() -> () -> NoOpPropagation.INSTANCE).get(); } return this.propagationFactory; } diff --git a/spring-cloud-sleuth-brave/src/main/java/org/springframework/cloud/sleuth/brave/propagation/PropagationType.java b/spring-cloud-sleuth-brave/src/main/java/org/springframework/cloud/sleuth/brave/propagation/PropagationType.java index ffc744e8c..02a13ed19 100644 --- a/spring-cloud-sleuth-brave/src/main/java/org/springframework/cloud/sleuth/brave/propagation/PropagationType.java +++ b/spring-cloud-sleuth-brave/src/main/java/org/springframework/cloud/sleuth/brave/propagation/PropagationType.java @@ -40,7 +40,8 @@ public enum PropagationType { W3C, /** - * Custom propagation type. + * Custom propagation type. If picked, requires bean registration overriding the + * default propagation mechanisms. */ CUSTOM 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 deleted file mode 100644 index 236bf90d7..000000000 --- a/spring-cloud-sleuth-brave/src/test/java/org/springframework/cloud/sleuth/brave/bridge/CompositePropagationFactorySupplierTests.java +++ /dev/null @@ -1,120 +0,0 @@ -/* - * 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.brave.bridge; - -import java.util.Collections; -import java.util.List; -import java.util.Objects; - -import brave.internal.codec.HexCodec; -import brave.internal.propagation.StringPropagationAdapter; -import brave.propagation.Propagation; -import brave.propagation.TraceContext; -import brave.propagation.TraceContextOrSamplingFlags; -import org.assertj.core.api.BDDAssertions; -import org.junit.jupiter.api.Test; -import org.mockito.Mockito; - -import org.springframework.beans.factory.BeanFactory; -import org.springframework.cloud.loadbalancer.support.SimpleObjectProvider; -import org.springframework.cloud.sleuth.brave.propagation.PropagationType; -import org.springframework.util.StringUtils; - -class CompositePropagationFactorySupplierTests { - - @Test - 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())); - - CompositePropagationFactorySupplier supplier = new CompositePropagationFactorySupplier(beanFactory, - Collections.emptyList(), Collections.singletonList(PropagationType.CUSTOM)); - - BDDAssertions.then(supplier.get().get().keys()).containsExactly(CustomTraceExtractor.CUSTOM_TRACE_HEADER); - } - -} - -class CustomTracePropagation extends Propagation.Factory implements Propagation { - - public static final List KEYS = Collections.singletonList(CustomTraceExtractor.CUSTOM_TRACE_HEADER); - - @Override - public List keys() { - return KEYS; - } - - @Override - public TraceContext.Injector injector(Setter setter) { - return (traceContext, request) -> { - String trace = traceContext.traceIdString() + ":" + traceContext.spanIdString(); - setter.put(request, CustomTraceExtractor.CUSTOM_TRACE_HEADER, trace); - }; - } - - @Override - public TraceContext.Extractor extractor(Getter getter) { - Objects.requireNonNull(getter); - return new CustomTraceExtractor<>(getter); - } - - @Override - public Propagation create(KeyFactory keyFactory) { - return StringPropagationAdapter.create(this, keyFactory); - } - -} - -class CustomTraceExtractor implements TraceContext.Extractor { - - static final String CUSTOM_TRACE_HEADER = "x-custom-trace"; - - final Propagation.Getter getter; - - CustomTraceExtractor(Propagation.Getter getter) { - this.getter = getter; - } - - @Override - @SuppressWarnings("ReturnCount") - public TraceContextOrSamplingFlags extract(R request) { - String traceString = getter.get(request, CUSTOM_TRACE_HEADER); - if (!StringUtils.hasText(traceString)) { - return TraceContextOrSamplingFlags.EMPTY; - } - String[] trace = traceString.split(":"); - if (trace.length != 2) { - return TraceContextOrSamplingFlags.EMPTY; - } - - try { - TraceContext traceContext = TraceContext.newBuilder().traceId(HexCodec.lowerHexToUnsignedLong(trace[0])) - .spanId(HexCodec.lowerHexToUnsignedLong(trace[1])).build(); - - return TraceContextOrSamplingFlags.create(traceContext); - } - catch (NumberFormatException ex) { - return TraceContextOrSamplingFlags.EMPTY; - } - } - -}