From f3f8610539369e03992e1fd5604cb51ae334ba2d Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Fri, 16 Jun 2023 10:17:08 -0700 Subject: [PATCH 1/3] Polish --- .../hazelcast/hazelcast-client-instance.xml | 4 --- .../ConfigurationPropertiesBinder.java | 31 ++++++++++++------- 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/spring-boot-project/spring-boot-autoconfigure/src/test/resources/org/springframework/boot/autoconfigure/hazelcast/hazelcast-client-instance.xml b/spring-boot-project/spring-boot-autoconfigure/src/test/resources/org/springframework/boot/autoconfigure/hazelcast/hazelcast-client-instance.xml index 750a658d79..5497f784de 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/test/resources/org/springframework/boot/autoconfigure/hazelcast/hazelcast-client-instance.xml +++ b/spring-boot-project/spring-boot-autoconfigure/src/test/resources/org/springframework/boot/autoconfigure/hazelcast/hazelcast-client-instance.xml @@ -2,19 +2,15 @@ - spring-boot - 60000 -
${address}
 - diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesBinder.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesBinder.java index d74bb84fe4..6111d8e9ed 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesBinder.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesBinder.java @@ -137,7 +137,6 @@ class ConfigurationPropertiesBinder { : new IgnoreTopLevelConverterNotFoundBindHandler(); } - @SuppressWarnings("unchecked") private List getValidators(Bindable target) { List validators = new ArrayList<>(3); if (this.configurationPropertiesValidator != null) { @@ -146,15 +145,23 @@ class ConfigurationPropertiesBinder { if (this.jsr303Present && target.getAnnotation(Validated.class) != null) { validators.add(getJsr303Validator()); } + Validator selfValidator = getSelfValidator(target); + if (selfValidator != null) { + validators.add(selfValidator); + } + return validators; + } + + private Validator getSelfValidator(Bindable target) { if (target.getValue() != null) { - if (target.getValue().get() instanceof Validator) { - validators.add((Validator) target.getValue().get()); - } + Object value = target.getValue().get(); + return (value instanceof Validator) ? (Validator) value : null; } - else if (Validator.class.isAssignableFrom(target.getType().resolve())) { - validators.add(new SelfValidatingConstructorBoundBindableValidator((Bindable) target)); + Class type = target.getType().resolve(); + if (Validator.class.isAssignableFrom(type)) { + return new SelfValidatingConstructorBoundBindableValidator(type); } - return validators; + return null; } private Validator getJsr303Validator() { @@ -271,15 +278,15 @@ class ConfigurationPropertiesBinder { */ static class SelfValidatingConstructorBoundBindableValidator implements Validator { - private final Bindable bindable; + private final Class type; - SelfValidatingConstructorBoundBindableValidator(Bindable bindable) { - this.bindable = bindable; + SelfValidatingConstructorBoundBindableValidator(Class type) { + this.type = type; } @Override - public boolean supports(Class clazz) { - return clazz.isAssignableFrom(this.bindable.getType().resolve()); + public boolean supports(Class candidate) { + return candidate.isAssignableFrom(this.type); } @Override From 9b72cc41abbe8a61ddffe1d63a51f361919c8753 Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Fri, 16 Jun 2023 12:52:49 -0700 Subject: [PATCH 2/3] Polish 'Add support for multiple tracing propagation formats' See gh-35611 --- .../tracing/BraveAutoConfiguration.java | 10 +- .../tracing/CompositePropagationFactory.java | 259 ++++++++++-------- .../tracing/CompositeTextMapPropagator.java | 165 +++++------ .../OpenTelemetryAutoConfiguration.java | 7 +- .../tracing/TracingProperties.java | 10 +- .../tracing/BraveAutoConfigurationTests.java | 11 +- .../CompositePropagationFactoryTests.java | 113 ++++---- .../CompositeTextMapPropagatorTests.java | 12 +- .../OpenTelemetryAutoConfigurationTests.java | 15 +- 9 files changed, 305 insertions(+), 297 deletions(-) diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/BraveAutoConfiguration.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/BraveAutoConfiguration.java index 2b1c6ddbcb..43006e4e1f 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/BraveAutoConfiguration.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/BraveAutoConfiguration.java @@ -167,8 +167,7 @@ public class BraveAutoConfiguration { @Bean @ConditionalOnMissingBean Factory propagationFactory(TracingProperties properties) { - return CompositePropagationFactory.create(properties.getPropagation().getEffectiveProducedTypes(), - properties.getPropagation().getEffectiveConsumedTypes()); + return CompositePropagationFactory.create(properties.getPropagation(), null); } } @@ -187,10 +186,9 @@ public class BraveAutoConfiguration { @ConditionalOnMissingBean BaggagePropagation.FactoryBuilder propagationFactoryBuilder( ObjectProvider baggagePropagationCustomizers) { - Factory delegate = CompositePropagationFactory.create(BRAVE_BAGGAGE_MANAGER, - this.tracingProperties.getPropagation().getEffectiveProducedTypes(), - this.tracingProperties.getPropagation().getEffectiveConsumedTypes()); - FactoryBuilder builder = BaggagePropagation.newFactoryBuilder(delegate); + CompositePropagationFactory factory = CompositePropagationFactory + .create(this.tracingProperties.getPropagation(), BRAVE_BAGGAGE_MANAGER); + FactoryBuilder builder = BaggagePropagation.newFactoryBuilder(factory); baggagePropagationCustomizers.orderedStream().forEach((customizer) -> customizer.customize(builder)); return builder; } diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/CompositePropagationFactory.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/CompositePropagationFactory.java index 9c25cd1963..c648f9adcf 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/CompositePropagationFactory.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/CompositePropagationFactory.java @@ -19,183 +19,206 @@ package org.springframework.boot.actuate.autoconfigure.tracing; import java.util.Collection; import java.util.Collections; import java.util.List; +import java.util.function.Predicate; import java.util.stream.Stream; import brave.internal.propagation.StringPropagationAdapter; import brave.propagation.B3Propagation; import brave.propagation.Propagation; +import brave.propagation.Propagation.Factory; import brave.propagation.TraceContext; import brave.propagation.TraceContextOrSamplingFlags; +import graphql.com.google.common.collect.Streams; import io.micrometer.tracing.BaggageManager; import io.micrometer.tracing.brave.bridge.W3CPropagation; +import org.springframework.boot.actuate.autoconfigure.tracing.TracingProperties.Propagation.PropagationType; + /** - * {@link Factory} which supports multiple tracing formats. It is able to configure - * different formats for injecting and for extracting. + * {@link brave.propagation.Propagation.Factory Propagation factory} which supports + * multiple tracing formats. It is able to configure different formats for injecting and + * for extracting. * * @author Marcin Grzejszczak * @author Moritz Halbritter + * @author Phillip Webb */ -class CompositePropagationFactory extends Propagation.Factory implements Propagation { - - private final Collection injectorFactories; - - private final Collection extractorFactories; - - private final List> injectors; +class CompositePropagationFactory extends Propagation.Factory { - private final List> extractors; + private final PropagationFactories injectors; - private final boolean supportsJoin; + private final PropagationFactories extractors; - private final boolean requires128BitTraceId; - - private final List keys; + private final CompositePropagation propagation; CompositePropagationFactory(Collection injectorFactories, Collection extractorFactories) { - this.injectorFactories = injectorFactories; - this.extractorFactories = extractorFactories; - this.injectors = this.injectorFactories.stream().map(Factory::get).toList(); - this.extractors = this.extractorFactories.stream().map(Factory::get).toList(); - this.supportsJoin = Stream.concat(this.injectorFactories.stream(), this.extractorFactories.stream()) - .allMatch(Factory::supportsJoin); - this.requires128BitTraceId = Stream.concat(this.injectorFactories.stream(), this.extractorFactories.stream()) - .anyMatch(Factory::requires128BitTraceId); - this.keys = Stream.concat(this.injectors.stream(), this.extractors.stream()) - .flatMap((entry) -> entry.keys().stream()) - .distinct() - .toList(); - } - - Collection getInjectorFactories() { - return this.injectorFactories; + this.injectors = new PropagationFactories(injectorFactories); + this.extractors = new PropagationFactories(extractorFactories); + this.propagation = new CompositePropagation(this.injectors, this.extractors); } - @Override - public List keys() { - return this.keys; + Stream getInjectors() { + return this.injectors.stream(); } @Override - public TraceContext.Injector injector(Setter setter) { - return (traceContext, request) -> { - for (Propagation injector : this.injectors) { - injector.injector(setter).inject(traceContext, request); - } - }; + public boolean supportsJoin() { + return this.injectors.supportsJoin() && this.extractors.supportsJoin(); } @Override - public TraceContext.Extractor extractor(Getter getter) { - return (request) -> { - for (Propagation extractor : this.extractors) { - TraceContextOrSamplingFlags extract = extractor.extractor(getter).extract(request); - if (extract != TraceContextOrSamplingFlags.EMPTY) { - return extract; - } - } - return TraceContextOrSamplingFlags.EMPTY; - }; + public boolean requires128BitTraceId() { + return this.injectors.requires128BitTraceId() || this.extractors.requires128BitTraceId(); } @Override @SuppressWarnings("deprecation") - public Propagation create(KeyFactory keyFactory) { - return StringPropagationAdapter.create(this, keyFactory); - } - - @Override - public boolean supportsJoin() { - return this.supportsJoin; - } - - @Override - public boolean requires128BitTraceId() { - return this.requires128BitTraceId; + public Propagation create(Propagation.KeyFactory keyFactory) { + return StringPropagationAdapter.create(this.propagation, keyFactory); } @Override public TraceContext decorate(TraceContext context) { - for (Factory injectorFactory : this.injectorFactories) { - TraceContext decorated = injectorFactory.decorate(context); - if (decorated != context) { - return decorated; - } - } - for (Factory extractorFactory : this.extractorFactories) { - TraceContext decorated = extractorFactory.decorate(context); - if (decorated != context) { - return decorated; - } - } - return super.decorate(context); + return Streams.concat(this.injectors.stream(), this.extractors.stream()) + .map((factory) -> factory.decorate(context)) + .filter((decorated) -> decorated != context) + .findFirst() + .orElse(context); } /** * Creates a new {@link CompositePropagationFactory}, which uses the given * {@code injectionTypes} for injection and {@code extractionTypes} for extraction. + * @param properties the propagation properties * @param baggageManager the baggage manager to use, or {@code null} - * @param injectionTypes the propagation types for injection - * @param extractionTypes the propagation types for extraction * @return the {@link CompositePropagationFactory} */ - static CompositePropagationFactory create(BaggageManager baggageManager, - Collection injectionTypes, - Collection extractionTypes) { - List injectors = injectionTypes.stream() - .map((injection) -> factoryForType(baggageManager, injection)) - .toList(); - List extractors = extractionTypes.stream() - .map((extraction) -> factoryForType(baggageManager, extraction)) - .toList(); + static CompositePropagationFactory create(TracingProperties.Propagation properties, BaggageManager baggageManager) { + PropagationFactoryMapper mapper = new PropagationFactoryMapper(baggageManager); + List injectors = properties.getEffectiveProducedTypes().stream().map(mapper::map).toList(); + List extractors = properties.getEffectiveConsumedTypes().stream().map(mapper::map).toList(); return new CompositePropagationFactory(injectors, extractors); } /** - * Creates a new {@link CompositePropagationFactory}, which uses the given - * {@code injectionTypes} for injection and {@code extractionTypes} for extraction. - * @param injectionTypes the propagation types for injection - * @param extractionTypes the propagation types for extraction - * @return the {@link CompositePropagationFactory} + * Mapper used to create a {@link brave.propagation.Propagation.Factory Propagation + * factory} from a {@link PropagationType}. */ - static CompositePropagationFactory create(Collection injectionTypes, - Collection extractionTypes) { - return create(null, injectionTypes, extractionTypes); - } + private static class PropagationFactoryMapper { - private static Factory factoryForType(BaggageManager baggageManager, - TracingProperties.Propagation.PropagationType type) { - return switch (type) { - case B3 -> b3Single(); - case B3_MULTI -> b3Multi(); - case W3C -> w3c(baggageManager); - }; - } + private final BaggageManager baggageManager; + + PropagationFactoryMapper(BaggageManager baggageManager) { + this.baggageManager = baggageManager; + } + + Propagation.Factory map(PropagationType type) { + return switch (type) { + case B3 -> b3Single(); + case B3_MULTI -> b3Multi(); + case W3C -> w3c(); + }; + } + + /** + * Creates a new B3 propagation factory using a single B3 header. + * @return the B3 propagation factory + */ + private Propagation.Factory b3Single() { + return B3Propagation.newFactoryBuilder().injectFormat(B3Propagation.Format.SINGLE_NO_PARENT).build(); + } + + /** + * Creates a new B3 propagation factory using multiple B3 headers. + * @return the B3 propagation factory + */ + private Propagation.Factory b3Multi() { + return B3Propagation.newFactoryBuilder().injectFormat(B3Propagation.Format.MULTI).build(); + } + + /** + * Creates a new W3C propagation factory. + * @return the W3C propagation factory + */ + private Propagation.Factory w3c() { + return (this.baggageManager != null) ? new W3CPropagation(this.baggageManager, Collections.emptyList()) + : new W3CPropagation(); + } - /** - * Creates a new B3 propagation factory using a single B3 header. - * @return the B3 propagation factory - */ - private static Factory b3Single() { - return B3Propagation.newFactoryBuilder().injectFormat(B3Propagation.Format.SINGLE_NO_PARENT).build(); } /** - * Creates a new B3 propagation factory using multiple B3 headers. - * @return the B3 propagation factory + * A collection of propagation factories. */ - private static Factory b3Multi() { - return B3Propagation.newFactoryBuilder().injectFormat(B3Propagation.Format.MULTI).build(); + private static class PropagationFactories { + + private final List factories; + + PropagationFactories(Collection factories) { + this.factories = List.copyOf(factories); + } + + boolean requires128BitTraceId() { + return stream().anyMatch(Propagation.Factory::requires128BitTraceId); + } + + boolean supportsJoin() { + return stream().allMatch(Propagation.Factory::supportsJoin); + } + + List> get() { + return stream().map(Factory::get).toList(); + } + + Stream stream() { + return this.factories.stream(); + } + } /** - * Creates a new W3C propagation factory. - * @param baggageManager baggage manager to use, or {@code null} - * @return the W3C propagation factory + * A composite {@link Propagation}. */ - private static W3CPropagation w3c(BaggageManager baggageManager) { - return (baggageManager != null) ? new W3CPropagation(baggageManager, Collections.emptyList()) - : new W3CPropagation(); + private static class CompositePropagation implements Propagation { + + private final List> injectors; + + private final List> extractors; + + private final List keys; + + CompositePropagation(PropagationFactories injectorFactories, PropagationFactories extractorFactories) { + this.injectors = injectorFactories.get(); + this.extractors = extractorFactories.get(); + this.keys = Stream.concat(keys(this.injectors), keys(this.extractors)).distinct().toList(); + } + + private Stream keys(List> propagations) { + return propagations.stream().flatMap((propagation) -> propagation.keys().stream()); + } + + @Override + public List keys() { + return this.keys; + } + + @Override + public TraceContext.Injector injector(Setter setter) { + return (traceContext, request) -> this.injectors.stream() + .map((propagation) -> propagation.injector(setter)) + .forEach((injector) -> injector.inject(traceContext, request)); + } + + @Override + public TraceContext.Extractor extractor(Getter getter) { + return (request) -> this.extractors.stream() + .map((propagation) -> propagation.extractor(getter)) + .map((extractor) -> extractor.extract(request)) + .filter(Predicate.not(TraceContextOrSamplingFlags.EMPTY::equals)) + .findFirst() + .orElse(TraceContextOrSamplingFlags.EMPTY); + } + } } diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/CompositeTextMapPropagator.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/CompositeTextMapPropagator.java index 03d89fb093..5db4f629a0 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/CompositeTextMapPropagator.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/CompositeTextMapPropagator.java @@ -19,6 +19,7 @@ package org.springframework.boot.actuate.autoconfigure.tracing; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.LinkedHashSet; import java.util.List; import java.util.Set; import java.util.stream.Collectors; @@ -32,6 +33,8 @@ import io.opentelemetry.context.propagation.TextMapPropagator; import io.opentelemetry.context.propagation.TextMapSetter; import io.opentelemetry.extension.trace.propagation.B3Propagator; +import org.springframework.boot.actuate.autoconfigure.tracing.TracingProperties.Propagation.PropagationType; + /** * {@link TextMapPropagator} which supports multiple tracing formats. It is able to * configure different formats for injecting and for extracting. @@ -42,9 +45,9 @@ class CompositeTextMapPropagator implements TextMapPropagator { private final Collection injectors; - private final Collection mutuallyExclusiveExtractors; + private final Collection extractors; - private final Collection alwaysRunningExtractors; + private final TextMapPropagator baggagePropagator; private final Set fields; @@ -54,19 +57,24 @@ class CompositeTextMapPropagator implements TextMapPropagator { * @param mutuallyExclusiveExtractors the mutually exclusive extractors. They are * applied in order, and as soon as an extractor extracts a context, the other * extractors after it are no longer invoked - * @param alwaysRunningExtractors the always running extractors. They always run in - * order, regardless of the mutually exclusive extractors or whether the extractor - * before it has already extracted a context + * @param baggagePropagator the baggage propagator to use, or {@code null} */ CompositeTextMapPropagator(Collection injectors, - Collection mutuallyExclusiveExtractors, - Collection alwaysRunningExtractors) { + Collection mutuallyExclusiveExtractors, TextMapPropagator baggagePropagator) { this.injectors = injectors; - this.mutuallyExclusiveExtractors = mutuallyExclusiveExtractors; - this.alwaysRunningExtractors = alwaysRunningExtractors; - this.fields = concat(this.injectors, this.mutuallyExclusiveExtractors, this.alwaysRunningExtractors) - .flatMap((entry) -> entry.fields().stream()) - .collect(Collectors.toSet()); + this.extractors = mutuallyExclusiveExtractors; + this.baggagePropagator = baggagePropagator; + Set fields = new LinkedHashSet<>(); + fields(this.injectors).forEach(fields::add); + fields(this.extractors).forEach(fields::add); + if (baggagePropagator != null) { + fields.addAll(baggagePropagator.fields()); + } + this.fields = Collections.unmodifiableSet(fields); + } + + private Stream fields(Collection propagators) { + return propagators.stream().flatMap((propagator) -> propagator.fields().stream()); } Collection getInjectors() { @@ -80,11 +88,8 @@ class CompositeTextMapPropagator implements TextMapPropagator { @Override public void inject(Context context, C carrier, TextMapSetter setter) { - if (context == null || setter == null) { - return; - } - for (TextMapPropagator injector : this.injectors) { - injector.inject(context, carrier, setter); + if (context != null && setter != null) { + this.injectors.forEach((injector) -> injector.inject(context, carrier, setter)); } } @@ -96,99 +101,81 @@ class CompositeTextMapPropagator implements TextMapPropagator { if (getter == null) { return context; } - Context currentContext = context; - for (TextMapPropagator extractor : this.mutuallyExclusiveExtractors) { - Context extractedContext = extractor.extract(currentContext, carrier, getter); - if (extractedContext != currentContext) { - currentContext = extractedContext; - break; - } + Context result = this.extractors.stream() + .map((extractor) -> extractor.extract(context, carrier, getter)) + .filter((extracted) -> extracted != context) + .findFirst() + .orElse(context); + if (this.baggagePropagator != null) { + result = this.baggagePropagator.extract(result, carrier, getter); } - for (TextMapPropagator extractor : this.alwaysRunningExtractors) { - currentContext = extractor.extract(currentContext, carrier, getter); - } - return currentContext; - } - - /** - * Creates a new {@link CompositeTextMapPropagator}, which uses the given - * {@code injectionTypes} for injection and {@code extractionTypes} for extraction. - * @param injectionTypes the propagation types for injection - * @param extractionTypes the propagation types for extraction - * @return the {@link CompositeTextMapPropagator} - */ - static TextMapPropagator create(Collection injectionTypes, - Collection extractionTypes) { - return create(null, injectionTypes, extractionTypes); + return result; } /** * Creates a new {@link CompositeTextMapPropagator}, which uses the given * {@code injectionTypes} for injection and {@code extractionTypes} for extraction. + * @param properties the tracing properties * @param baggagePropagator the baggage propagator to use, or {@code null} - * @param injectionTypes the propagation types for injection - * @param extractionTypes the propagation types for extraction * @return the {@link CompositeTextMapPropagator} */ - static CompositeTextMapPropagator create(TextMapPropagator baggagePropagator, - Collection injectionTypes, - Collection extractionTypes) { - List injectors = injectionTypes.stream() - .map((injection) -> forType(injection, baggagePropagator != null)) + static TextMapPropagator create(TracingProperties.Propagation properties, TextMapPropagator baggagePropagator) { + TextMapPropagatorMapper mapper = new TextMapPropagatorMapper(baggagePropagator != null); + List injectors = properties.getEffectiveProducedTypes() + .stream() + .map(mapper::map) .collect(Collectors.toCollection(ArrayList::new)); if (baggagePropagator != null) { injectors.add(baggagePropagator); } - List extractors = extractionTypes.stream() - .map((extraction) -> forType(extraction, baggagePropagator != null)) - .toList(); - return new CompositeTextMapPropagator(injectors, extractors, - (baggagePropagator != null) ? List.of(baggagePropagator) : Collections.emptyList()); - } - - @SafeVarargs - private static Stream concat(Collection... collections) { - Stream result = Stream.empty(); - for (Collection collection : collections) { - result = Stream.concat(result, collection.stream()); - } - return result; + List extractors = properties.getEffectiveProducedTypes().stream().map(mapper::map).toList(); + return new CompositeTextMapPropagator(injectors, extractors, baggagePropagator); } /** - * Creates a new B3 propagator using a single B3 header. - * @return the B3 propagator + * Mapper used to create a {@link TextMapPropagator} from a {@link PropagationType}. */ - private static TextMapPropagator b3Single() { - return B3Propagator.injectingSingleHeader(); - } + private static class TextMapPropagatorMapper { - /** - * Creates a new B3 propagator using multiple B3 headers. - * @return the B3 propagator - */ - private static TextMapPropagator b3Multi() { - return B3Propagator.injectingMultiHeaders(); - } + private final boolean baggage; - /** - * Creates a new W3C propagator. - * @param baggage whether baggage propagation should be supported - * @return the W3C propagator - */ - private static TextMapPropagator w3c(boolean baggage) { - if (!baggage) { - return W3CTraceContextPropagator.getInstance(); + TextMapPropagatorMapper(boolean baggage) { + this.baggage = baggage; + } + + TextMapPropagator map(PropagationType type) { + return switch (type) { + case B3 -> b3Single(); + case B3_MULTI -> b3Multi(); + case W3C -> w3c(); + }; + } + + /** + * Creates a new B3 propagator using a single B3 header. + * @return the B3 propagator + */ + private TextMapPropagator b3Single() { + return B3Propagator.injectingSingleHeader(); + } + + /** + * Creates a new B3 propagator using multiple B3 headers. + * @return the B3 propagator + */ + private TextMapPropagator b3Multi() { + return B3Propagator.injectingMultiHeaders(); + } + + /** + * Creates a new W3C propagator. + * @return the W3C propagator + */ + private TextMapPropagator w3c() { + return (!this.baggage) ? W3CTraceContextPropagator.getInstance() : TextMapPropagator + .composite(W3CTraceContextPropagator.getInstance(), W3CBaggagePropagator.getInstance()); } - return TextMapPropagator.composite(W3CTraceContextPropagator.getInstance(), W3CBaggagePropagator.getInstance()); - } - private static TextMapPropagator forType(TracingProperties.Propagation.PropagationType type, boolean baggage) { - return switch (type) { - case B3 -> b3Single(); - case B3_MULTI -> b3Multi(); - case W3C -> w3c(baggage); - }; } } diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/OpenTelemetryAutoConfiguration.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/OpenTelemetryAutoConfiguration.java index 256fa8a6d4..952b6ed7c1 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/OpenTelemetryAutoConfiguration.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/OpenTelemetryAutoConfiguration.java @@ -187,9 +187,7 @@ public class OpenTelemetryAutoConfiguration { List remoteFields = this.tracingProperties.getBaggage().getRemoteFields(); BaggageTextMapPropagator baggagePropagator = new BaggageTextMapPropagator(remoteFields, new OtelBaggageManager(otelCurrentTraceContext, remoteFields, Collections.emptyList())); - return CompositeTextMapPropagator.create(baggagePropagator, - this.tracingProperties.getPropagation().getEffectiveProducedTypes(), - this.tracingProperties.getPropagation().getEffectiveConsumedTypes()); + return CompositeTextMapPropagator.create(this.tracingProperties.getPropagation(), baggagePropagator); } @Bean @@ -208,8 +206,7 @@ public class OpenTelemetryAutoConfiguration { @Bean TextMapPropagator textMapPropagator(TracingProperties properties) { - return CompositeTextMapPropagator.create(properties.getPropagation().getEffectiveProducedTypes(), - properties.getPropagation().getEffectiveConsumedTypes()); + return CompositeTextMapPropagator.create(properties.getPropagation(), null); } } diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/TracingProperties.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/TracingProperties.java index 44af0ae5db..a1a6f69a10 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/TracingProperties.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/TracingProperties.java @@ -212,10 +212,7 @@ public class TracingProperties { * @return the effective context propagation types produced by the application */ List getEffectiveProducedTypes() { - if (this.type != null) { - return this.type; - } - return this.produce; + return (this.type != null) ? this.type : this.produce; } /** @@ -224,10 +221,7 @@ public class TracingProperties { * @return the effective context propagation types consumed by the application */ List getEffectiveConsumedTypes() { - if (this.type != null) { - return this.type; - } - return this.consume; + return (this.type != null) ? this.type : this.consume; } /** diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/tracing/BraveAutoConfigurationTests.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/tracing/BraveAutoConfigurationTests.java index abcba2b4ef..80d4de8424 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/tracing/BraveAutoConfigurationTests.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/tracing/BraveAutoConfigurationTests.java @@ -18,6 +18,7 @@ package org.springframework.boot.actuate.autoconfigure.tracing; import java.util.Collections; import java.util.List; +import java.util.stream.Stream; import brave.Span; import brave.SpanCustomizer; @@ -131,8 +132,8 @@ class BraveAutoConfigurationTests { this.contextRunner.run((context) -> { assertThat(context).hasBean("propagationFactory"); Factory factory = context.getBean(Factory.class); - List injectors = getInjectors(factory); - assertThat(injectors).extracting(Factory::getClass).containsExactly(W3CPropagation.class); + Stream> injectors = getInjectors(factory).stream().map(Object::getClass); + assertThat(injectors).containsExactly(W3CPropagation.class); assertThat(context).hasSingleBean(BaggagePropagation.FactoryBuilder.class); }); } @@ -165,8 +166,8 @@ class BraveAutoConfigurationTests { this.contextRunner.withPropertyValues("management.tracing.baggage.enabled=false").run((context) -> { assertThat(context).hasBean("propagationFactory"); Factory factory = context.getBean(Factory.class); - List injectors = getInjectors(factory); - assertThat(injectors).extracting(Factory::getClass).containsExactly(W3CPropagation.class); + Stream> injectors = getInjectors(factory).stream().map(Object::getClass); + assertThat(injectors).containsExactly(W3CPropagation.class); assertThat(context).doesNotHaveBean(BaggagePropagation.FactoryBuilder.class); }); } @@ -270,7 +271,7 @@ class BraveAutoConfigurationTests { private List getInjectors(Factory factory) { assertThat(factory).as("factory").isNotNull(); if (factory instanceof CompositePropagationFactory compositePropagationFactory) { - return compositePropagationFactory.getInjectorFactories().stream().toList(); + return compositePropagationFactory.getInjectors().toList(); } Assertions.fail("Expected CompositePropagationFactory, found %s".formatted(factory.getClass())); throw new AssertionError("Unreachable"); diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/tracing/CompositePropagationFactoryTests.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/tracing/CompositePropagationFactoryTests.java index a1bb020e76..a232104de4 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/tracing/CompositePropagationFactoryTests.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/tracing/CompositePropagationFactoryTests.java @@ -25,6 +25,7 @@ import brave.internal.propagation.StringPropagationAdapter; import brave.propagation.Propagation; import brave.propagation.TraceContext; import brave.propagation.TraceContextOrSamplingFlags; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.mockito.Mockito; @@ -39,72 +40,80 @@ import static org.mockito.BDDMockito.given; */ class CompositePropagationFactoryTests { - @Test - void returnsAllKeys() { - CompositePropagationFactory factory = new CompositePropagationFactory(List.of(field("a")), List.of(field("b"))); - assertThat(factory.keys()).containsExactly("a", "b"); - } - @Test void supportsJoin() { - Propagation.Factory supportsJoin = Mockito.mock(Propagation.Factory.class); - given(supportsJoin.supportsJoin()).willReturn(true); - given(supportsJoin.get()).willReturn(new DummyPropagation("a")); - Propagation.Factory doesNotSupportsJoin = Mockito.mock(Propagation.Factory.class); - given(doesNotSupportsJoin.supportsJoin()).willReturn(false); - given(doesNotSupportsJoin.get()).willReturn(new DummyPropagation("a")); - CompositePropagationFactory factory = new CompositePropagationFactory(List.of(supportsJoin), - List.of(doesNotSupportsJoin)); + Propagation.Factory supported = Mockito.mock(Propagation.Factory.class); + given(supported.supportsJoin()).willReturn(true); + given(supported.get()).willReturn(new DummyPropagation("a")); + Propagation.Factory unsupported = Mockito.mock(Propagation.Factory.class); + given(unsupported.supportsJoin()).willReturn(false); + given(unsupported.get()).willReturn(new DummyPropagation("a")); + CompositePropagationFactory factory = new CompositePropagationFactory(List.of(supported), List.of(unsupported)); assertThat(factory.supportsJoin()).isFalse(); } @Test void requires128BitTraceId() { - Propagation.Factory requires128BitTraceId = Mockito.mock(Propagation.Factory.class); - given(requires128BitTraceId.requires128BitTraceId()).willReturn(true); - given(requires128BitTraceId.get()).willReturn(new DummyPropagation("a")); - Propagation.Factory doesNotRequire128BitTraceId = Mockito.mock(Propagation.Factory.class); - given(doesNotRequire128BitTraceId.requires128BitTraceId()).willReturn(false); - given(doesNotRequire128BitTraceId.get()).willReturn(new DummyPropagation("a")); - CompositePropagationFactory factory = new CompositePropagationFactory(List.of(requires128BitTraceId), - List.of(doesNotRequire128BitTraceId)); + Propagation.Factory required = Mockito.mock(Propagation.Factory.class); + given(required.requires128BitTraceId()).willReturn(true); + given(required.get()).willReturn(new DummyPropagation("a")); + Propagation.Factory notRequired = Mockito.mock(Propagation.Factory.class); + given(notRequired.requires128BitTraceId()).willReturn(false); + given(notRequired.get()).willReturn(new DummyPropagation("a")); + CompositePropagationFactory factory = new CompositePropagationFactory(List.of(required), List.of(notRequired)); assertThat(factory.requires128BitTraceId()).isTrue(); } - @Test - void inject() { - CompositePropagationFactory factory = new CompositePropagationFactory(List.of(field("a"), field("b")), - List.of(field("c"))); - TraceContext context = context(); - Map request = new HashMap<>(); - factory.injector(new MapSetter()).inject(context, request); - assertThat(request).containsOnly(entry("a", "a-value"), entry("b", "b-value")); - } + @Nested + static class CompostePropagationTests { - @Test - void extractorStopsAfterSuccessfulExtraction() { - CompositePropagationFactory factory = new CompositePropagationFactory(Collections.emptyList(), - List.of(field("a"), field("b"))); - Map request = Map.of("a", "a-value", "b", "b-value"); - TraceContextOrSamplingFlags context = factory.extractor(new MapGetter()).extract(request); - assertThat(context.context().extra()).containsExactly("a"); - } + @Test + void keys() { + CompositePropagationFactory factory = new CompositePropagationFactory(List.of(field("a")), + List.of(field("b"))); + Propagation propagation = factory.get(); + assertThat(propagation.keys()).containsExactly("a", "b"); + } - @Test - void returnsEmptyContextWhenNoExtractorMatches() { - CompositePropagationFactory factory = new CompositePropagationFactory(Collections.emptyList(), - Collections.emptyList()); - Map request = Collections.emptyMap(); - TraceContextOrSamplingFlags context = factory.extractor(new MapGetter()).extract(request); - assertThat(context.context()).isNull(); - } + @Test + void inject() { + CompositePropagationFactory factory = new CompositePropagationFactory(List.of(field("a"), field("b")), + List.of(field("c"))); + Propagation propagation = factory.get(); + TraceContext context = context(); + Map request = new HashMap<>(); + propagation.injector(new MapSetter()).inject(context, request); + assertThat(request).containsOnly(entry("a", "a-value"), entry("b", "b-value")); + } - private static TraceContext context() { - return TraceContext.newBuilder().traceId(1).spanId(2).build(); - } + @Test + void extractorWhenDelegateExtractsReturnsExtraction() { + CompositePropagationFactory factory = new CompositePropagationFactory(Collections.emptyList(), + List.of(field("a"), field("b"))); + Propagation propagation = factory.get(); + Map request = Map.of("a", "a-value", "b", "b-value"); + TraceContextOrSamplingFlags context = propagation.extractor(new MapGetter()).extract(request); + assertThat(context.context().extra()).containsExactly("a"); + } + + @Test + void extractorWhenWhenNoExtractorMatchesReturnsEmptyContext() { + CompositePropagationFactory factory = new CompositePropagationFactory(Collections.emptyList(), + Collections.emptyList()); + Propagation propagation = factory.get(); + Map request = Collections.emptyMap(); + TraceContextOrSamplingFlags context = propagation.extractor(new MapGetter()).extract(request); + assertThat(context.context()).isNull(); + } + + private static TraceContext context() { + return TraceContext.newBuilder().traceId(1).spanId(2).build(); + } + + private static DummyPropagation field(String field) { + return new DummyPropagation(field); + } - private static DummyPropagation field(String field) { - return new DummyPropagation(field); } private static final class MapSetter implements Propagation.Setter, String> { diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/tracing/CompositeTextMapPropagatorTests.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/tracing/CompositeTextMapPropagatorTests.java index 90cb2a4c51..dc5ccbc6c9 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/tracing/CompositeTextMapPropagatorTests.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/tracing/CompositeTextMapPropagatorTests.java @@ -51,14 +51,14 @@ class CompositeTextMapPropagatorTests { @Test void collectsAllFields() { CompositeTextMapPropagator propagator = new CompositeTextMapPropagator(List.of(field("a")), List.of(field("b")), - List.of(field("c"))); + field("c")); assertThat(propagator.fields()).containsExactly("a", "b", "c"); } @Test void injectAllFields() { CompositeTextMapPropagator propagator = new CompositeTextMapPropagator(List.of(field("a"), field("b")), - Collections.emptyList(), Collections.emptyList()); + Collections.emptyList(), null); TextMapSetter setter = setter(); Object carrier = carrier(); propagator.inject(context(), carrier, setter); @@ -68,9 +68,9 @@ class CompositeTextMapPropagatorTests { } @Test - void extractMutuallyExclusive() { + void extractWithoutBaggagePropagator() { CompositeTextMapPropagator propagator = new CompositeTextMapPropagator(Collections.emptyList(), - List.of(field("a"), field("b")), Collections.emptyList()); + List.of(field("a"), field("b")), null); Context context = context(); Map carrier = Map.of("a", "a-value", "b", "b-value"); context = propagator.extract(context, carrier, new MapTextMapGetter()); @@ -81,9 +81,9 @@ class CompositeTextMapPropagatorTests { } @Test - void extractAlwaysRunning() { + void extractWithBaggagePropagator() { CompositeTextMapPropagator propagator = new CompositeTextMapPropagator(Collections.emptyList(), - List.of(field("a"), field("b")), List.of(field("c"))); + List.of(field("a"), field("b")), field("c")); Context context = context(); Map carrier = Map.of("a", "a-value", "b", "b-value", "c", "c-value"); context = propagator.extract(context, carrier, new MapTextMapGetter()); diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/tracing/OpenTelemetryAutoConfigurationTests.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/tracing/OpenTelemetryAutoConfigurationTests.java index 1192e9aa3b..128dad34e0 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/tracing/OpenTelemetryAutoConfigurationTests.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/tracing/OpenTelemetryAutoConfigurationTests.java @@ -18,6 +18,7 @@ package org.springframework.boot.actuate.autoconfigure.tracing; import java.util.ArrayList; import java.util.List; +import java.util.stream.Stream; import io.micrometer.tracing.SpanCustomizer; import io.micrometer.tracing.otel.bridge.OtelCurrentTraceContext; @@ -174,9 +175,8 @@ class OpenTelemetryAutoConfigurationTests { void shouldSupplyB3PropagationIfPropagationPropertySet() { this.contextRunner.withPropertyValues("management.tracing.propagation.type=B3").run((context) -> { TextMapPropagator propagator = context.getBean(TextMapPropagator.class); - List injectors = getInjectors(propagator); - assertThat(injectors).extracting(TextMapPropagator::getClass) - .containsExactly(B3Propagator.class, BaggageTextMapPropagator.class); + Stream> injectors = getInjectors(propagator).stream().map(Object::getClass); + assertThat(injectors).containsExactly(B3Propagator.class, BaggageTextMapPropagator.class); }); } @@ -186,8 +186,8 @@ class OpenTelemetryAutoConfigurationTests { .withPropertyValues("management.tracing.propagation.type=B3", "management.tracing.baggage.enabled=false") .run((context) -> { TextMapPropagator propagator = context.getBean(TextMapPropagator.class); - List injectors = getInjectors(propagator); - assertThat(injectors).extracting(TextMapPropagator::getClass).containsExactly(B3Propagator.class); + Stream> injectors = getInjectors(propagator).stream().map(Object::getClass); + assertThat(injectors).containsExactly(B3Propagator.class); }); } @@ -208,9 +208,8 @@ class OpenTelemetryAutoConfigurationTests { void shouldSupplyW3CPropagationWithoutBaggageWhenDisabled() { this.contextRunner.withPropertyValues("management.tracing.baggage.enabled=false").run((context) -> { TextMapPropagator propagator = context.getBean(TextMapPropagator.class); - List injectors = getInjectors(propagator); - assertThat(injectors).extracting(TextMapPropagator::getClass) - .containsExactly(W3CTraceContextPropagator.class); + Stream> injectors = getInjectors(propagator).stream().map(Object::getClass); + assertThat(injectors).containsExactly(W3CTraceContextPropagator.class); }); } From 0fd1c89889d7c7a8cb70618854d9499519429593 Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Fri, 16 Jun 2023 13:53:33 -0700 Subject: [PATCH 3/3] Polish --- .../boot/context/properties/bind/JavaBeanBinder.java | 8 +++----- .../boot/image/assertions/ContainerConfigAssert.java | 11 +++++++++-- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/JavaBeanBinder.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/JavaBeanBinder.java index 4134cc58e4..44af313086 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/JavaBeanBinder.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/JavaBeanBinder.java @@ -392,11 +392,9 @@ class JavaBeanBinder implements DataObjectBinder { } private boolean isUninitializedKotlinProperty(Exception ex) { - if (ex instanceof InvocationTargetException ite) { - return "kotlin.UninitializedPropertyAccessException" - .equals(ite.getTargetException().getClass().getName()); - } - return false; + return (ex instanceof InvocationTargetException invocationTargetException) + && "kotlin.UninitializedPropertyAccessException" + .equals(invocationTargetException.getTargetException().getClass().getName()); } boolean isSettable() { diff --git a/spring-boot-system-tests/spring-boot-image-tests/src/systemTest/java/org/springframework/boot/image/assertions/ContainerConfigAssert.java b/spring-boot-system-tests/spring-boot-image-tests/src/systemTest/java/org/springframework/boot/image/assertions/ContainerConfigAssert.java index e8dffb4b16..3c0368813b 100644 --- a/spring-boot-system-tests/spring-boot-image-tests/src/systemTest/java/org/springframework/boot/image/assertions/ContainerConfigAssert.java +++ b/spring-boot-system-tests/spring-boot-image-tests/src/systemTest/java/org/springframework/boot/image/assertions/ContainerConfigAssert.java @@ -16,6 +16,7 @@ package org.springframework.boot.image.assertions; +import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.Map; @@ -106,8 +107,14 @@ public class ContainerConfigAssert extends AbstractAssert (list != null) ? ((List) list).stream().map(Objects::toString).toList() - : Collections.emptyList()); + .flatMap(this::getArgs); + } + + private Collection getArgs(Object obj) { + if (obj instanceof List list) { + return list.stream().map(Objects::toString).toList(); + } + return Collections.emptyList(); } }