Mark 5xx status codes as an error

dd-java-agent/instrumentation/servlet-2/src/test/groovy/JettyServletTest.groovy

@@ -99,7 +99,7 @@ class JettyServletTest extends AgentTestRunner {
 
     span.context().serviceName == "unnamed-java-app"
     span.context().operationName == "servlet.request"
-    span.context().resourceName == "servlet.request"
+    span.context().resourceName == "GET /$path"
     span.context().spanType == DDSpanTypes.WEB_SERVLET
     !span.context().getErrorFlag()
     span.context().parentId != 0 // parent should be the okhttp call.
@@ -134,7 +134,7 @@ class JettyServletTest extends AgentTestRunner {
     def span = trace[0]
 
     span.context().operationName == "servlet.request"
-    span.context().resourceName == "servlet.request"
+    span.context().resourceName == "GET /$path"
     span.context().spanType == DDSpanTypes.WEB_SERVLET
     span.context().getErrorFlag()
     span.context().parentId != 0 // parent should be the okhttp call.

dd-java-agent/instrumentation/servlet-3/src/test/groovy/JettyServletTest.groovy

@@ -100,7 +100,7 @@ class JettyServletTest extends AgentTestRunner {
 
     span.context().serviceName == "unnamed-java-app"
     span.context().operationName == "servlet.request"
-    span.context().resourceName == "servlet.request"
+    span.context().resourceName == "GET /$path"
     span.context().spanType == DDSpanTypes.WEB_SERVLET
     !span.context().getErrorFlag()
     span.context().parentId != 0 // parent should be the okhttp call.
@@ -137,7 +137,7 @@ class JettyServletTest extends AgentTestRunner {
 
     span.context().serviceName == "unnamed-java-app"
     span.context().operationName == "servlet.request"
-    span.context().resourceName == "servlet.request"
+    span.context().resourceName == "GET /$path"
     span.context().spanType == DDSpanTypes.WEB_SERVLET
     span.context().getErrorFlag()
     span.context().parentId != 0 // parent should be the okhttp call.
@@ -160,6 +160,46 @@ class JettyServletTest extends AgentTestRunner {
     "sync" | "Hello Sync"
   }
 
+  @Unroll
+  def "test #path non-throwing-error servlet call"() {
+    setup:
+    def request = new Request.Builder()
+      .url("http://localhost:$PORT/$path?non-throwing-error=true")
+      .get()
+      .build()
+    def response = client.newCall(request).execute()
+
+    expect:
+    response.body().string().trim() != expectedResponse
+    writer.size() == 2 // second (parent) trace is the okhttp call above...
+    def trace = writer.firstTrace()
+    trace.size() == 1
+    def span = trace[0]
+
+    span.context().serviceName == "unnamed-java-app"
+    span.context().operationName == "servlet.request"
+    span.context().resourceName == "GET /$path"
+    span.context().spanType == DDSpanTypes.WEB_SERVLET
+    span.context().getErrorFlag()
+    span.context().parentId != 0 // parent should be the okhttp call.
+    span.context().tags["http.url"] == "http://localhost:$PORT/$path"
+    span.context().tags["http.method"] == "GET"
+    span.context().tags["span.kind"] == "server"
+    span.context().tags["component"] == "java-web-servlet"
+    span.context().tags["http.status_code"] == 500
+    span.context().tags["thread.name"] != null
+    span.context().tags["thread.id"] != null
+    span.context().tags["error"] == true
+    span.context().tags["error.msg"] == null
+    span.context().tags["error.type"] == null
+    span.context().tags["error.stack"] == null
+    span.context().tags.size() == 9
+
+    where:
+    path   | expectedResponse
+    "sync" | "Hello Sync"
+  }
+
   private static int randomOpenPort() {
     new ServerSocket(0).withCloseable {
       it.setReuseAddress(true)

dd-java-agent/instrumentation/servlet-3/src/test/groovy/TestServlet.groovy

@@ -13,6 +13,10 @@ class TestServlet {
       if (req.getParameter("error") != null) {
         throw new RuntimeException("some sync error")
       }
+      if (req.getParameter("non-throwing-error") != null) {
+        resp.sendError(500, "some sync error")
+        return
+      }
       resp.writer.print("Hello Sync")
     }
   }

dd-java-agent/instrumentation/servlet-3/src/test/groovy/TomcatServletTest.groovy

@@ -99,7 +99,7 @@ class TomcatServletTest extends AgentTestRunner {
 
     span.context().serviceName == "unnamed-java-app"
     span.context().operationName == "servlet.request"
-    span.context().resourceName == "servlet.request"
+    span.context().resourceName == "GET /$path"
     span.context().spanType == DDSpanTypes.WEB_SERVLET
     !span.context().getErrorFlag()
     span.context().parentId != 0 // parent should be the okhttp call.
@@ -136,7 +136,7 @@ class TomcatServletTest extends AgentTestRunner {
 
     span.context().serviceName == "unnamed-java-app"
     span.context().operationName == "servlet.request"
-    span.context().resourceName == "servlet.request"
+    span.context().resourceName == "GET /$path"
     span.context().spanType == DDSpanTypes.WEB_SERVLET
     span.context().getErrorFlag()
     span.context().parentId != 0 // parent should be the okhttp call.
@@ -159,6 +159,46 @@ class TomcatServletTest extends AgentTestRunner {
     "sync" | "Hello Sync"
   }
 
+  @Unroll
+  def "test #path error servlet call for non-throwing error"() {
+    setup:
+    def request = new Request.Builder()
+      .url("http://localhost:$PORT/$path?non-throwing-error=true")
+      .get()
+      .build()
+    def response = client.newCall(request).execute()
+
+    expect:
+    response.body().string().trim() != expectedResponse
+    writer.size() == 2 // second (parent) trace is the okhttp call above...
+    def trace = writer.firstTrace()
+    trace.size() == 1
+    def span = trace[0]
+
+    span.context().serviceName == "unnamed-java-app"
+    span.context().operationName == "servlet.request"
+    span.context().resourceName == "GET /$path"
+    span.context().spanType == DDSpanTypes.WEB_SERVLET
+    span.context().getErrorFlag()
+    span.context().parentId != 0 // parent should be the okhttp call.
+    span.context().tags["http.url"] == "http://localhost:$PORT/$path"
+    span.context().tags["http.method"] == "GET"
+    span.context().tags["span.kind"] == "server"
+    span.context().tags["component"] == "java-web-servlet"
+    span.context().tags["http.status_code"] == 500
+    span.context().tags["thread.name"] != null
+    span.context().tags["thread.id"] != null
+    span.context().tags["error"] == true
+    span.context().tags["error.msg"] == null
+    span.context().tags["error.type"] == null
+    span.context().tags["error.stack"] == null
+    span.context().tags.size() == 9
+
+    where:
+    path   | expectedResponse
+    "sync" | "Hello Sync"
+  }
+
   private static int randomOpenPort() {
     new ServerSocket(0).withCloseable {
       it.setReuseAddress(true)

dd-java-agent/instrumentation/spring-web/src/test/groovy/test/SpringBootBasedTest.groovy

@@ -148,17 +148,17 @@ class SpringBootBasedTest extends AgentTestRunner {
     span1.context().operationName == "servlet.request"
     span1.context().resourceName == "GET /error"
     span1.context().spanType == DDSpanTypes.WEB_SERVLET
-    !span1.context().getErrorFlag()
     span1.context().parentId == 0
     span1.context().tags["http.url"] == "http://localhost:$port/error"
     span1.context().tags["http.method"] == "GET"
     span1.context().tags["span.kind"] == "server"
     span1.context().tags["span.type"] == "web"
     span1.context().tags["component"] == "java-web-servlet"
     span1.context().tags["http.status_code"] == 500
+    span1.context().getErrorFlag()
     span1.context().tags["thread.name"] != null
     span1.context().tags["thread.id"] != null
-    span1.context().tags.size() == 8
+    span1.context().tags.size() == 9
   }
 
   def "validated form"() {

dd-java-agent/testing/src/main/java/datadog/trace/agent/test/AgentTestRunner.java

@@ -4,8 +4,6 @@
 import ch.qos.logback.classic.Logger;
 import datadog.opentracing.DDSpan;
 import datadog.opentracing.DDTracer;
-import datadog.opentracing.decorators.AbstractDecorator;
-import datadog.opentracing.decorators.DDDecoratorsFactory;
 import datadog.trace.agent.tooling.AgentInstaller;
 import datadog.trace.agent.tooling.Instrumenter;
 import datadog.trace.common.writer.ListWriter;
@@ -78,10 +76,6 @@ public boolean add(final List<DDSpan> trace) {
         };
     TEST_TRACER = new DDTracer(TEST_WRITER);
 
-    final List<AbstractDecorator> decorators = DDDecoratorsFactory.createBuiltinDecorators();
-    for (final AbstractDecorator decorator : decorators) {
-      ((DDTracer) TEST_TRACER).addDecorator(decorator);
-    }
     ByteBuddyAgent.install();
     instrumentation = ByteBuddyAgent.getInstrumentation();
   }

dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java

@@ -92,13 +92,6 @@ public DDTracer(final Properties config) {
         Sampler.Builder.forConfig(config),
         DDTraceConfig.parseMap(config.getProperty(DDTraceConfig.SPAN_TAGS)));
     log.debug("Using config: {}", config);
-
-    // Create decorators from resource files
-    final List<AbstractDecorator> decorators = DDDecoratorsFactory.createBuiltinDecorators();
-    for (final AbstractDecorator decorator : decorators) {
-      log.debug("Loading decorator: {}", decorator.getClass().getSimpleName());
-      addDecorator(decorator);
-    }
   }
 
   public DDTracer(final String serviceName, final Writer writer, final Sampler sampler) {
@@ -126,6 +119,12 @@ public DDTracer(
 
     registerClassLoader(ClassLoader.getSystemClassLoader());
 
+    final List<AbstractDecorator> decorators = DDDecoratorsFactory.createBuiltinDecorators();
+    for (final AbstractDecorator decorator : decorators) {
+      log.debug("Loading decorator: {}", decorator.getClass().getSimpleName());
+      addDecorator(decorator);
+    }
+
     log.info("New instance: {}", this);
   }
 

dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java

@@ -25,6 +25,7 @@ public static List<AbstractDecorator> createBuiltinDecorators() {
     builtin.add(new OperationDecorator());
     builtin.add(new Status404Decorator());
     builtin.add(new URLAsResourceName());
+    builtin.add(new Status5XXDecorator());
 
     return builtin;
   }

dd-trace-ot/src/main/java/datadog/opentracing/decorators/Status5XXDecorator.java

@@ -0,0 +1,24 @@
+package datadog.opentracing.decorators;
+
+import datadog.opentracing.DDSpanContext;
+import io.opentracing.tag.Tags;
+
+/** Mark all 5xx status codes as an error */
+public class Status5XXDecorator extends AbstractDecorator {
+  public Status5XXDecorator() {
+    super();
+    this.setMatchingTag(Tags.HTTP_STATUS.getKey());
+  }
+
+  @Override
+  public boolean afterSetTag(final DDSpanContext context, final String tag, final Object value) {
+    if (Tags.HTTP_STATUS.getKey().equals(tag)) {
+      final int responseCode = Integer.parseInt(value.toString());
+      if (500 <= responseCode && responseCode < 600) {
+        context.setTag(Tags.ERROR.getKey(), true);
+        return true;
+      }
+    }
+    return false;
+  }
+}