From 03be784678e7757e613d86243e285423befd991d Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Mon, 18 May 2020 23:43:23 +0800 Subject: [PATCH] Ensures Zipkin handlers order last (after redacters etc) (#1648) This is more prevention of a problem, then seeing one in practice. Technically, a TracingCustomizer can order things backwards, and it hit me in a test env (not sleuth). --- .../autoconfig/TraceAutoConfiguration.java | 31 +++++++++++++++++++ .../TraceAutoConfigurationTests.java | 25 +++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/autoconfig/TraceAutoConfiguration.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/autoconfig/TraceAutoConfiguration.java index 95fb32cc8..14fe86070 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/autoconfig/TraceAutoConfiguration.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/autoconfig/TraceAutoConfiguration.java @@ -16,7 +16,9 @@ package org.springframework.cloud.sleuth.autoconfig; +import java.util.ArrayList; import java.util.Collections; +import java.util.Comparator; import java.util.List; import brave.CurrentSpanCustomizer; @@ -37,6 +39,7 @@ import zipkin2.Span; import zipkin2.reporter.InMemoryReporterMetrics; import zipkin2.reporter.Reporter; import zipkin2.reporter.ReporterMetrics; +import zipkin2.reporter.brave.ZipkinSpanHandler; import zipkin2.reporter.metrics.micrometer.MicrometerReporterMetrics; import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; @@ -85,6 +88,22 @@ public class TraceAutoConfiguration { */ public static final String DEFAULT_SERVICE_NAME = "default"; + /** + * Sort Zipkin Handlers last, so that redactions etc happen prior. + */ + static final Comparator SPAN_HANDLER_COMPARATOR = (o1, o2) -> { + if (o1 instanceof ZipkinSpanHandler) { + if (o2 instanceof ZipkinSpanHandler) { + return 0; + } + return 1; + } + else if (o2 instanceof ZipkinSpanHandler) { + return -1; + } + return 0; + }; + @Bean @ConditionalOnMissingBean // NOTE: stable bean name as might be used outside sleuth @@ -117,9 +136,21 @@ public class TraceAutoConfiguration { customizer.customize(builder); } } + + reorderZipkinHandlersLast(builder); return builder.build(); } + private void reorderZipkinHandlersLast(Tracing.Builder builder) { + List configuredSpanHandlers = new ArrayList<>( + builder.spanHandlers()); + configuredSpanHandlers.sort(SPAN_HANDLER_COMPARATOR); + builder.clearSpanHandlers(); + for (SpanHandler spanHandler : configuredSpanHandlers) { + builder.addSpanHandler(spanHandler); + } + } + @Bean(name = TRACER_BEAN_NAME) @ConditionalOnMissingBean Tracer tracer(Tracing tracing) { diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/autoconfig/TraceAutoConfigurationTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/autoconfig/TraceAutoConfigurationTests.java index a294edecb..943104edd 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/autoconfig/TraceAutoConfigurationTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/autoconfig/TraceAutoConfigurationTests.java @@ -16,6 +16,7 @@ package org.springframework.cloud.sleuth.autoconfig; +import java.util.ArrayList; import java.util.List; import brave.Tracing; @@ -23,6 +24,7 @@ import brave.baggage.BaggageField; import brave.baggage.BaggagePropagation; import brave.baggage.BaggagePropagationConfig.SingleBaggageField; import brave.baggage.BaggagePropagationCustomizer; +import brave.handler.SpanHandler; import brave.propagation.B3Propagation; import brave.propagation.B3SinglePropagation; import brave.propagation.ExtraFieldPropagation; @@ -38,6 +40,7 @@ import org.junit.Test; import zipkin2.reporter.InMemoryReporterMetrics; import zipkin2.reporter.Reporter; import zipkin2.reporter.ReporterMetrics; +import zipkin2.reporter.brave.ZipkinSpanHandler; import zipkin2.reporter.metrics.micrometer.MicrometerReporterMetrics; import org.springframework.beans.factory.annotation.Autowired; @@ -47,11 +50,33 @@ import org.springframework.boot.test.context.runner.ApplicationContextRunner; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.springframework.cloud.sleuth.autoconfig.TraceAutoConfiguration.SPAN_HANDLER_COMPARATOR; + public class TraceAutoConfigurationTests { private final ApplicationContextRunner contextRunner = new ApplicationContextRunner() .withConfiguration(AutoConfigurations.of(TraceAutoConfiguration.class)); + @Test + public void span_handler_comparator() { + SpanHandler handler1 = mock(SpanHandler.class); + SpanHandler handler2 = mock(SpanHandler.class); + ZipkinSpanHandler zipkin1 = mock(ZipkinSpanHandler.class); + ZipkinSpanHandler zipkin2 = mock(ZipkinSpanHandler.class); + + ArrayList spanHandlers = new ArrayList<>(); + spanHandlers.add(handler1); + spanHandlers.add(zipkin1); + spanHandlers.add(handler2); + spanHandlers.add(zipkin2); + + spanHandlers.sort(SPAN_HANDLER_COMPARATOR); + + assertThat(spanHandlers).containsExactly(handler1, handler2, zipkin1, zipkin2); + } + @Test public void should_apply_micrometer_reporter_metrics_when_meter_registry_bean_present() { this.contextRunner.withUserConfiguration(WithMeterRegistry.class)