Merge pull request #772 from DataDog/tyler/status-error-mapping

Implement Http Status error mapping via config

dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpClientDecorator.java

@@ -49,7 +49,8 @@ public abstract class HttpClientDecorator<REQUEST, RESPONSE> 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);
         }
       }

dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpServerDecorator.java

@@ -45,7 +45,8 @@ public abstract class HttpServerDecorator<REQUEST, RESPONSE> 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);
         }
       }

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:
+    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
+    status | error | errorRange | resp
-    200    | false | [status: 200]
+    200    | false | null       | [status: 200]
-    399    | false | [status: 399]
+    399    | false | null       | [status: 399]
-    400    | true  | [status: 400]
+    400    | true  | null       | [status: 400]
-    499    | true  | [status: 499]
+    499    | true  | null       | [status: 499]
-    500    | false | [status: 500]
+    500    | false | null       | [status: 500]
-    600    | false | [status: 600]
+    500    | true  | "500"      | [status: 500]
-    null   | false | [status: null]
+    500    | true  | "400-500"  | [status: 500]
-    null   | false | null
+    600    | false | null       | [status: 600]
+    null   | false | null       | [status: null]
+    null   | false | null       | null
   }
 
   def "test assert null span"() {

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:
+    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
+    status | error | errorRange | resp
-    200    | false | [status: 200]
+    200    | false | null       | [status: 200]
-    399    | false | [status: 399]
+    399    | false | null       | [status: 399]
-    400    | false | [status: 400]
+    400    | false | null       | [status: 400]
-    499    | false | [status: 499]
+    404    | true  | "404"      | [status: 404]
-    500    | true  | [status: 500]
+    404    | true  | "400-500"  | [status: 404]
-    600    | true  | [status: 600]
+    499    | false | null       | [status: 499]
-    null   | false | [status: null]
+    500    | true  | null       | [status: 500]
-    null   | false | null
+    600    | false | null       | [status: 600]
+    null   | false | null       | [status: null]
+    null   | false | null       | null
   }
 
   def "test assert null span"() {

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<Integer> DEFAULT_HTTP_SERVER_ERROR_STATUSES =
+      parseIntegerRangeSet("500-599", "default");
+  private static final Set<Integer> 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<String, String> spanTags;
   private final Map<String, String> jmxTags;
   @Getter private final Map<String, String> headerTags;
+  @Getter private final Set<Integer> httpServerErrorStatuses;
+  @Getter private final Set<Integer> 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<Integer> getIntegerRangeSettingFromEnvironment(
+      final String name, final Set<Integer> 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,7 +435,19 @@ public class Config {
     return value == null || value.trim().isEmpty() ? defaultValue : Integer.valueOf(value);
   }
 
+  private Set<Integer> getPropertyIntegerRangeValue(
+      final Properties properties, final String name, final Set<Integer> 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<String, String> 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();
     }
@@ -425,6 +475,40 @@ public class Config {
     return Collections.unmodifiableMap(map);
   }
 
+  private static Set<Integer> parseIntegerRangeSet(String str, final String settingName)
+      throws NumberFormatException {
+    if (str == null) {
+      str = "";
+    }
+    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<Integer> 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<String, String> newHashMap(final int size) {
     return new HashMap<>(size + 1, 1f);
   }

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,37 @@ 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) {
+      assert config.httpServerErrorStatuses == expected.toSet()
+      assert config.httpClientErrorStatuses == expected.toSet()
+    } else {
+      assert config.httpServerErrorStatuses == Config.DEFAULT_HTTP_SERVER_ERROR_STATUSES
+      assert config.httpClientErrorStatuses == Config.DEFAULT_HTTP_CLIENT_ERROR_STATUSES
+    }
+
+    where:
+    value               | expected // null means default value
+    "1"                 | null
+    "a"                 | null
+    ""                  | null
+    "1000"              | null
+    "100-200-300"       | 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)