From 715af67e70a42d0829701202dbdfabc06685a202 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Tue, 19 Mar 2019 17:11:48 -0700 Subject: [PATCH 1/2] Implement Http Status error mapping via config Using the following defaults: * `DD_HTTP_CLIENT_ERROR_STATUSES=400-499` * `DD_HTTP_SERVER_ERROR_STATUSES=500-599` --- .../agent/decorator/HttpClientDecorator.java | 3 +- .../agent/decorator/HttpServerDecorator.java | 3 +- .../decorator/HttpClientDecoratorTest.groovy | 24 ++--- .../decorator/HttpServerDecoratorTest.groovy | 27 +++--- .../main/java/datadog/trace/api/Config.java | 88 ++++++++++++++++++- .../datadog/trace/api/ConfigTest.groovy | 46 ++++++++++ 6 files changed, 167 insertions(+), 24 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpClientDecorator.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpClientDecorator.java index f9bcc2af02..ab3e25d022 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpClientDecorator.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpClientDecorator.java @@ -49,7 +49,8 @@ public abstract class HttpClientDecorator extends ClientDecor final Integer status = status(response); if (status != null) { Tags.HTTP_STATUS.set(span, status); - if (400 <= status && status < 500) { + + if (Config.get().getHttpClientErrorStatuses().contains(status)) { Tags.ERROR.set(span, true); } } diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpServerDecorator.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpServerDecorator.java index d29a9f423c..0a0aea0886 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpServerDecorator.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpServerDecorator.java @@ -45,7 +45,8 @@ public abstract class HttpServerDecorator extends ServerDecor final Integer status = status(response); if (status != null) { Tags.HTTP_STATUS.set(span, status); - if (status >= 500) { + + if (Config.get().getHttpServerErrorStatuses().contains(status)) { Tags.ERROR.set(span, true); } } diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/HttpClientDecoratorTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/HttpClientDecoratorTest.groovy index 90e17ed08f..92fe79a0fa 100644 --- a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/HttpClientDecoratorTest.groovy +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/HttpClientDecoratorTest.groovy @@ -45,7 +45,9 @@ class HttpClientDecoratorTest extends ClientDecoratorTest { def decorator = newDecorator() when: - decorator.onResponse(span, resp) + withConfigOverride(Config.HTTP_CLIENT_ERROR_STATUSES, "$errorRange") { + decorator.onResponse(span, resp) + } then: if (status) { @@ -57,15 +59,17 @@ class HttpClientDecoratorTest extends ClientDecoratorTest { 0 * _ where: - status | error | resp - 200 | false | [status: 200] - 399 | false | [status: 399] - 400 | true | [status: 400] - 499 | true | [status: 499] - 500 | false | [status: 500] - 600 | false | [status: 600] - null | false | [status: null] - null | false | null + status | error | errorRange | resp + 200 | false | null | [status: 200] + 399 | false | null | [status: 399] + 400 | true | null | [status: 400] + 499 | true | null | [status: 499] + 500 | false | null | [status: 500] + 500 | true | "500" | [status: 500] + 500 | true | "400-500" | [status: 500] + 600 | false | null | [status: 600] + null | false | null | [status: null] + null | false | null | null } def "test assert null span"() { diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/HttpServerDecoratorTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/HttpServerDecoratorTest.groovy index 1c1e3c01ec..41f3865957 100644 --- a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/HttpServerDecoratorTest.groovy +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/HttpServerDecoratorTest.groovy @@ -1,8 +1,11 @@ package datadog.trace.agent.decorator +import datadog.trace.api.Config import io.opentracing.Span import io.opentracing.tag.Tags +import static datadog.trace.agent.test.utils.TraceUtils.withConfigOverride + class HttpServerDecoratorTest extends ServerDecoratorTest { def span = Mock(Span) @@ -32,7 +35,9 @@ class HttpServerDecoratorTest extends ServerDecoratorTest { def decorator = newDecorator() when: - decorator.onResponse(span, resp) + withConfigOverride(Config.HTTP_SERVER_ERROR_STATUSES, "$errorRange") { + decorator.onResponse(span, resp) + } then: if (status) { @@ -44,15 +49,17 @@ class HttpServerDecoratorTest extends ServerDecoratorTest { 0 * _ where: - status | error | resp - 200 | false | [status: 200] - 399 | false | [status: 399] - 400 | false | [status: 400] - 499 | false | [status: 499] - 500 | true | [status: 500] - 600 | true | [status: 600] - null | false | [status: null] - null | false | null + status | error | errorRange | resp + 200 | false | null | [status: 200] + 399 | false | null | [status: 399] + 400 | false | null | [status: 400] + 404 | true | "404" | [status: 404] + 404 | true | "400-500" | [status: 404] + 499 | false | null | [status: 499] + 500 | true | null | [status: 500] + 600 | false | null | [status: 600] + null | false | null | [status: null] + null | false | null | null } def "test assert null span"() { diff --git a/dd-trace-api/src/main/java/datadog/trace/api/Config.java b/dd-trace-api/src/main/java/datadog/trace/api/Config.java index fffabbd764..2d58c5b907 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/Config.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/Config.java @@ -3,9 +3,11 @@ package datadog.trace.api; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Properties; +import java.util.Set; import java.util.SortedSet; import java.util.UUID; import java.util.regex.Pattern; @@ -30,8 +32,6 @@ public class Config { private static final Pattern ENV_REPLACEMENT = Pattern.compile("[^a-zA-Z0-9_]"); - private static final Config INSTANCE = new Config(); - public static final String SERVICE_NAME = "service.name"; public static final String SERVICE = "service"; public static final String WRITER_TYPE = "writer.type"; @@ -49,6 +49,8 @@ public class Config { public static final String TRACE_ANNOTATIONS = "trace.annotations"; public static final String TRACE_METHODS = "trace.methods"; public static final String HEADER_TAGS = "trace.header.tags"; + public static final String HTTP_SERVER_ERROR_STATUSES = "trace.http.server.error.statuses"; + public static final String HTTP_CLIENT_ERROR_STATUSES = "trace.http.client.error.statuses"; public static final String HTTP_CLIENT_HOST_SPLIT_BY_DOMAIN = "trace.http.client.split-by-domain"; public static final String PARTIAL_FLUSH_MIN_SPANS = "trace.partial.flush.min.spans"; public static final String RUNTIME_CONTEXT_FIELD_INJECTION = @@ -81,6 +83,10 @@ public class Config { private static final boolean DEFAULT_PRIORITY_SAMPLING_ENABLED = true; private static final boolean DEFAULT_TRACE_RESOLVER_ENABLED = true; + private static final Set DEFAULT_HTTP_SERVER_ERROR_STATUSES = + parseIntegerRangeSet("500-599", "default"); + private static final Set DEFAULT_HTTP_CLIENT_ERROR_STATUSES = + parseIntegerRangeSet("400-499", "default"); private static final boolean DEFAULT_HTTP_CLIENT_SPLIT_BY_DOMAIN = false; private static final int DEFAULT_PARTIAL_FLUSH_MIN_SPANS = 0; private static final boolean DEFAULT_JMX_FETCH_ENABLED = false; @@ -89,6 +95,9 @@ public class Config { private static final boolean DEFAULT_APP_CUSTOM_LOG_MANAGER = false; + // Must be defined last to allow above defaults to be properly initialized. + private static final Config INSTANCE = new Config(); + /** * this is a random UUID that gets generated on JVM start up and is attached to every root span * and every JMX metric that is sent out. @@ -107,6 +116,8 @@ public class Config { private final Map spanTags; private final Map jmxTags; @Getter private final Map headerTags; + @Getter private final Set httpServerErrorStatuses; + @Getter private final Set httpClientErrorStatuses; @Getter private final boolean httpClientSplitByDomain; @Getter private final Integer partialFlushMinSpans; @Getter private final boolean runtimeContextFieldInjection; @@ -144,6 +155,14 @@ public class Config { jmxTags = getMapSettingFromEnvironment(JMX_TAGS, null); headerTags = getMapSettingFromEnvironment(HEADER_TAGS, null); + httpServerErrorStatuses = + getIntegerRangeSettingFromEnvironment( + HTTP_SERVER_ERROR_STATUSES, DEFAULT_HTTP_SERVER_ERROR_STATUSES); + + httpClientErrorStatuses = + getIntegerRangeSettingFromEnvironment( + HTTP_CLIENT_ERROR_STATUSES, DEFAULT_HTTP_CLIENT_ERROR_STATUSES); + httpClientSplitByDomain = getBooleanSettingFromEnvironment( HTTP_CLIENT_HOST_SPLIT_BY_DOMAIN, DEFAULT_HTTP_CLIENT_SPLIT_BY_DOMAIN); @@ -197,6 +216,14 @@ public class Config { jmxTags = getPropertyMapValue(properties, JMX_TAGS, parent.jmxTags); headerTags = getPropertyMapValue(properties, HEADER_TAGS, parent.headerTags); + httpServerErrorStatuses = + getPropertyIntegerRangeValue( + properties, HTTP_SERVER_ERROR_STATUSES, parent.httpServerErrorStatuses); + + httpClientErrorStatuses = + getPropertyIntegerRangeValue( + properties, HTTP_CLIENT_ERROR_STATUSES, parent.httpClientErrorStatuses); + httpClientSplitByDomain = getPropertyBooleanValue( properties, HTTP_CLIENT_HOST_SPLIT_BY_DOMAIN, parent.httpClientSplitByDomain); @@ -369,6 +396,17 @@ public class Config { } } + private Set getIntegerRangeSettingFromEnvironment( + final String name, final Set defaultValue) { + final String value = getSettingFromEnvironment(name, null); + try { + return value == null ? defaultValue : parseIntegerRangeSet(value, name); + } catch (final NumberFormatException e) { + log.warn("Invalid configuration for " + name, e); + return defaultValue; + } + } + private static String propertyToEnvironmentName(final String name) { return ENV_REPLACEMENT.matcher(name.toUpperCase()).replaceAll("_"); } @@ -397,6 +435,17 @@ public class Config { return value == null || value.trim().isEmpty() ? defaultValue : Integer.valueOf(value); } + private Set getPropertyIntegerRangeValue( + final Properties properties, final String name, final Set defaultValue) { + final String value = properties.getProperty(name); + try { + return value == null ? defaultValue : parseIntegerRangeSet(value, name); + } catch (final NumberFormatException e) { + log.warn("Invalid configuration for " + name, e); + return defaultValue; + } + } + private static Map parseMap(final String str, final String settingName) { if (str == null || str.trim().isEmpty()) { return Collections.emptyMap(); @@ -425,6 +474,41 @@ public class Config { return Collections.unmodifiableMap(map); } + private static Set parseIntegerRangeSet(String str, final String settingName) + throws NumberFormatException { + + if (str == null || str.trim().isEmpty()) { + return Collections.emptySet(); + } + str = str.replaceAll("\\s", ""); + if (!str.matches("\\d{3}(?:-\\d{3})*(?:,\\d{3}(?:-\\d{3})*)*")) { + log.warn( + "Invalid config for {}: '{}'. Must be formatted like '400-403,405,410-499'.", + settingName, + str); + throw new NumberFormatException(); + } + + final String[] tokens = str.split(",", -1); + final Set set = new HashSet<>(); + + for (final String token : tokens) { + final String[] range = token.split("-", -1); + if (range.length == 1) { + set.add(Integer.parseInt(range[0])); + } else if (range.length == 2) { + final int left = Integer.parseInt(range[0]); + final int right = Integer.parseInt(range[1]); + final int min = Math.min(left, right); + final int max = Math.max(left, right); + for (int i = min; i <= max; i++) { + set.add(i); + } + } + } + return Collections.unmodifiableSet(set); + } + private static Map newHashMap(final int size) { return new HashMap<>(size + 1, 1f); } diff --git a/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy b/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy index 0fa6b5ab59..b4cd695de2 100644 --- a/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy +++ b/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy @@ -11,7 +11,9 @@ import static datadog.trace.api.Config.AGENT_UNIX_DOMAIN_SOCKET import static datadog.trace.api.Config.DEFAULT_JMX_FETCH_STATSD_PORT import static datadog.trace.api.Config.GLOBAL_TAGS import static datadog.trace.api.Config.HEADER_TAGS +import static datadog.trace.api.Config.HTTP_CLIENT_ERROR_STATUSES import static datadog.trace.api.Config.HTTP_CLIENT_HOST_SPLIT_BY_DOMAIN +import static datadog.trace.api.Config.HTTP_SERVER_ERROR_STATUSES import static datadog.trace.api.Config.JMX_FETCH_CHECK_PERIOD import static datadog.trace.api.Config.JMX_FETCH_ENABLED import static datadog.trace.api.Config.JMX_FETCH_METRICS_CONFIGS @@ -65,6 +67,8 @@ class ConfigTest extends Specification { config.mergedSpanTags == [:] config.mergedJmxTags == [(RUNTIME_ID_TAG): config.getRuntimeId(), (SERVICE): config.serviceName, (LANGUAGE_TAG_KEY): LANGUAGE_TAG_VALUE] config.headerTags == [:] + config.httpServerErrorStatuses == (500..599).toSet() + config.httpClientErrorStatuses == (400..499).toSet() config.httpClientSplitByDomain == false config.partialFlushMinSpans == 0 config.runtimeContextFieldInjection == true @@ -92,6 +96,8 @@ class ConfigTest extends Specification { System.setProperty(PREFIX + SPAN_TAGS, "c:3") System.setProperty(PREFIX + JMX_TAGS, "d:4") System.setProperty(PREFIX + HEADER_TAGS, "e:5") + System.setProperty(PREFIX + HTTP_SERVER_ERROR_STATUSES, "123-456,457,124-125,122") + System.setProperty(PREFIX + HTTP_CLIENT_ERROR_STATUSES, "111") System.setProperty(PREFIX + HTTP_CLIENT_HOST_SPLIT_BY_DOMAIN, "true") System.setProperty(PREFIX + PARTIAL_FLUSH_MIN_SPANS, "15") System.setProperty(PREFIX + RUNTIME_CONTEXT_FIELD_INJECTION, "false") @@ -117,6 +123,8 @@ class ConfigTest extends Specification { config.mergedSpanTags == [b: "2", c: "3"] config.mergedJmxTags == [b: "2", d: "4", (RUNTIME_ID_TAG): config.getRuntimeId(), (SERVICE): config.serviceName, (LANGUAGE_TAG_KEY): LANGUAGE_TAG_VALUE] config.headerTags == [e: "5"] + config.httpServerErrorStatuses == (122..457).toSet() + config.httpClientErrorStatuses == (111..111).toSet() config.httpClientSplitByDomain == true config.partialFlushMinSpans == 15 config.runtimeContextFieldInjection == false @@ -176,6 +184,8 @@ class ConfigTest extends Specification { System.setProperty(PREFIX + SERVICE_MAPPING, " ") System.setProperty(PREFIX + HEADER_TAGS, "1") System.setProperty(PREFIX + SPAN_TAGS, "invalid") + System.setProperty(PREFIX + HTTP_SERVER_ERROR_STATUSES, "1111") + System.setProperty(PREFIX + HTTP_CLIENT_ERROR_STATUSES, "1:1") System.setProperty(PREFIX + HTTP_CLIENT_HOST_SPLIT_BY_DOMAIN, "invalid") when: @@ -191,6 +201,8 @@ class ConfigTest extends Specification { config.serviceMapping == [:] config.mergedSpanTags == [:] config.headerTags == [:] + config.httpServerErrorStatuses == (500..599).toSet() + config.httpClientErrorStatuses == (400..499).toSet() config.httpClientSplitByDomain == false } @@ -251,6 +263,8 @@ class ConfigTest extends Specification { properties.setProperty(SPAN_TAGS, "c:3") properties.setProperty(JMX_TAGS, "d:4") properties.setProperty(HEADER_TAGS, "e:5") + properties.setProperty(HTTP_SERVER_ERROR_STATUSES, "123-456,457,124-125,122") + properties.setProperty(HTTP_CLIENT_ERROR_STATUSES, "111") properties.setProperty(HTTP_CLIENT_HOST_SPLIT_BY_DOMAIN, "true") properties.setProperty(PARTIAL_FLUSH_MIN_SPANS, "15") properties.setProperty(JMX_FETCH_METRICS_CONFIGS, "/foo.yaml,/bar.yaml") @@ -274,6 +288,8 @@ class ConfigTest extends Specification { config.mergedSpanTags == [b: "2", c: "3"] config.mergedJmxTags == [b: "2", d: "4", (RUNTIME_ID_TAG): config.getRuntimeId(), (SERVICE): config.serviceName, (LANGUAGE_TAG_KEY): LANGUAGE_TAG_VALUE] config.headerTags == [e: "5"] + config.httpServerErrorStatuses == (122..457).toSet() + config.httpClientErrorStatuses == (111..111).toSet() config.httpClientSplitByDomain == true config.partialFlushMinSpans == 15 config.jmxFetchMetricsConfigs == ["/foo.yaml", "/bar.yaml"] @@ -448,6 +464,36 @@ class ConfigTest extends Specification { "!a" | [:] } + def "verify integer range configs on tracer"() { + setup: + System.setProperty(PREFIX + HTTP_SERVER_ERROR_STATUSES, value) + System.setProperty(PREFIX + HTTP_CLIENT_ERROR_STATUSES, value) + + when: + def config = new Config() + + then: + if (expected) { + config.httpServerErrorStatuses == expected.toSet() + config.httpClientErrorStatuses == expected.toSet() + } else { + config.httpServerErrorStatuses == Config.DEFAULT_HTTP_SERVER_ERROR_STATUSES + config.httpClientErrorStatuses == Config.DEFAULT_HTTP_CLIENT_ERROR_STATUSES + } + + where: + value | expected // null means default value + "1" | null + "a" | null + "" | null + "1000" | null + "500" | [500] + "100,999" | [100, 999] + "999-888" | 888..999 + "400-403,405-407" | [400, 401, 402, 403, 405, 406, 407] + " 400 - 403 , 405 " | [400, 401, 402, 403, 405] + } + def "verify null value mapping configs on tracer"() { setup: environmentVariables.set(DD_SERVICE_MAPPING_ENV, mapString) From 4187bf3eb0142cb33d59e71c125df5be773111ed Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Wed, 20 Mar 2019 17:09:39 -0700 Subject: [PATCH 2/2] code review changes. --- dd-trace-api/src/main/java/datadog/trace/api/Config.java | 8 ++++---- .../src/test/groovy/datadog/trace/api/ConfigTest.groovy | 9 +++++---- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/dd-trace-api/src/main/java/datadog/trace/api/Config.java b/dd-trace-api/src/main/java/datadog/trace/api/Config.java index 2d58c5b907..144472107b 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/Config.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/Config.java @@ -447,6 +447,7 @@ public class Config { } private static Map parseMap(final String str, final String settingName) { + // If we ever want to have default values besides an empty map, this will need to change. if (str == null || str.trim().isEmpty()) { return Collections.emptyMap(); } @@ -476,12 +477,11 @@ public class Config { private static Set parseIntegerRangeSet(String str, final String settingName) throws NumberFormatException { - - if (str == null || str.trim().isEmpty()) { - return Collections.emptySet(); + if (str == null) { + str = ""; } str = str.replaceAll("\\s", ""); - if (!str.matches("\\d{3}(?:-\\d{3})*(?:,\\d{3}(?:-\\d{3})*)*")) { + if (!str.matches("\\d{3}(?:-\\d{3})?(?:,\\d{3}(?:-\\d{3})?)*")) { log.warn( "Invalid config for {}: '{}'. Must be formatted like '400-403,405,410-499'.", settingName, diff --git a/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy b/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy index b4cd695de2..726401bc14 100644 --- a/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy +++ b/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy @@ -474,11 +474,11 @@ class ConfigTest extends Specification { then: if (expected) { - config.httpServerErrorStatuses == expected.toSet() - config.httpClientErrorStatuses == expected.toSet() + assert config.httpServerErrorStatuses == expected.toSet() + assert config.httpClientErrorStatuses == expected.toSet() } else { - config.httpServerErrorStatuses == Config.DEFAULT_HTTP_SERVER_ERROR_STATUSES - config.httpClientErrorStatuses == Config.DEFAULT_HTTP_CLIENT_ERROR_STATUSES + assert config.httpServerErrorStatuses == Config.DEFAULT_HTTP_SERVER_ERROR_STATUSES + assert config.httpClientErrorStatuses == Config.DEFAULT_HTTP_CLIENT_ERROR_STATUSES } where: @@ -487,6 +487,7 @@ class ConfigTest extends Specification { "a" | null "" | null "1000" | null + "100-200-300" | null "500" | [500] "100,999" | [100, 999] "999-888" | 888..999