From 7fc10e80cdab77aecfec1eb0557fa2b4aa8bcd43 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Wed, 12 Sep 2018 18:19:09 +0200 Subject: [PATCH] Picking skip patterns frmo various locations; fixes gh-971 --- .../instrument/web/SingleSkipPattern.java | 32 +++++ .../web/TraceWebAutoConfiguration.java | 126 ++++++++++-------- .../web/SkipPatternProviderConfigTest.java | 110 +++++++-------- .../DemoSleuthSkipApplicationTests.java | 72 ++++++++++ 4 files changed, 222 insertions(+), 118 deletions(-) create mode 100644 spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/SingleSkipPattern.java create mode 100644 spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/issues/issue971/DemoSleuthSkipApplicationTests.java diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/SingleSkipPattern.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/SingleSkipPattern.java new file mode 100644 index 000000000..62fc22883 --- /dev/null +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/SingleSkipPattern.java @@ -0,0 +1,32 @@ +/* + * Copyright 2013-2018 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cloud.sleuth.instrument.web; + +import java.util.Optional; +import java.util.regex.Pattern; + +/** + * Provides a URL {@link Pattern} for spans that should be not sampled. + * The default implementation of {@link SkipPatternProvider} will harvest all + * {@link SingleSkipPattern}s and combine them in a single pattern + * + * @author Marcin Grzejszczak + * @since 2.1.0 + */ +interface SingleSkipPattern { + Optional skipPattern(); +} diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebAutoConfiguration.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebAutoConfiguration.java index 230c1c350..27d696436 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebAutoConfiguration.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebAutoConfiguration.java @@ -15,16 +15,21 @@ */ package org.springframework.cloud.sleuth.instrument.web; +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; import java.util.regex.Pattern; +import java.util.stream.Collectors; import brave.Tracing; +import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.actuate.autoconfigure.web.server.ManagementServerProperties; import org.springframework.boot.autoconfigure.AutoConfigureAfter; import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; 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.sleuth.autoconfig.TraceAutoConfiguration; import org.springframework.context.annotation.Bean; @@ -46,82 +51,93 @@ import org.springframework.util.StringUtils; @EnableConfigurationProperties(SleuthWebProperties.class) public class TraceWebAutoConfiguration { + @Autowired(required = false) List patterns = new ArrayList<>(); + + @Bean + @ConditionalOnMissingBean + SkipPatternProvider sleuthSkipPatternProvider() { + return () -> Pattern.compile(this.patterns + .stream() + .map(SingleSkipPattern::skipPattern) + .filter(Optional::isPresent) + .map(Optional::get) + .map(Pattern::pattern) + .collect(Collectors.joining("|"))); + } + @Configuration @ConditionalOnClass(ManagementServerProperties.class) - @ConditionalOnMissingBean(SkipPatternProvider.class) - @EnableConfigurationProperties(SleuthWebProperties.class) - protected static class SkipPatternProviderConfig { + protected static class ManagementSkipPatternProviderConfig { @Bean @ConditionalOnBean(ManagementServerProperties.class) - public SkipPatternProvider skipPatternForManagementServerProperties( - final ManagementServerProperties managementServerProperties, - final SleuthWebProperties sleuthWebProperties) { - return new SkipPatternProvider() { - @Override - public Pattern skipPattern() { - return getPatternForManagementServerProperties( - managementServerProperties, - sleuthWebProperties); - } - }; + public SingleSkipPattern skipPatternForManagementServerProperties( + final ManagementServerProperties managementServerProperties) { + return () -> getPatternForManagementServerProperties(managementServerProperties); } /** * Sets or appends {@link ManagementServerProperties#getServlet()#getContextPath()} to the skip * pattern. If neither is available then sets the default one */ - static Pattern getPatternForManagementServerProperties( - ManagementServerProperties managementServerProperties, - SleuthWebProperties sleuthWebProperties) { - String skipPattern = sleuthWebProperties.getSkipPattern(); - String additionalSkipPattern = sleuthWebProperties.getAdditionalSkipPattern(); + static Optional getPatternForManagementServerProperties( + ManagementServerProperties managementServerProperties) { String contextPath = managementServerProperties.getServlet().getContextPath(); - - if (StringUtils.hasText(skipPattern) && StringUtils.hasText(contextPath)) { - return Pattern.compile(combinedPattern(skipPattern + "|" + contextPath + ".*", additionalSkipPattern)); + if (StringUtils.hasText(contextPath)) { + return Optional.of(Pattern.compile(contextPath + ".*")); } - else if (StringUtils.hasText(contextPath)) { - return Pattern.compile(combinedPattern(contextPath + ".*", additionalSkipPattern)); - } - return defaultSkipPattern(skipPattern, additionalSkipPattern); + return Optional.empty(); } + } + + @Configuration + @ConditionalOnClass(ServerProperties.class) + protected static class ServerSkipPatternProviderConfig { @Bean - @ConditionalOnMissingBean(ManagementServerProperties.class) - public SkipPatternProvider defaultSkipPatternBeanIfManagementServerPropsArePresent(SleuthWebProperties sleuthWebProperties) { - return defaultSkipPatternProvider(sleuthWebProperties.getSkipPattern(), - sleuthWebProperties.getAdditionalSkipPattern()); + @ConditionalOnBean(ServerProperties.class) + public SingleSkipPattern skipPatternForServerProperties( + final ServerProperties serverProperties) { + return () -> getPatternForServerProperties(serverProperties); + } + + /** + * Sets or appends {@link ServerProperties#getServlet()#getContextPath()} to the skip + * pattern. If neither is available then sets the default one + */ + static Optional getPatternForServerProperties( + ServerProperties serverProperties) { + String contextPath = serverProperties.getServlet().getContextPath(); + if (StringUtils.hasText(contextPath)) { + return Optional.of(Pattern.compile(contextPath + ".*")); + } + return Optional.empty(); } } - @Bean - @ConditionalOnMissingClass("org.springframework.boot.actuate.autoconfigure.ManagementServerProperties") - @ConditionalOnMissingBean( - SkipPatternProvider.class) - public SkipPatternProvider defaultSkipPatternBean(SleuthWebProperties sleuthWebProperties) { - return defaultSkipPatternProvider(sleuthWebProperties.getSkipPattern(), - sleuthWebProperties.getAdditionalSkipPattern()); - } + @Configuration + static class DefaultSkipPatternConfig { - private static SkipPatternProvider defaultSkipPatternProvider( - final String skipPattern, final String additionalSkipPattern) { - return () -> defaultSkipPattern(skipPattern, additionalSkipPattern); - } - - private static Pattern defaultSkipPattern(String skipPattern, String additionalSkipPattern) { - return Pattern.compile(combinedPattern(skipPattern, additionalSkipPattern)); - } - - private static String combinedPattern(String skipPattern, String additionalSkipPattern) { - String pattern = skipPattern; - if (!StringUtils.hasText(skipPattern)) { - pattern = SleuthWebProperties.DEFAULT_SKIP_PATTERN; + @Bean + SingleSkipPattern defaultSkipPatternBean(SleuthWebProperties sleuthWebProperties) { + return () -> Optional.of( + Pattern.compile( + combinedPattern(sleuthWebProperties.getSkipPattern(), + sleuthWebProperties.getAdditionalSkipPattern()) + ) + ); } - if (StringUtils.hasText(additionalSkipPattern)) { - return pattern + "|" + additionalSkipPattern; + + private static String combinedPattern(String skipPattern, String additionalSkipPattern) { + String pattern = skipPattern; + if (!StringUtils.hasText(skipPattern)) { + pattern = SleuthWebProperties.DEFAULT_SKIP_PATTERN; + } + if (StringUtils.hasText(additionalSkipPattern)) { + return pattern + "|" + additionalSkipPattern; + } + return pattern; } - return pattern; } } diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/SkipPatternProviderConfigTest.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/SkipPatternProviderConfigTest.java index 7df40583b..c1b3a81c2 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/SkipPatternProviderConfigTest.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/SkipPatternProviderConfigTest.java @@ -16,10 +16,13 @@ package org.springframework.cloud.sleuth.instrument.web; +import java.util.Arrays; +import java.util.Optional; import java.util.regex.Pattern; import org.junit.Test; import org.springframework.boot.actuate.autoconfigure.web.server.ManagementServerProperties; +import org.springframework.boot.autoconfigure.web.ServerProperties; import static org.assertj.core.api.BDDAssertions.then; @@ -29,100 +32,81 @@ import static org.assertj.core.api.BDDAssertions.then; public class SkipPatternProviderConfigTest { @Test - public void should_combine_skip_pattern_and_management_context_when_they_are_both_not_empty() throws Exception { + public void should_pick_skip_pattern_from_sleuth_properties() throws Exception { SleuthWebProperties sleuthWebProperties = new SleuthWebProperties(); sleuthWebProperties.setSkipPattern("foo.*|bar.*"); - Pattern pattern = TraceWebAutoConfiguration.SkipPatternProviderConfig.getPatternForManagementServerProperties( - managementServerPropertiesWithContextPath(), sleuthWebProperties); - - then(pattern.pattern()).isEqualTo("foo.*|bar.*|/management/context.*"); - } - - @Test - public void should_combine_skip_pattern_management_context_and_additional_pattern_when_all_are_not_empty() throws Exception { - SleuthWebProperties sleuthWebProperties = new SleuthWebProperties(); - sleuthWebProperties.setSkipPattern("foo.*|bar.*"); - sleuthWebProperties.setAdditionalSkipPattern("baz.*|faz.*"); - Pattern pattern = TraceWebAutoConfiguration.SkipPatternProviderConfig.getPatternForManagementServerProperties( - managementServerPropertiesWithContextPath(), sleuthWebProperties); - - then(pattern.pattern()).isEqualTo("foo.*|bar.*|/management/context.*|baz.*|faz.*"); - } - - @Test - public void should_pick_skip_pattern_when_its_not_empty_and_management_context_is_empty() throws Exception { - SleuthWebProperties sleuthWebProperties = new SleuthWebProperties(); - sleuthWebProperties.setSkipPattern("foo.*|bar.*"); - - Pattern pattern = TraceWebAutoConfiguration.SkipPatternProviderConfig.getPatternForManagementServerProperties(new ManagementServerProperties(), sleuthWebProperties); + Pattern pattern = new TraceWebAutoConfiguration.DefaultSkipPatternConfig().defaultSkipPatternBean(sleuthWebProperties) + .skipPattern().get(); then(pattern.pattern()).isEqualTo("foo.*|bar.*"); } @Test - public void should_pick_skip_pattern_and_additional_pattern_when_its_not_empty_and_management_context_is_empty() throws Exception { + public void should_combine_skip_pattern_and_additional_pattern_when_all_are_not_empty() throws Exception { SleuthWebProperties sleuthWebProperties = new SleuthWebProperties(); sleuthWebProperties.setSkipPattern("foo.*|bar.*"); sleuthWebProperties.setAdditionalSkipPattern("baz.*|faz.*"); - - Pattern pattern = TraceWebAutoConfiguration.SkipPatternProviderConfig.getPatternForManagementServerProperties(new ManagementServerProperties(), sleuthWebProperties); + Pattern pattern = new TraceWebAutoConfiguration.DefaultSkipPatternConfig().defaultSkipPatternBean(sleuthWebProperties) + .skipPattern().get(); then(pattern.pattern()).isEqualTo("foo.*|bar.*|baz.*|faz.*"); } @Test - public void should_pick_management_context_when_skip_patterns_is_empty_and_context_path_is_not() throws Exception { - SleuthWebProperties sleuthWebProperties = new SleuthWebProperties(); - sleuthWebProperties.setSkipPattern(""); + public void should_return_empty_when_management_context_has_no_context_path() throws Exception { + Optional pattern = new TraceWebAutoConfiguration.ManagementSkipPatternProviderConfig() + .skipPatternForManagementServerProperties(new ManagementServerProperties()).skipPattern(); - Pattern pattern = TraceWebAutoConfiguration.SkipPatternProviderConfig.getPatternForManagementServerProperties( - managementServerPropertiesWithContextPath(), sleuthWebProperties); - - then(pattern.pattern()).isEqualTo("/management/context.*"); + then(pattern).isEmpty(); } @Test - public void should_pick_management_context_and_additional_pattern_when_skip_patterns_is_empty_and_context_path_is_not() throws Exception { - SleuthWebProperties sleuthWebProperties = new SleuthWebProperties(); - sleuthWebProperties.setSkipPattern(""); - sleuthWebProperties.setAdditionalSkipPattern("baz.*|faz.*"); + public void should_return_management_context_with_context_path() throws Exception { + ManagementServerProperties properties = new ManagementServerProperties(); + properties.getServlet().setContextPath("foo"); - Pattern pattern = TraceWebAutoConfiguration.SkipPatternProviderConfig.getPatternForManagementServerProperties( - managementServerPropertiesWithContextPath(), sleuthWebProperties); + Optional pattern = new TraceWebAutoConfiguration.ManagementSkipPatternProviderConfig() + .skipPatternForManagementServerProperties(properties).skipPattern(); - then(pattern.pattern()).isEqualTo("/management/context.*|baz.*|faz.*"); + then(pattern).isNotEmpty(); + then(pattern.get().pattern()).isEqualTo("foo.*"); } @Test - public void should_pick_default_pattern_when_both_management_context_and_skip_patterns_are_empty() throws Exception { - SleuthWebProperties sleuthWebProperties = new SleuthWebProperties(); - sleuthWebProperties.setSkipPattern(""); - ManagementServerProperties managementServerProperties = new ManagementServerProperties(); - managementServerProperties.getServlet().setContextPath(""); + public void should_return_empty_when_server_props_have_no_context_path() throws Exception { + Optional pattern = new TraceWebAutoConfiguration.ServerSkipPatternProviderConfig() + .skipPatternForServerProperties(new ServerProperties()).skipPattern(); - Pattern pattern = TraceWebAutoConfiguration.SkipPatternProviderConfig.getPatternForManagementServerProperties( - managementServerProperties, sleuthWebProperties); - - then(pattern.pattern()).isEqualTo(SleuthWebProperties.DEFAULT_SKIP_PATTERN); + then(pattern).isEmpty(); } @Test - public void should_pick_default_pattern_with_additional_pattern_when_both_management_context_and_skip_patterns_are_empty() throws Exception { - SleuthWebProperties sleuthWebProperties = new SleuthWebProperties(); - sleuthWebProperties.setSkipPattern(""); - sleuthWebProperties.setAdditionalSkipPattern("baz.*|faz.*"); - ManagementServerProperties managementServerProperties = new ManagementServerProperties(); - managementServerProperties.getServlet().setContextPath(""); + public void should_return_server_props_with_context_path() throws Exception { + ServerProperties properties = new ServerProperties(); + properties.getServlet().setContextPath("foo"); - Pattern pattern = TraceWebAutoConfiguration.SkipPatternProviderConfig.getPatternForManagementServerProperties( - managementServerProperties, sleuthWebProperties); + Optional pattern = new TraceWebAutoConfiguration.ServerSkipPatternProviderConfig() + .skipPatternForServerProperties(properties).skipPattern(); - then(pattern.pattern()).isEqualTo(SleuthWebProperties.DEFAULT_SKIP_PATTERN + "|baz.*|faz.*"); + then(pattern).isNotEmpty(); + then(pattern.get().pattern()).isEqualTo("foo.*"); } - private ManagementServerProperties managementServerPropertiesWithContextPath() { - ManagementServerProperties managementServerProperties = new ManagementServerProperties(); - managementServerProperties.getServlet().setContextPath("/management/context"); - return managementServerProperties; + @Test + public void should_combine_skip_patterns_from_list() throws Exception { + TraceWebAutoConfiguration configuration = new TraceWebAutoConfiguration(); + configuration.patterns.addAll(Arrays.asList(foo(), bar())); + + Pattern pattern = configuration.sleuthSkipPatternProvider().skipPattern(); + + then(pattern.pattern()).isEqualTo("foo|bar"); + } + + private SingleSkipPattern foo() { + return () -> Optional.of(Pattern.compile("foo")); + } + + private SingleSkipPattern bar() { + return () -> Optional.of(Pattern.compile("bar")); } } \ No newline at end of file diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/issues/issue971/DemoSleuthSkipApplicationTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/issues/issue971/DemoSleuthSkipApplicationTests.java new file mode 100644 index 000000000..d1800d7d6 --- /dev/null +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/issues/issue971/DemoSleuthSkipApplicationTests.java @@ -0,0 +1,72 @@ +/* + * Copyright 2013-2018 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cloud.sleuth.instrument.web.issues.issue971; + +import brave.Tracer; +import brave.sampler.Sampler; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.autoconfigure.EnableAutoConfiguration; +import org.springframework.boot.autoconfigure.amqp.RabbitAutoConfiguration; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.web.server.LocalServerPort; +import org.springframework.cloud.sleuth.DisableSecurity; +import org.springframework.cloud.sleuth.util.ArrayListSpanReporter; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.test.context.junit4.SpringRunner; +import org.springframework.web.client.RestTemplate; + +import static org.assertj.core.api.BDDAssertions.then; + +@RunWith(SpringRunner.class) +@SpringBootTest(classes = DemoSleuthSkipApplicationTests.Config.class, + webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT, + properties = { "management.endpoints.web.exposure.include:*", + "server.servlet.context-path:/context-path", + "spring.sleuth.http.legacy.enabled:true"}) +public class DemoSleuthSkipApplicationTests { + + @Autowired ArrayListSpanReporter accumulator; + @Autowired Tracer tracer; + + @LocalServerPort int port; + + @Test + public void should_not_sample_skipped_endpoint_with_context_path() { + new RestTemplate().getForObject("http://localhost:" + + this.port + "/context-path/actuator/health", String.class); + + then(this.tracer.currentSpan()).isNull(); + then(this.accumulator.getSpans()).hasSize(0); + } + + @EnableAutoConfiguration(exclude = RabbitAutoConfiguration.class) + @Configuration + @DisableSecurity + public static class Config { + + @Bean ArrayListSpanReporter reporter() { + return new ArrayListSpanReporter(); + } + + @Bean Sampler sampler() { + return Sampler.ALWAYS_SAMPLE; + } + } +} \ No newline at end of file