From a559c11bcd11e1eb25fc69a756315440949be070 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Wed, 11 Jan 2017 12:31:18 +0100 Subject: [PATCH] Due to possible performance problems when DiscoveryClient gives nonexistent DNS name as local address Local endpoint is created from server properties by default. Local Ip determined from spring.commons InetUtils. To enable local endpoint resolution via service discovery, DiscoveryClient must be present and 'spring.zipkin.service.locator.discovery' should be set to true. FallbackHavingEndpointLocator was caching and recreating EndpointLocator in the same time - removed the caching part. Fixes gh-403 --- .../main/asciidoc/spring-cloud-sleuth.adoc | 15 +++ spring-cloud-sleuth-stream/pom.xml | 1 - .../stream/DiscoveryClientHostLocator.java | 4 +- .../stream/ServerPropertiesHostLocator.java | 34 +++++-- .../stream/SleuthStreamAutoConfiguration.java | 19 +++- .../cloud/sleuth/stream/ZipkinProperties.java | 63 +++++++++++-- ...lientEndpointLocatorConfigurationTest.java | 94 +++++++++++++++++++ .../DiscoveryClientHostLocatorTest.java | 2 +- .../ServerPropertiesHostLocatorTests.java | 37 +++++--- .../SleuthStreamAutoConfigurationTest.java | 3 +- .../stream/StreamSpanListenerTests.java | 3 +- spring-cloud-sleuth-zipkin/pom.xml | 1 - .../zipkin/FallbackHavingEndpointLocator.java | 7 +- .../ServerPropertiesEndpointLocator.java | 22 +++-- .../zipkin/ZipkinAutoConfiguration.java | 17 +++- .../cloud/sleuth/zipkin/ZipkinProperties.java | 37 ++++++++ .../sleuth/zipkin/ZipkinSpanListener.java | 12 +-- ...lientEndpointLocatorConfigurationTest.java | 89 ++++++++++++++++++ .../zipkin/HttpZipkinSpanReporterTest.java | 4 +- .../ServerPropertiesEndpointLocatorTests.java | 34 +++++-- 20 files changed, 426 insertions(+), 72 deletions(-) create mode 100644 spring-cloud-sleuth-stream/src/test/java/org/springframework/cloud/sleuth/stream/DiscoveryClientEndpointLocatorConfigurationTest.java create mode 100644 spring-cloud-sleuth-zipkin/src/test/java/org/springframework/cloud/sleuth/zipkin/DiscoveryClientEndpointLocatorConfigurationTest.java diff --git a/docs/src/main/asciidoc/spring-cloud-sleuth.adoc b/docs/src/main/asciidoc/spring-cloud-sleuth.adoc index 446e24ca1..6f07f6a07 100644 --- a/docs/src/main/asciidoc/spring-cloud-sleuth.adoc +++ b/docs/src/main/asciidoc/spring-cloud-sleuth.adoc @@ -305,6 +305,21 @@ By default Sleuth assumes that when you send a span to Zipkin, you want the span spring.zipkin.service.name: foo ---- +=== Host locator + +In order to define the host that is corresponding to a particular span we need to resolve the host name +and port. The default approach is to take it from server properties. If those for some reason are not set +then we're trying to retrieve the host name from the network interfaces. + +If you have the discovery client enabled and prefer to retrieve the host address from the registered +instance in a service registry then you have to set the property (it's applicable for both HTTP and +Stream based span reporting). + +[source,yaml] +---- +spring.zipkin.locator.discovery.enabled: true +---- + == Span Data as Messages You can accumulate and send span data over diff --git a/spring-cloud-sleuth-stream/pom.xml b/spring-cloud-sleuth-stream/pom.xml index 33759d78e..2fac45dd4 100644 --- a/spring-cloud-sleuth-stream/pom.xml +++ b/spring-cloud-sleuth-stream/pom.xml @@ -27,7 +27,6 @@ org.springframework.cloud spring-cloud-commons - true org.springframework.boot diff --git a/spring-cloud-sleuth-stream/src/main/java/org/springframework/cloud/sleuth/stream/DiscoveryClientHostLocator.java b/spring-cloud-sleuth-stream/src/main/java/org/springframework/cloud/sleuth/stream/DiscoveryClientHostLocator.java index 86b618f6e..172c80eb0 100644 --- a/spring-cloud-sleuth-stream/src/main/java/org/springframework/cloud/sleuth/stream/DiscoveryClientHostLocator.java +++ b/spring-cloud-sleuth-stream/src/main/java/org/springframework/cloud/sleuth/stream/DiscoveryClientHostLocator.java @@ -52,8 +52,8 @@ public class DiscoveryClientHostLocator implements HostLocator { @Override public Host locate(Span span) { ServiceInstance instance = this.client.getLocalServiceInstance(); - String serviceId = StringUtils.hasText(this.zipkinProperties.getName()) ? - this.zipkinProperties.getName() : instance.getServiceId(); + String serviceId = StringUtils.hasText(this.zipkinProperties.getService().getName()) ? + this.zipkinProperties.getService().getName() : instance.getServiceId(); return new Host(serviceId, getIpAddress(instance), instance.getPort()); } diff --git a/spring-cloud-sleuth-stream/src/main/java/org/springframework/cloud/sleuth/stream/ServerPropertiesHostLocator.java b/spring-cloud-sleuth-stream/src/main/java/org/springframework/cloud/sleuth/stream/ServerPropertiesHostLocator.java index 37e33a582..1a9c90f4d 100644 --- a/spring-cloud-sleuth-stream/src/main/java/org/springframework/cloud/sleuth/stream/ServerPropertiesHostLocator.java +++ b/spring-cloud-sleuth-stream/src/main/java/org/springframework/cloud/sleuth/stream/ServerPropertiesHostLocator.java @@ -16,8 +16,14 @@ package org.springframework.cloud.sleuth.stream; +import java.lang.invoke.MethodHandles; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; import org.springframework.boot.autoconfigure.web.ServerProperties; import org.springframework.boot.context.embedded.EmbeddedServletContainerInitializedEvent; +import org.springframework.cloud.commons.util.InetUtils; +import org.springframework.cloud.commons.util.InetUtilsProperties; import org.springframework.cloud.sleuth.Span; import org.springframework.context.event.EventListener; import org.springframework.util.Assert; @@ -32,30 +38,37 @@ import org.springframework.util.StringUtils; *
  • port - from lazily assigned port or {@link ServerProperties}
    • * * - * You can override the value of service id by {@link ZipkinProperties#setName(String)} + * You can override the value of service id by {@link ZipkinProperties#getService()} * * @author Dave Syer * @since 1.0.0 */ public class ServerPropertiesHostLocator implements HostLocator { + private static final Log log = LogFactory.getLog(MethodHandles.lookup().lookupClass()); + private final ServerProperties serverProperties; // Nullable private final String appName; + private final InetUtils inetUtils; private final ZipkinProperties zipkinProperties; private Integer port; // Lazy assigned @Deprecated - public ServerPropertiesHostLocator(ServerProperties serverProperties, - String appName) { - this(serverProperties, appName, new ZipkinProperties()); + public ServerPropertiesHostLocator(ServerProperties serverProperties, String appName) { + this(serverProperties, appName, new ZipkinProperties(),null); } - public ServerPropertiesHostLocator(ServerProperties serverProperties, - String appName, ZipkinProperties zipkinProperties) { + public ServerPropertiesHostLocator(ServerProperties serverProperties, String appName, + ZipkinProperties zipkinProperties, InetUtils inetUtils) { this.serverProperties = serverProperties; this.appName = appName; Assert.notNull(this.appName, "appName"); this.zipkinProperties = zipkinProperties; + if (inetUtils == null) { + this.inetUtils = new InetUtils(new InetUtilsProperties()); + } else { + this.inetUtils = inetUtils; + } } @Override @@ -91,21 +104,24 @@ public class ServerPropertiesHostLocator implements HostLocator { address = this.serverProperties.getAddress().getHostAddress(); } else { - address = "127.0.0.1"; + address = this.inetUtils.findFirstNonLoopbackAddress().getHostAddress(); } return address; } private String getServiceName(Span span) { String serviceName; - if (StringUtils.hasText(this.zipkinProperties.getName())) { - serviceName = this.zipkinProperties.getName(); + if (StringUtils.hasText(this.zipkinProperties.getService().getName())) { + serviceName = this.zipkinProperties.getService().getName(); } else if (span.getProcessId() != null) { serviceName = span.getProcessId(); } else { serviceName = this.appName; } + if (log.isDebugEnabled()) { + log.debug("Span will contain serviceName [" + serviceName + "]"); + } return serviceName; } diff --git a/spring-cloud-sleuth-stream/src/main/java/org/springframework/cloud/sleuth/stream/SleuthStreamAutoConfiguration.java b/spring-cloud-sleuth-stream/src/main/java/org/springframework/cloud/sleuth/stream/SleuthStreamAutoConfiguration.java index cd71a167a..eb027ff00 100644 --- a/spring-cloud-sleuth-stream/src/main/java/org/springframework/cloud/sleuth/stream/SleuthStreamAutoConfiguration.java +++ b/spring-cloud-sleuth-stream/src/main/java/org/springframework/cloud/sleuth/stream/SleuthStreamAutoConfiguration.java @@ -22,11 +22,11 @@ import org.springframework.boot.autoconfigure.AutoConfigureAfter; import org.springframework.boot.autoconfigure.AutoConfigureBefore; import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; -import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.boot.autoconfigure.web.ServerProperties; import org.springframework.boot.context.properties.EnableConfigurationProperties; import org.springframework.cloud.client.discovery.DiscoveryClient; +import org.springframework.cloud.commons.util.InetUtils; import org.springframework.cloud.sleuth.Sampler; import org.springframework.cloud.sleuth.metric.SpanMetricReporter; import org.springframework.cloud.sleuth.metric.TraceMetricsAutoConfiguration; @@ -91,7 +91,8 @@ public class SleuthStreamAutoConfiguration { } @Configuration - @ConditionalOnMissingClass("org.springframework.cloud.client.discovery.DiscoveryClient") + @ConditionalOnMissingBean(HostLocator.class) + @ConditionalOnProperty(value = "spring.zipkin.locator.discovery.enabled", havingValue = "false", matchIfMissing = true) protected static class DefaultEndpointLocatorConfiguration { @Autowired(required = false) @@ -100,18 +101,24 @@ public class SleuthStreamAutoConfiguration { @Autowired private ZipkinProperties zipkinProperties; + @Autowired + private InetUtils inetUtils; + @Value("${spring.application.name:unknown}") private String appName; @Bean public HostLocator zipkinEndpointLocator() { - return new ServerPropertiesHostLocator(this.serverProperties, this.appName, this.zipkinProperties); + return new ServerPropertiesHostLocator(this.serverProperties, this.appName, this.zipkinProperties, + this.inetUtils); } } @Configuration @ConditionalOnClass(DiscoveryClient.class) + @ConditionalOnMissingBean(HostLocator.class) + @ConditionalOnProperty(value = "spring.zipkin.locator.discovery.enabled", havingValue = "true") protected static class DiscoveryClientEndpointLocatorConfiguration { @Autowired(required = false) @@ -120,6 +127,9 @@ public class SleuthStreamAutoConfiguration { @Autowired private ZipkinProperties zipkinProperties; + @Autowired(required = false) + private InetUtils inetUtils; + @Value("${spring.application.name:unknown}") private String appName; @@ -131,7 +141,8 @@ public class SleuthStreamAutoConfiguration { if (this.client != null) { return new DiscoveryClientHostLocator(this.client, this.zipkinProperties); } - return new ServerPropertiesHostLocator(this.serverProperties, this.appName, this.zipkinProperties); + return new ServerPropertiesHostLocator(this.serverProperties, this.appName, this.zipkinProperties, + this.inetUtils); } } diff --git a/spring-cloud-sleuth-stream/src/main/java/org/springframework/cloud/sleuth/stream/ZipkinProperties.java b/spring-cloud-sleuth-stream/src/main/java/org/springframework/cloud/sleuth/stream/ZipkinProperties.java index 700ee1023..908de229c 100644 --- a/spring-cloud-sleuth-stream/src/main/java/org/springframework/cloud/sleuth/stream/ZipkinProperties.java +++ b/spring-cloud-sleuth-stream/src/main/java/org/springframework/cloud/sleuth/stream/ZipkinProperties.java @@ -24,17 +24,66 @@ import org.springframework.boot.context.properties.ConfigurationProperties; * @author Marcin Grzejszczak * @since 1.0.12 */ -@ConfigurationProperties("spring.zipkin.service") +@ConfigurationProperties("spring.zipkin") public class ZipkinProperties { - /** The name of the service, from which the Span was sent via Stream, that should appear in Zipkin */ - private String name; + private Service service = new Service(); - public String getName() { - return this.name; + private Locator locator = new Locator(); + + public Service getService() { + return this.service; } - public void setName(String name) { - this.name = name; + public void setService(Service service) { + this.service = service; + } + + public Locator getLocator() { + return this.locator; + } + + public void setLocator(Locator locator) { + this.locator = locator; + } + + public static class Service { + /** The name of the service, from which the Span was sent via Stream, that should appear in Zipkin */ + private String name; + + public String getName() { + return this.name; + } + + public void setName(String name) { + this.name = name; + } + } + + public static class Locator { + + private Discovery discovery; + + public Discovery getDiscovery() { + return this.discovery; + } + + public void setDiscovery(Discovery discovery) { + this.discovery = discovery; + } + + public static class Discovery { + + /** Enabling of locating the host name via service discovery */ + private boolean enabled; + + public boolean isEnabled() { + return this.enabled; + } + + public void setEnabled(boolean enabled) { + this.enabled = enabled; + } + } } } diff --git a/spring-cloud-sleuth-stream/src/test/java/org/springframework/cloud/sleuth/stream/DiscoveryClientEndpointLocatorConfigurationTest.java b/spring-cloud-sleuth-stream/src/test/java/org/springframework/cloud/sleuth/stream/DiscoveryClientEndpointLocatorConfigurationTest.java new file mode 100644 index 000000000..c01dd1e16 --- /dev/null +++ b/spring-cloud-sleuth-stream/src/test/java/org/springframework/cloud/sleuth/stream/DiscoveryClientEndpointLocatorConfigurationTest.java @@ -0,0 +1,94 @@ +package org.springframework.cloud.sleuth.stream; + +import org.junit.Test; +import org.mockito.Mockito; +import org.springframework.boot.SpringApplication; +import org.springframework.boot.autoconfigure.EnableAutoConfiguration; +import org.springframework.cloud.client.discovery.DiscoveryClient; +import org.springframework.context.ConfigurableApplicationContext; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * @author Matcin Wielgus + */ +public class DiscoveryClientEndpointLocatorConfigurationTest { + @Test + public void endpointLocatorShouldDefaultToServerPropertiesEndpointLocator() { + try (ConfigurableApplicationContext ctxt = new SpringApplication( + EmptyConfiguration.class).run("--spring.main.web_environment=false")) { + assertThat(ctxt.getBean(HostLocator.class)) + .isInstanceOf(ServerPropertiesHostLocator.class); + } + } + + @Test + public void endpointLocatorShouldDefaultToServerPropertiesEndpointLocatorEvenWhenDiscoveryClientPresent() { + try (ConfigurableApplicationContext ctxt = new SpringApplication( + ConfigurationWithDiscoveryClient.class) + .run("--spring.main.web_environment=false")) { + assertThat(ctxt.getBean(HostLocator.class)) + .isInstanceOf(ServerPropertiesHostLocator.class); + } + } + + @Test + public void endpointLocatorShouldRespectExistingEndpointLocator() { + try (ConfigurableApplicationContext ctxt = new SpringApplication( + ConfigurationWithCustomLocator.class) + .run("--spring.main.web_environment=false")) { + assertThat(ctxt.getBean(HostLocator.class)) + .isSameAs(ConfigurationWithCustomLocator.locator); + } + } + + @Test + public void endpointLocatorShouldBeFallbackHavingEndpointLocatorWhenAskedTo() { + try (ConfigurableApplicationContext ctxt = new SpringApplication( + ConfigurationWithDiscoveryClient.class).run( + "--spring.zipkin.locator.discovery.enabled=true", + "--spring.main.web_environment=false")) { + assertThat(ctxt.getBean(HostLocator.class)) + .isInstanceOf(DiscoveryClientHostLocator.class); + } + } + + @Test + public void endpointLocatorShouldRespectExistingEndpointLocatorEvenWhenAskedToBeDiscovery() { + try (ConfigurableApplicationContext ctxt = new SpringApplication( + ConfigurationWithDiscoveryClient.class, + ConfigurationWithCustomLocator.class).run( + "--spring.zipkin.locator.discovery.enabled=true", + "--spring.main.web_environment=false")) { + assertThat(ctxt.getBean(HostLocator.class)) + .isSameAs(ConfigurationWithCustomLocator.locator); + } + } + + @Configuration + @EnableAutoConfiguration + public static class EmptyConfiguration { + } + + @Configuration + @EnableAutoConfiguration + public static class ConfigurationWithDiscoveryClient { + @Bean + public DiscoveryClient getDiscoveryClient() { + return Mockito.mock(DiscoveryClient.class); + } + } + + @Configuration + @EnableAutoConfiguration + public static class ConfigurationWithCustomLocator { + static HostLocator locator = Mockito.mock(HostLocator.class); + + @Bean + public HostLocator getEndpointLocator() { + return locator; + } + } +} \ No newline at end of file diff --git a/spring-cloud-sleuth-stream/src/test/java/org/springframework/cloud/sleuth/stream/DiscoveryClientHostLocatorTest.java b/spring-cloud-sleuth-stream/src/test/java/org/springframework/cloud/sleuth/stream/DiscoveryClientHostLocatorTest.java index 46f4bf72d..b596eed16 100644 --- a/spring-cloud-sleuth-stream/src/test/java/org/springframework/cloud/sleuth/stream/DiscoveryClientHostLocatorTest.java +++ b/spring-cloud-sleuth-stream/src/test/java/org/springframework/cloud/sleuth/stream/DiscoveryClientHostLocatorTest.java @@ -67,7 +67,7 @@ public class DiscoveryClientHostLocatorTest { public void should_override_the_service_name_from_properties() throws Exception { given(this.discoveryClient.getLocalServiceInstance()).willReturn(serviceInstanceWithValidHost()); ZipkinProperties zipkinProperties = new ZipkinProperties(); - zipkinProperties.setName("foo"); + zipkinProperties.getService().setName("foo"); this.discoveryClientHostLocator = new DiscoveryClientHostLocator(this.discoveryClient, zipkinProperties); Host host = this.discoveryClientHostLocator.locate(null); diff --git a/spring-cloud-sleuth-stream/src/test/java/org/springframework/cloud/sleuth/stream/ServerPropertiesHostLocatorTests.java b/spring-cloud-sleuth-stream/src/test/java/org/springframework/cloud/sleuth/stream/ServerPropertiesHostLocatorTests.java index a63c02268..22ca4ea5c 100644 --- a/spring-cloud-sleuth-stream/src/test/java/org/springframework/cloud/sleuth/stream/ServerPropertiesHostLocatorTests.java +++ b/spring-cloud-sleuth-stream/src/test/java/org/springframework/cloud/sleuth/stream/ServerPropertiesHostLocatorTests.java @@ -21,49 +21,57 @@ import java.net.UnknownHostException; import java.util.Collections; import org.junit.Test; +import org.mockito.Mockito; import org.springframework.boot.autoconfigure.web.ServerProperties; +import org.springframework.cloud.commons.util.InetUtils; +import org.springframework.cloud.commons.util.InetUtilsProperties; import org.springframework.cloud.sleuth.Span; import static org.assertj.core.api.Assertions.assertThat; public class ServerPropertiesHostLocatorTests { + + public static final byte[] ADR1234 = { 1, 2, 3, 4 }; + Span span = new Span(1, 3, "http:name", 1L, Collections.emptyList(), 2L, true, true, "process"); @Test - public void portDefaultsTo8080() { + public void portDefaultsTo8080() throws UnknownHostException { ServerPropertiesHostLocator locator = new ServerPropertiesHostLocator( - new ServerProperties(), "unknown", new ZipkinProperties()); + new ServerProperties(), "unknown", new ZipkinProperties(), + localAddress(ADR1234)); assertThat(locator.locate(this.span).getPort()).isEqualTo((short) 8080); } @Test - public void portFromServerProperties() { + public void portFromServerProperties() throws UnknownHostException { ServerProperties properties = new ServerProperties(); properties.setPort(1234); ServerPropertiesHostLocator locator = new ServerPropertiesHostLocator(properties, - "unknown", new ZipkinProperties()); + "unknown", new ZipkinProperties(),localAddress(ADR1234)); assertThat(locator.locate(this.span).getPort()).isEqualTo((short) 1234); } @Test - public void portDefaultsToLocalhost() { + public void portDefaultsToLocalhost() throws UnknownHostException { ServerPropertiesHostLocator locator = new ServerPropertiesHostLocator( - new ServerProperties(), "unknown", new ZipkinProperties()); + new ServerProperties(), "unknown", new ZipkinProperties(), + localAddress(ADR1234)); - assertThat(locator.locate(this.span).getAddress()).isEqualTo("127.0.0.1"); + assertThat(locator.locate(this.span).getAddress()).isEqualTo("1.2.3.4"); } @Test public void hostFromServerPropertiesIp() throws UnknownHostException { ServerProperties properties = new ServerProperties(); - properties.setAddress(InetAddress.getByAddress(new byte[] { 1, 2, 3, 4 })); + properties.setAddress(InetAddress.getByAddress(ADR1234)); ServerPropertiesHostLocator locator = new ServerPropertiesHostLocator(properties, - "unknown", new ZipkinProperties()); + "unknown", new ZipkinProperties(),localAddress(new byte[] { 1, 1, 1, 1 })); assertThat(locator.locate(this.span).getAddress()).isEqualTo("1.2.3.4"); } @@ -73,11 +81,18 @@ public class ServerPropertiesHostLocatorTests { ServerProperties properties = new ServerProperties(); properties.setAddress(InetAddress.getByAddress(new byte[] { 1, 2, 3, 4 })); ZipkinProperties zipkinProperties = new ZipkinProperties(); - zipkinProperties.setName("foo"); + zipkinProperties.getService().setName("foo"); ServerPropertiesHostLocator locator = new ServerPropertiesHostLocator(properties, - "unknown", zipkinProperties); + "unknown", zipkinProperties,localAddress(ADR1234)); assertThat(locator.locate(this.span).getServiceName()).isEqualTo("foo"); } + + private InetUtils localAddress(byte[] address) throws UnknownHostException { + InetUtils mocked = Mockito.spy(new InetUtils(new InetUtilsProperties())); + Mockito.when(mocked.findFirstNonLoopbackAddress()) + .thenReturn(InetAddress.getByAddress(address)); + return mocked; + } } diff --git a/spring-cloud-sleuth-stream/src/test/java/org/springframework/cloud/sleuth/stream/SleuthStreamAutoConfigurationTest.java b/spring-cloud-sleuth-stream/src/test/java/org/springframework/cloud/sleuth/stream/SleuthStreamAutoConfigurationTest.java index 3e0d02bf1..51cfab3bf 100644 --- a/spring-cloud-sleuth-stream/src/test/java/org/springframework/cloud/sleuth/stream/SleuthStreamAutoConfigurationTest.java +++ b/spring-cloud-sleuth-stream/src/test/java/org/springframework/cloud/sleuth/stream/SleuthStreamAutoConfigurationTest.java @@ -10,6 +10,7 @@ import org.junit.BeforeClass; import org.junit.Test; import org.springframework.boot.autoconfigure.PropertyPlaceholderAutoConfiguration; import org.springframework.boot.test.EnvironmentTestUtils; +import org.springframework.cloud.commons.util.UtilAutoConfiguration; import org.springframework.cloud.sleuth.autoconfig.TraceAutoConfiguration; import org.springframework.cloud.sleuth.log.NoOpSpanLogger; import org.springframework.cloud.sleuth.log.SpanLogger; @@ -120,7 +121,7 @@ public class SleuthStreamAutoConfigurationTest { @Import({ SleuthStreamAutoConfiguration.class, TraceMetricsAutoConfiguration.class, TestSupportBinderAutoConfiguration.class, ChannelBindingAutoConfiguration.class, TraceAutoConfiguration.class, - PropertyPlaceholderAutoConfiguration.class }) + PropertyPlaceholderAutoConfiguration.class, UtilAutoConfiguration.class }) public static class BaseConfiguration { @Bean SpanLogger spanLogger() { diff --git a/spring-cloud-sleuth-stream/src/test/java/org/springframework/cloud/sleuth/stream/StreamSpanListenerTests.java b/spring-cloud-sleuth-stream/src/test/java/org/springframework/cloud/sleuth/stream/StreamSpanListenerTests.java index c99df54f0..f26bb6399 100644 --- a/spring-cloud-sleuth-stream/src/test/java/org/springframework/cloud/sleuth/stream/StreamSpanListenerTests.java +++ b/spring-cloud-sleuth-stream/src/test/java/org/springframework/cloud/sleuth/stream/StreamSpanListenerTests.java @@ -33,6 +33,7 @@ import org.mockito.Mockito; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.actuate.metrics.CounterService; import org.springframework.boot.autoconfigure.PropertyPlaceholderAutoConfiguration; +import org.springframework.cloud.commons.util.UtilAutoConfiguration; import org.springframework.cloud.sleuth.Sampler; import org.springframework.cloud.sleuth.Span; import org.springframework.cloud.sleuth.SpanReporter; @@ -158,7 +159,7 @@ public class StreamSpanListenerTests { @Import({ ZipkinTestConfiguration.class, SleuthStreamAutoConfiguration.class, TraceMetricsAutoConfiguration.class, TestSupportBinderAutoConfiguration.class, ChannelBindingAutoConfiguration.class, TraceAutoConfiguration.class, - PropertyPlaceholderAutoConfiguration.class }) + PropertyPlaceholderAutoConfiguration.class, UtilAutoConfiguration.class }) protected static class TestConfiguration { } diff --git a/spring-cloud-sleuth-zipkin/pom.xml b/spring-cloud-sleuth-zipkin/pom.xml index 1c2eff394..d2c1f579e 100644 --- a/spring-cloud-sleuth-zipkin/pom.xml +++ b/spring-cloud-sleuth-zipkin/pom.xml @@ -40,7 +40,6 @@ org.springframework.cloud spring-cloud-commons - true org.springframework.boot diff --git a/spring-cloud-sleuth-zipkin/src/main/java/org/springframework/cloud/sleuth/zipkin/FallbackHavingEndpointLocator.java b/spring-cloud-sleuth-zipkin/src/main/java/org/springframework/cloud/sleuth/zipkin/FallbackHavingEndpointLocator.java index 5ac4ca5d2..e9c58805d 100644 --- a/spring-cloud-sleuth-zipkin/src/main/java/org/springframework/cloud/sleuth/zipkin/FallbackHavingEndpointLocator.java +++ b/spring-cloud-sleuth-zipkin/src/main/java/org/springframework/cloud/sleuth/zipkin/FallbackHavingEndpointLocator.java @@ -1,7 +1,5 @@ package org.springframework.cloud.sleuth.zipkin; -import java.util.concurrent.atomic.AtomicReference; - import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import zipkin.Endpoint; @@ -14,8 +12,6 @@ import zipkin.Endpoint; */ public class FallbackHavingEndpointLocator implements EndpointLocator { - private final AtomicReference cachedEndpoint = new AtomicReference<>(); - private static final Log log = LogFactory.getLog(FallbackHavingEndpointLocator.class); private final DiscoveryClientEndpointLocator discoveryClientEndpointLocator; @@ -29,8 +25,7 @@ public class FallbackHavingEndpointLocator implements EndpointLocator { @Override public Endpoint local() { - this.cachedEndpoint.compareAndSet(null, endpoint()); - return this.cachedEndpoint.get(); + return endpoint(); } private Endpoint endpoint() { diff --git a/spring-cloud-sleuth-zipkin/src/main/java/org/springframework/cloud/sleuth/zipkin/ServerPropertiesEndpointLocator.java b/spring-cloud-sleuth-zipkin/src/main/java/org/springframework/cloud/sleuth/zipkin/ServerPropertiesEndpointLocator.java index b148b5ff9..8749914a9 100644 --- a/spring-cloud-sleuth-zipkin/src/main/java/org/springframework/cloud/sleuth/zipkin/ServerPropertiesEndpointLocator.java +++ b/spring-cloud-sleuth-zipkin/src/main/java/org/springframework/cloud/sleuth/zipkin/ServerPropertiesEndpointLocator.java @@ -17,12 +17,14 @@ package org.springframework.cloud.sleuth.zipkin; import java.lang.invoke.MethodHandles; +import java.nio.ByteBuffer; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.springframework.boot.autoconfigure.web.ServerProperties; import org.springframework.boot.context.embedded.EmbeddedServletContainerInitializedEvent; import org.springframework.cloud.commons.util.InetUtils; +import org.springframework.cloud.commons.util.InetUtilsProperties; import org.springframework.context.event.EventListener; import org.springframework.util.StringUtils; @@ -47,20 +49,25 @@ public class ServerPropertiesEndpointLocator implements EndpointLocator { private final ServerProperties serverProperties; private final String appName; + private final InetUtils inetUtils; private final ZipkinProperties zipkinProperties; private Integer port; @Deprecated - public ServerPropertiesEndpointLocator(ServerProperties serverProperties, - String appName) { - this(serverProperties, appName, new ZipkinProperties()); + public ServerPropertiesEndpointLocator(ServerProperties serverProperties,String appName) { + this(serverProperties,appName,new ZipkinProperties(), null); } public ServerPropertiesEndpointLocator(ServerProperties serverProperties, - String appName, ZipkinProperties zipkinProperties) { + String appName, ZipkinProperties zipkinProperties, InetUtils inetUtils) { this.serverProperties = serverProperties; this.appName = appName; this.zipkinProperties = zipkinProperties; + if (inetUtils == null) { + this.inetUtils = new InetUtils(new InetUtilsProperties()); + } else { + this.inetUtils = inetUtils; + } } @Override @@ -97,11 +104,12 @@ public class ServerPropertiesEndpointLocator implements EndpointLocator { } private int getAddress() { - if (this.serverProperties!=null && this.serverProperties.getAddress() != null) { - return InetUtils.getIpAddressAsInt(this.serverProperties.getAddress().getHostAddress()); + if (this.serverProperties != null && this.serverProperties.getAddress() != null) { + return ByteBuffer.wrap(this.serverProperties.getAddress().getAddress()) + .getInt(); } else { - return 127 << 24 | 1; + return ByteBuffer.wrap(this.inetUtils.findFirstNonLoopbackAddress().getAddress()).getInt(); } } } diff --git a/spring-cloud-sleuth-zipkin/src/main/java/org/springframework/cloud/sleuth/zipkin/ZipkinAutoConfiguration.java b/spring-cloud-sleuth-zipkin/src/main/java/org/springframework/cloud/sleuth/zipkin/ZipkinAutoConfiguration.java index 829f7aa33..01082bdb1 100644 --- a/spring-cloud-sleuth-zipkin/src/main/java/org/springframework/cloud/sleuth/zipkin/ZipkinAutoConfiguration.java +++ b/spring-cloud-sleuth-zipkin/src/main/java/org/springframework/cloud/sleuth/zipkin/ZipkinAutoConfiguration.java @@ -21,11 +21,11 @@ import org.springframework.beans.factory.annotation.Value; import org.springframework.boot.autoconfigure.AutoConfigureBefore; import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; -import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.boot.autoconfigure.web.ServerProperties; import org.springframework.boot.context.properties.EnableConfigurationProperties; import org.springframework.cloud.client.discovery.DiscoveryClient; +import org.springframework.cloud.commons.util.InetUtils; import org.springframework.cloud.sleuth.Sampler; import org.springframework.cloud.sleuth.SpanReporter; import org.springframework.cloud.sleuth.autoconfig.TraceAutoConfiguration; @@ -70,7 +70,8 @@ public class ZipkinAutoConfiguration { } @Configuration - @ConditionalOnMissingClass("org.springframework.cloud.client.discovery.DiscoveryClient") + @ConditionalOnMissingBean(EndpointLocator.class) + @ConditionalOnProperty(value = "spring.zipkin.locator.discovery.enabled", havingValue = "false", matchIfMissing = true) protected static class DefaultEndpointLocatorConfiguration { @Autowired(required=false) @@ -79,19 +80,24 @@ public class ZipkinAutoConfiguration { @Autowired private ZipkinProperties zipkinProperties; + @Autowired(required=false) + private InetUtils inetUtils; + @Value("${spring.application.name:unknown}") private String appName; @Bean public EndpointLocator zipkinEndpointLocator() { return new ServerPropertiesEndpointLocator(this.serverProperties, this.appName, - this.zipkinProperties); + this.zipkinProperties, this.inetUtils); } } @Configuration @ConditionalOnClass(DiscoveryClient.class) + @ConditionalOnMissingBean(EndpointLocator.class) + @ConditionalOnProperty(value = "spring.zipkin.locator.discovery.enabled", havingValue = "true") protected static class DiscoveryClientEndpointLocatorConfiguration { @Autowired(required=false) @@ -100,6 +106,9 @@ public class ZipkinAutoConfiguration { @Autowired private ZipkinProperties zipkinProperties; + @Autowired(required=false) + private InetUtils inetUtils; + @Value("${spring.application.name:unknown}") private String appName; @@ -110,7 +119,7 @@ public class ZipkinAutoConfiguration { public EndpointLocator zipkinEndpointLocator() { return new FallbackHavingEndpointLocator(discoveryClientEndpointLocator(), new ServerPropertiesEndpointLocator(this.serverProperties, this.appName, - this.zipkinProperties)); + this.zipkinProperties, this.inetUtils)); } private DiscoveryClientEndpointLocator discoveryClientEndpointLocator() { diff --git a/spring-cloud-sleuth-zipkin/src/main/java/org/springframework/cloud/sleuth/zipkin/ZipkinProperties.java b/spring-cloud-sleuth-zipkin/src/main/java/org/springframework/cloud/sleuth/zipkin/ZipkinProperties.java index 1a27fd047..1144d0d0c 100644 --- a/spring-cloud-sleuth-zipkin/src/main/java/org/springframework/cloud/sleuth/zipkin/ZipkinProperties.java +++ b/spring-cloud-sleuth-zipkin/src/main/java/org/springframework/cloud/sleuth/zipkin/ZipkinProperties.java @@ -34,6 +34,12 @@ public class ZipkinProperties { private Service service = new Service(); + private Locator locator = new Locator(); + + public Locator getLocator() { + return this.locator; + } + public String getBaseUrl() { return this.baseUrl; } @@ -74,6 +80,10 @@ public class ZipkinProperties { this.service = service; } + public void setLocator(Locator locator) { + this.locator = locator; + } + /** When enabled, spans are gzipped before sent to the zipkin server */ public static class Compression { @@ -102,4 +112,31 @@ public class ZipkinProperties { this.name = name; } } + + public static class Locator { + + private Discovery discovery; + + public Discovery getDiscovery() { + return this.discovery; + } + + public void setDiscovery(Discovery discovery) { + this.discovery = discovery; + } + + public static class Discovery { + + /** Enabling of locating the host name via service discovery */ + private boolean enabled; + + public boolean isEnabled() { + return this.enabled; + } + + public void setEnabled(boolean enabled) { + this.enabled = enabled; + } + } + } } diff --git a/spring-cloud-sleuth-zipkin/src/main/java/org/springframework/cloud/sleuth/zipkin/ZipkinSpanListener.java b/spring-cloud-sleuth-zipkin/src/main/java/org/springframework/cloud/sleuth/zipkin/ZipkinSpanListener.java index f00aa22d3..4b33fdf91 100644 --- a/spring-cloud-sleuth-zipkin/src/main/java/org/springframework/cloud/sleuth/zipkin/ZipkinSpanListener.java +++ b/spring-cloud-sleuth-zipkin/src/main/java/org/springframework/cloud/sleuth/zipkin/ZipkinSpanListener.java @@ -119,7 +119,7 @@ public class ZipkinSpanListener implements SpanReporter { return zipkinSpan.build(); } - private void ensureLocalComponent(Span span, zipkin.Span.Builder zipkinSpan) { + private void ensureLocalComponent(Span span, zipkin.Span.Builder zipkinSpan, Endpoint localEndpoint) { if (span.tags().containsKey(Constants.LOCAL_COMPONENT)) { return; } @@ -130,14 +130,14 @@ public class ZipkinSpanListener implements SpanReporter { .type(BinaryAnnotation.Type.STRING) .key("lc") // LOCAL_COMPONENT .value(processId) - .endpoint(this.endpointLocator.local()).build(); + .endpoint(localEndpoint).build(); zipkinSpan.addBinaryAnnotation(component); } - private void ensureServerAddr(Span span, zipkin.Span.Builder zipkinSpan) { + private void ensureServerAddr(Span span, zipkin.Span.Builder zipkinSpan, Endpoint localEndpoint) { if (span.tags().containsKey(Span.SPAN_PEER_SERVICE_TAG_NAME)) { zipkinSpan.addBinaryAnnotation(BinaryAnnotation.address(Constants.SERVER_ADDR, - this.endpointLocator.local().toBuilder().serviceName( + localEndpoint.toBuilder().serviceName( span.tags().get(Span.SPAN_PEER_SERVICE_TAG_NAME)).build())); } } @@ -160,10 +160,10 @@ public class ZipkinSpanListener implements SpanReporter { } if (notClientOrServer) { // A zipkin span without any annotations cannot be queried, add special "lc" to avoid that. - ensureLocalComponent(span, zipkinSpan); + ensureLocalComponent(span, zipkinSpan, endpoint); } if (hasClientSend) { - ensureServerAddr(span, zipkinSpan); + ensureServerAddr(span, zipkinSpan, endpoint); } if (instanceIdToTag && this.environment != null) { setInstanceIdIfPresent(zipkinSpan, endpoint, Span.INSTANCEID); diff --git a/spring-cloud-sleuth-zipkin/src/test/java/org/springframework/cloud/sleuth/zipkin/DiscoveryClientEndpointLocatorConfigurationTest.java b/spring-cloud-sleuth-zipkin/src/test/java/org/springframework/cloud/sleuth/zipkin/DiscoveryClientEndpointLocatorConfigurationTest.java new file mode 100644 index 000000000..510d18899 --- /dev/null +++ b/spring-cloud-sleuth-zipkin/src/test/java/org/springframework/cloud/sleuth/zipkin/DiscoveryClientEndpointLocatorConfigurationTest.java @@ -0,0 +1,89 @@ +package org.springframework.cloud.sleuth.zipkin; + +import org.junit.Test; +import org.mockito.Mockito; +import org.springframework.boot.SpringApplication; +import org.springframework.boot.autoconfigure.EnableAutoConfiguration; +import org.springframework.cloud.client.discovery.DiscoveryClient; +import org.springframework.context.ConfigurableApplicationContext; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * @author Matcin Wielgus + */ +public class DiscoveryClientEndpointLocatorConfigurationTest { + + @Test + public void endpointLocatorShouldDefaultToServerPropertiesEndpointLocator() { + ConfigurableApplicationContext ctxt = new SpringApplication( + EmptyConfiguration.class).run(); + assertThat(ctxt.getBean(EndpointLocator.class)) + .isInstanceOf(ServerPropertiesEndpointLocator.class); + ctxt.close(); + } + + @Test + public void endpointLocatorShouldDefaultToServerPropertiesEndpointLocatorEvenWhenDiscoveryClientPresent() { + ConfigurableApplicationContext ctxt = new SpringApplication( + ConfigurationWithDiscoveryClient.class).run(); + assertThat(ctxt.getBean(EndpointLocator.class)) + .isInstanceOf(ServerPropertiesEndpointLocator.class); + ctxt.close(); + } + + @Test + public void endpointLocatorShouldRespectExistingEndpointLocator() { + ConfigurableApplicationContext ctxt = new SpringApplication( + ConfigurationWithCustomLocator.class).run(); + assertThat(ctxt.getBean(EndpointLocator.class)) + .isSameAs(ConfigurationWithCustomLocator.locator); + ctxt.close(); + } + + @Test + public void endpointLocatorShouldBeFallbackHavingEndpointLocatorWhenAskedTo() { + ConfigurableApplicationContext ctxt = new SpringApplication( + ConfigurationWithDiscoveryClient.class).run("--spring.zipkin.locator.discovery.enabled=true"); + assertThat(ctxt.getBean(EndpointLocator.class)) + .isInstanceOf(FallbackHavingEndpointLocator.class); + ctxt.close(); + } + + @Test + public void endpointLocatorShouldRespectExistingEndpointLocatorEvenWhenAskedToBeDiscovery() { + ConfigurableApplicationContext ctxt = new SpringApplication( + ConfigurationWithDiscoveryClient.class,ConfigurationWithCustomLocator.class).run("--spring.zipkin.locator.discovery.enabled=true"); + assertThat(ctxt.getBean(EndpointLocator.class)) + .isSameAs(ConfigurationWithCustomLocator.locator); + ctxt.close(); + } + + @Configuration + @EnableAutoConfiguration + public static class EmptyConfiguration { + } + + @Configuration + @EnableAutoConfiguration + public static class ConfigurationWithDiscoveryClient { + @Bean + public DiscoveryClient getDiscoveryClient() { + return Mockito.mock(DiscoveryClient.class); + } + } + + @Configuration + @EnableAutoConfiguration + public static class ConfigurationWithCustomLocator { + static EndpointLocator locator = Mockito.mock(EndpointLocator.class); + + @Bean + public EndpointLocator getEndpointLocator() { + return locator; + } + } + +} \ No newline at end of file diff --git a/spring-cloud-sleuth-zipkin/src/test/java/org/springframework/cloud/sleuth/zipkin/HttpZipkinSpanReporterTest.java b/spring-cloud-sleuth-zipkin/src/test/java/org/springframework/cloud/sleuth/zipkin/HttpZipkinSpanReporterTest.java index e5b0a9b1e..a7b9ddb82 100644 --- a/spring-cloud-sleuth-zipkin/src/test/java/org/springframework/cloud/sleuth/zipkin/HttpZipkinSpanReporterTest.java +++ b/spring-cloud-sleuth-zipkin/src/test/java/org/springframework/cloud/sleuth/zipkin/HttpZipkinSpanReporterTest.java @@ -6,6 +6,8 @@ import java.util.concurrent.atomic.AtomicReference; import org.junit.Rule; import org.junit.Test; import org.springframework.boot.autoconfigure.web.ServerProperties; +import org.springframework.cloud.commons.util.InetUtils; +import org.springframework.cloud.commons.util.InetUtilsProperties; import org.springframework.cloud.sleuth.DefaultSpanNamer; import org.springframework.cloud.sleuth.Tracer; import org.springframework.cloud.sleuth.log.NoOpSpanLogger; @@ -130,7 +132,7 @@ public class HttpZipkinSpanReporterTest { Tracer tracer = new DefaultTracer(new AlwaysSampler(), new Random(), new DefaultSpanNamer(), new NoOpSpanLogger(), new ZipkinSpanListener(receivedSpan::set, new ServerPropertiesEndpointLocator(new ServerProperties(), "foo", - new ZipkinProperties()))); + new ZipkinProperties(), new InetUtils(new InetUtilsProperties())))); // tag::service_name[] org.springframework.cloud.sleuth.Span newSpan = tracer.createSpan("redis"); try { diff --git a/spring-cloud-sleuth-zipkin/src/test/java/org/springframework/cloud/sleuth/zipkin/ServerPropertiesEndpointLocatorTests.java b/spring-cloud-sleuth-zipkin/src/test/java/org/springframework/cloud/sleuth/zipkin/ServerPropertiesEndpointLocatorTests.java index 2614388f5..92304542c 100644 --- a/spring-cloud-sleuth-zipkin/src/test/java/org/springframework/cloud/sleuth/zipkin/ServerPropertiesEndpointLocatorTests.java +++ b/spring-cloud-sleuth-zipkin/src/test/java/org/springframework/cloud/sleuth/zipkin/ServerPropertiesEndpointLocatorTests.java @@ -20,46 +20,53 @@ import java.net.InetAddress; import java.net.UnknownHostException; import org.junit.Test; +import org.mockito.Mockito; import org.springframework.boot.autoconfigure.web.ServerProperties; +import org.springframework.cloud.commons.util.InetUtils; +import org.springframework.cloud.commons.util.InetUtilsProperties; import static org.assertj.core.api.Assertions.assertThat; public class ServerPropertiesEndpointLocatorTests { + public static final byte[] ADDRESS1234 = { 1, 2, 3, 4 }; + @Test - public void portDefaultsTo8080() { + public void portDefaultsTo8080() throws UnknownHostException { ServerPropertiesEndpointLocator locator = new ServerPropertiesEndpointLocator( - new ServerProperties(), "unknown", new ZipkinProperties()); + new ServerProperties(), "unknown", new ZipkinProperties(), + localAddress(ADDRESS1234)); assertThat(locator.local().port).isEqualTo((short) 8080); } @Test - public void portFromServerProperties() { + public void portFromServerProperties() throws UnknownHostException { ServerProperties properties = new ServerProperties(); properties.setPort(1234); ServerPropertiesEndpointLocator locator = new ServerPropertiesEndpointLocator( - properties, "unknown", new ZipkinProperties()); + properties, "unknown", new ZipkinProperties(),localAddress(ADDRESS1234)); assertThat(locator.local().port).isEqualTo((short) 1234); } @Test - public void portDefaultsToLocalhost() { + public void portDefaultsToLocalhost() throws UnknownHostException { ServerPropertiesEndpointLocator locator = new ServerPropertiesEndpointLocator( - new ServerProperties(), "unknown", new ZipkinProperties()); + new ServerProperties(), "unknown", new ZipkinProperties(), localAddress(ADDRESS1234)); - assertThat(locator.local().ipv4).isEqualTo(127 << 24 | 1); + assertThat(locator.local().ipv4).isEqualTo(1 << 24 | 2 << 16 | 3 << 8 | 4); } @Test public void hostFromServerPropertiesIp() throws UnknownHostException { ServerProperties properties = new ServerProperties(); - properties.setAddress(InetAddress.getByAddress(new byte[] { 1, 2, 3, 4 })); + properties.setAddress(InetAddress.getByAddress(ADDRESS1234)); ServerPropertiesEndpointLocator locator = new ServerPropertiesEndpointLocator( - properties, "unknown", new ZipkinProperties()); + properties, "unknown", new ZipkinProperties(), + localAddress(new byte[] { 4, 4, 4, 4 })); assertThat(locator.local().ipv4).isEqualTo(1 << 24 | 2 << 16 | 3 << 8 | 4); } @@ -71,8 +78,15 @@ public class ServerPropertiesEndpointLocatorTests { zipkinProperties.getService().setName("foo"); ServerPropertiesEndpointLocator locator = new ServerPropertiesEndpointLocator( - properties, "unknown", zipkinProperties); + properties, "unknown", zipkinProperties,localAddress(ADDRESS1234)); assertThat(locator.local().serviceName).isEqualTo("foo"); } + + private InetUtils localAddress(byte[] address) throws UnknownHostException { + InetUtils mocked = Mockito.spy(new InetUtils(new InetUtilsProperties())); + Mockito.when(mocked.findFirstNonLoopbackAddress()) + .thenReturn(InetAddress.getByAddress(address)); + return mocked; + } }