From 0bbd7cb70f0cc9c0f9b59217c5bc3ac9e59f2df6 Mon Sep 17 00:00:00 2001 From: Dave Syer Date: Thu, 21 Jan 2016 09:48:31 +0000 Subject: [PATCH] Fix messaging and ribbon instrumentation for hex headers It still feels brittle because it's up to the suthor of the instrumentation. This patches all the places where we were still using Long instead of Long.toHexString(..). Fixes gh-125 --- .../AbstractTraceChannelInterceptor.java | 12 +++---- .../integration/SpanMessageHeaders.java | 19 +++++----- .../integration/StompMessageBuilder.java | 14 ++++---- .../TraceFeignClientAutoConfiguration.java | 20 ++++++----- .../TraceRestClientRibbonCommandFactory.java | 18 +++++----- .../AbstractTraceStompIntegrationTests.java | 8 ++--- .../TraceChannelInterceptorTests.java | 36 +++++++++---------- .../pom.xml | 5 --- 8 files changed, 67 insertions(+), 65 deletions(-) diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/integration/AbstractTraceChannelInterceptor.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/integration/AbstractTraceChannelInterceptor.java index c43546c23..e0ec960d3 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/integration/AbstractTraceChannelInterceptor.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/integration/AbstractTraceChannelInterceptor.java @@ -1,5 +1,7 @@ package org.springframework.cloud.sleuth.instrument.integration; +import java.util.Random; + import org.springframework.cloud.sleuth.Span; import org.springframework.cloud.sleuth.Tracer; import org.springframework.cloud.sleuth.instrument.TraceKeys; @@ -9,8 +11,6 @@ import org.springframework.messaging.Message; import org.springframework.messaging.MessageChannel; import org.springframework.messaging.support.ChannelInterceptorAdapter; -import java.util.Random; - /** * Abstraction over classes related to channel intercepting * @@ -47,13 +47,13 @@ abstract class AbstractTraceChannelInterceptor extends ChannelInterceptorAdapter return null; // cannot build a span without ids } long spanId = hasHeader(message, Span.SPAN_ID_NAME) ? - getHeader(message, Span.SPAN_ID_NAME, Long.class) : this.random.nextLong(); - long traceId = getHeader(message, Span.TRACE_ID_NAME, Long.class); + Span.fromHex(getHeader(message, Span.SPAN_ID_NAME)) : this.random.nextLong(); + long traceId = Span.fromHex(getHeader(message, Span.TRACE_ID_NAME)); Span.SpanBuilder span = Span.builder().traceId(traceId).spanId(spanId); - Long parentId = getHeader(message, Span.PARENT_ID_NAME, Long.class); if (message.getHeaders().containsKey(Span.NOT_SAMPLED_NAME)) { span.exportable(false); } + String parentId = getHeader(message, Span.PARENT_ID_NAME); String processId = getHeader(message, Span.PROCESS_ID_NAME); String spanName = getHeader(message, Span.SPAN_NAME_NAME); if (spanName != null) { @@ -63,7 +63,7 @@ abstract class AbstractTraceChannelInterceptor extends ChannelInterceptorAdapter span.processId(processId); } if (parentId != null) { - span.parent(parentId); + span.parent(Span.fromHex(parentId)); } span.remote(true); return span.build(); diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/integration/SpanMessageHeaders.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/integration/SpanMessageHeaders.java index cae280089..57b119ec5 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/integration/SpanMessageHeaders.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/integration/SpanMessageHeaders.java @@ -16,16 +16,16 @@ package org.springframework.cloud.sleuth.instrument.integration; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + import org.springframework.cloud.sleuth.Span; import org.springframework.cloud.sleuth.instrument.TraceKeys; import org.springframework.integration.support.MessageBuilder; import org.springframework.messaging.Message; import org.springframework.util.StringUtils; -import java.util.HashMap; -import java.util.List; -import java.util.Map; - /** * Utility for manipulating message headers related to span data. * @@ -45,12 +45,15 @@ public class SpanMessageHeaders { } Map headers = new HashMap<>(); - addHeader(headers, Span.TRACE_ID_NAME, span.getTraceId()); - addHeader(headers, Span.SPAN_ID_NAME, span.getSpanId()); + addHeader(headers, Span.TRACE_ID_NAME, Span.toHex(span.getTraceId())); + addHeader(headers, Span.SPAN_ID_NAME, Span.toHex(span.getSpanId())); if (span.isExportable()) { addAnnotations(traceKeys, message, span); - addHeader(headers, Span.PARENT_ID_NAME, getFirst(span.getParents())); + Long parentId = getFirst(span.getParents()); + if (parentId != null) { + addHeader(headers, Span.PARENT_ID_NAME, Span.toHex(parentId)); + } addHeader(headers, Span.SPAN_NAME_NAME, span.getName()); addHeader(headers, Span.PROCESS_ID_NAME, span.getProcessId()); } @@ -69,7 +72,7 @@ public class SpanMessageHeaders { if (value == null) { value = "null"; } - span.tag(key, value.toString()); // TODO: better way to serialize? + span.tag(key, value.toString()); // TODO: better way to serialize? } } addPayloadAnnotations(traceKeys, message.getPayload(), span); diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/integration/StompMessageBuilder.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/integration/StompMessageBuilder.java index 3d3351bdb..a6b9c5d06 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/integration/StompMessageBuilder.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/integration/StompMessageBuilder.java @@ -16,6 +16,10 @@ package org.springframework.cloud.sleuth.instrument.integration; +import java.util.List; +import java.util.Map; +import java.util.TreeMap; + import org.springframework.cloud.sleuth.Span; import org.springframework.cloud.sleuth.trace.SpanContextHolder; import org.springframework.messaging.Message; @@ -23,10 +27,6 @@ import org.springframework.messaging.simp.SimpMessageHeaderAccessor; import org.springframework.messaging.simp.SimpMessageType; import org.springframework.util.StringUtils; -import java.util.List; -import java.util.Map; -import java.util.TreeMap; - /** * Builder class to create STOMP message * @@ -61,12 +61,12 @@ public class StompMessageBuilder { public StompMessageBuilder setHeadersFromSpan(final Span span) { if (span != null) { - setHeaderIfAbsent(Span.SPAN_ID_NAME, span.getSpanId()); - setHeaderIfAbsent(Span.TRACE_ID_NAME, span.getTraceId()); + setHeaderIfAbsent(Span.SPAN_ID_NAME, Span.toHex(span.getSpanId())); + setHeaderIfAbsent(Span.TRACE_ID_NAME, Span.toHex(span.getTraceId())); setHeaderIfAbsent(Span.SPAN_NAME_NAME, span.getName()); Long parentId = getParentId(SpanContextHolder.getCurrentSpan()); if (parentId != null) - setHeaderIfAbsent(Span.PARENT_ID_NAME, parentId); + setHeaderIfAbsent(Span.PARENT_ID_NAME, Span.toHex(parentId)); String processId = span.getProcessId(); if (StringUtils.hasText(processId)) diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceFeignClientAutoConfiguration.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceFeignClientAutoConfiguration.java index 12d1a4e18..c0ec51f15 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceFeignClientAutoConfiguration.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceFeignClientAutoConfiguration.java @@ -89,8 +89,8 @@ public class TraceFeignClientAutoConfiguration { @ConditionalOnMissingBean(SleuthHystrixConcurrencyStrategy.class) @ConditionalOnProperty(name = "feign.hystrix.enabled", matchIfMissing = true) public Feign.Builder feignHystrixBuilder(Tracer tracer) { - return HystrixFeign.builder() - .invocationHandlerFactory(new SleuthHystrixInvocationHandler.Factory(tracer)); + return HystrixFeign.builder().invocationHandlerFactory( + new SleuthHystrixInvocationHandler.Factory(tracer)); } @Bean @@ -127,8 +127,11 @@ public class TraceFeignClientAutoConfiguration { } template.header(Span.TRACE_ID_NAME, Span.toHex(span.getTraceId())); setHeader(template, Span.SPAN_NAME_NAME, span.getName()); - setHeader(template, Span.SPAN_ID_NAME, span.getSpanId()); - setHeader(template, Span.PARENT_ID_NAME, getParentId(span)); + setHeader(template, Span.SPAN_ID_NAME, Span.toHex(span.getSpanId())); + Long parentId = getParentId(span); + if (parentId != null) { + setHeader(template, Span.PARENT_ID_NAME, Span.toHex(parentId)); + } setHeader(template, Span.PROCESS_ID_NAME, span.getProcessId()); publish(new ClientSentEvent(this, span)); } @@ -142,8 +145,7 @@ public class TraceFeignClientAutoConfiguration { } private Long getParentId(Span span) { - return !span.getParents().isEmpty() - ? span.getParents().get(0) : null; + return !span.getParents().isEmpty() ? span.getParents().get(0) : null; } public void setHeader(RequestTemplate request, String name, String value) { @@ -176,13 +178,15 @@ public class TraceFeignClientAutoConfiguration { public void setHeader(Map> headers, String name, String value) { - if (StringUtils.hasText(value) && !headers.containsKey(name) && this.accessor.isTracing()) { + if (StringUtils.hasText(value) && !headers.containsKey(name) + && this.accessor.isTracing()) { headers.put(name, singletonList(value)); } } + public void setHeader(Map> headers, String name, Long value) { - if (value != null ){ + if (value != null) { setHeader(headers, name, Span.toHex(value)); } } diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/zuul/TraceRestClientRibbonCommandFactory.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/zuul/TraceRestClientRibbonCommandFactory.java index fe1dcffd1..20a379102 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/zuul/TraceRestClientRibbonCommandFactory.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/zuul/TraceRestClientRibbonCommandFactory.java @@ -16,9 +16,9 @@ package org.springframework.cloud.sleuth.instrument.zuul; -import com.netflix.client.http.HttpRequest; -import com.netflix.niws.client.http.RestClient; -import lombok.SneakyThrows; +import java.io.InputStream; +import java.net.URISyntaxException; + import org.springframework.cloud.netflix.ribbon.SpringClientFactory; import org.springframework.cloud.netflix.zuul.filters.route.RestClientRibbonCommand; import org.springframework.cloud.netflix.zuul.filters.route.RestClientRibbonCommandFactory; @@ -31,8 +31,10 @@ import org.springframework.context.ApplicationEventPublisher; import org.springframework.context.ApplicationEventPublisherAware; import org.springframework.util.MultiValueMap; -import java.io.InputStream; -import java.net.URISyntaxException; +import com.netflix.client.http.HttpRequest; +import com.netflix.niws.client.http.RestClient; + +import lombok.SneakyThrows; /** * @author Spencer Gibb @@ -93,11 +95,11 @@ public class TraceRestClientRibbonCommandFactory extends RestClientRibbonCommand setHeader(requestBuilder, Span.NOT_SAMPLED_NAME, ""); return; } - setHeader(requestBuilder, Span.TRACE_ID_NAME, span.getTraceId()); - setHeader(requestBuilder, Span.SPAN_ID_NAME, span.getSpanId()); + setHeader(requestBuilder, Span.TRACE_ID_NAME, Span.toHex(span.getTraceId())); + setHeader(requestBuilder, Span.SPAN_ID_NAME, Span.toHex(span.getSpanId())); setHeader(requestBuilder, Span.SPAN_NAME_NAME, span.getName()); setHeader(requestBuilder, Span.PARENT_ID_NAME, - getParentId(span)); + Span.toHex(getParentId(span))); setHeader(requestBuilder, Span.PROCESS_ID_NAME, span.getProcessId()); publish(new ClientSentEvent(this, span)); diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/integration/AbstractTraceStompIntegrationTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/integration/AbstractTraceStompIntegrationTests.java index b56c04fa1..ca550b796 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/integration/AbstractTraceStompIntegrationTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/integration/AbstractTraceStompIntegrationTests.java @@ -1,5 +1,7 @@ package org.springframework.cloud.sleuth.instrument.integration; +import static org.assertj.core.api.BDDAssertions.then; + import org.junit.After; import org.junit.Before; import org.junit.runner.RunWith; @@ -14,8 +16,6 @@ import org.springframework.messaging.support.ExecutorSubscribableChannel; import org.springframework.messaging.support.GenericMessage; import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; -import static org.assertj.core.api.BDDAssertions.then; - @RunWith(SpringJUnit4ClassRunner.class) public abstract class AbstractTraceStompIntegrationTests { @@ -52,13 +52,13 @@ public abstract class AbstractTraceStompIntegrationTests { } Long thenSpanIdFromHeadersIsNotEmpty() { - Long header = getValueFromHeaders(Span.SPAN_ID_NAME, Long.class); + Long header = Span.fromHex(getValueFromHeaders(Span.SPAN_ID_NAME, String.class)); then(header).as("Span id should not be empty").isNotNull(); return header; } Long thenTraceIdFromHeadersIsNotEmpty() { - Long header = getValueFromHeaders(Span.TRACE_ID_NAME, Long.class); + Long header = Span.fromHex(getValueFromHeaders(Span.TRACE_ID_NAME, String.class)); then(header).as("Trace id should not be empty").isNotNull(); return header; } diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/integration/TraceChannelInterceptorTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/integration/TraceChannelInterceptorTests.java index 3d1d8bb57..c72057cbb 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/integration/TraceChannelInterceptorTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/integration/TraceChannelInterceptorTests.java @@ -16,6 +16,15 @@ package org.springframework.cloud.sleuth.instrument.integration; +import static org.assertj.core.api.BDDAssertions.then; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; + +import java.util.ArrayList; +import java.util.List; + import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -43,15 +52,6 @@ import org.springframework.messaging.MessagingException; import org.springframework.test.annotation.DirtiesContext; import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; -import java.util.ArrayList; -import java.util.List; - -import static org.assertj.core.api.BDDAssertions.then; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertNull; - /** * @author Dave Syer */ @@ -97,8 +97,8 @@ public class TraceChannelInterceptorTests implements MessageHandler { @Test public void nonExportableSpanCreation() { - this.channel.send(MessageBuilder.withPayload("hi").setHeader(Span.NOT_SAMPLED_NAME, "") - .build()); + this.channel.send(MessageBuilder.withPayload("hi") + .setHeader(Span.NOT_SAMPLED_NAME, "").build()); assertNotNull("message was null", this.message); String spanId = this.message.getHeaders().get(Span.SPAN_ID_NAME, String.class); @@ -109,9 +109,9 @@ public class TraceChannelInterceptorTests implements MessageHandler { @Test public void parentSpanIncluded() { - this.channel.send(MessageBuilder.withPayload("hi").setHeader(Span.TRACE_ID_NAME, 10L) - .setHeader(Span.SPAN_ID_NAME, 20L) - .build()); + this.channel.send(MessageBuilder.withPayload("hi") + .setHeader(Span.TRACE_ID_NAME, Span.toHex(10L)) + .setHeader(Span.SPAN_ID_NAME, Span.toHex(20L)).build()); assertNotNull("message was null", this.message); String spanId = this.message.getHeaders().get(Span.SPAN_ID_NAME, String.class); @@ -139,8 +139,7 @@ public class TraceChannelInterceptorTests implements MessageHandler { @Test public void headerCreation() { - Span span = this.tracer.startTrace("testSendMessage", - new AlwaysSampler()); + Span span = this.tracer.startTrace("testSendMessage", new AlwaysSampler()); this.channel.send(MessageBuilder.withPayload("hi").build()); this.tracer.close(span); assertNotNull("message was null", this.message); @@ -156,8 +155,7 @@ public class TraceChannelInterceptorTests implements MessageHandler { // TODO: Refactor to parametrized test together with sending messages via channel @Test public void headerCreationViaMessagingTemplate() { - Span span = this.tracer.startTrace("testSendMessage", - new AlwaysSampler()); + Span span = this.tracer.startTrace("testSendMessage", new AlwaysSampler()); this.messagingTemplate.send(MessageBuilder.withPayload("hi").build()); this.tracer.close(span); assertNotNull("message was null", this.message); @@ -177,7 +175,7 @@ public class TraceChannelInterceptorTests implements MessageHandler { private List events = new ArrayList<>(); @EventListener - public void handle(SpanReleasedEvent event) { + public void handle(SpanReleasedEvent event) { this.events.add(event); } diff --git a/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-zipkin-stream/pom.xml b/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-zipkin-stream/pom.xml index 77f6e8273..922aeb7f9 100644 --- a/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-zipkin-stream/pom.xml +++ b/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-zipkin-stream/pom.xml @@ -28,11 +28,6 @@ spring-boot-configuration-processor true - - org.springframework.boot - spring-boot-devtools - true - org.springframework.boot spring-boot-starter-jdbc