diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/ConfiguredResponseStatusExceptions.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/ConfiguredResponseStatusExceptions.java new file mode 100644 index 00000000000..e15254ba2c9 --- /dev/null +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/ConfiguredResponseStatusExceptions.java @@ -0,0 +1,60 @@ +package datadog.trace.bootstrap.instrumentation.decorator; + +import datadog.trace.api.Config; +import datadog.trace.bootstrap.ClassHierarchyIterable; +import java.util.Map; + +/** + * Lets users teach the tracer about their own, framework-agnostic exception types via {@code + * trace.response-status.exceptions} ({@code DD_TRACE_RESPONSE_STATUS_EXCEPTIONS}): a list of {@code + * fully.qualified.ExceptionClass#accessorMethod} entries. When a configured exception (or a + * subclass of one) is thrown from a request handler, the named no-arg accessor is invoked + * reflectively and its numeric return value is used as the HTTP status for deciding whether the + * span should be flagged as an error, instead of unconditionally marking it as an error. + * + *

Only ever reflects on classes/methods the user explicitly named, unlike a generic heuristic + * that would probe arbitrary exceptions for common accessor names and risk silently clearing a + * genuine error whose exception happens to have a same-named, unrelated method. + */ +public final class ConfiguredResponseStatusExceptions { + + public static Integer extractStatus(final Throwable throwable) { + Map accessors = Config.get().getResponseStatusExceptionAccessors(); + if (accessors.isEmpty()) { + return null; + } + Throwable current = throwable; + for (int depth = 0; current != null && depth < 5; depth++, current = current.getCause()) { + for (Class type : new ClassHierarchyIterable(current.getClass())) { + String methodName = accessors.get(type.getName()); + if (methodName != null) { + Integer status = invoke(current, methodName); + if (status != null) { + return status; + } + } + } + } + return null; + } + + private static Integer invoke(final Throwable throwable, final String methodName) { + try { + Object result = throwable.getClass().getMethod(methodName).invoke(throwable); + if (result instanceof Number) { + int status = ((Number) result).intValue(); + // Guard against sentinel values (e.g. -1 for "unknown") reflectively returned by a + // misconfigured accessor: an out-of-range status would otherwise throw when later used + // to index into Config#getHttpServerErrorStatuses, aborting normal error handling. + if (status >= 100 && status <= 599) { + return status; + } + } + } catch (Throwable ignored) { + // misconfigured entry (wrong method name, non-numeric return, etc.) -- fall through + } + return null; + } + + private ConfiguredResponseStatusExceptions() {} +} diff --git a/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/JakartaRsAnnotationsDecorator.java b/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/JakartaRsAnnotationsDecorator.java index 57d41ff0012..22615f863ea 100644 --- a/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/JakartaRsAnnotationsDecorator.java +++ b/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/JakartaRsAnnotationsDecorator.java @@ -2,16 +2,20 @@ import static datadog.trace.bootstrap.instrumentation.decorator.http.HttpResourceDecorator.HTTP_RESOURCE_DECORATOR; +import datadog.trace.api.Config; import datadog.trace.api.GenericClassValue; import datadog.trace.api.Pair; import datadog.trace.bootstrap.ClassHierarchyIterable; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; +import datadog.trace.bootstrap.instrumentation.api.ErrorPriorities; import datadog.trace.bootstrap.instrumentation.api.InternalSpanTypes; import datadog.trace.bootstrap.instrumentation.api.Tags; import datadog.trace.bootstrap.instrumentation.api.UTF8BytesString; import datadog.trace.bootstrap.instrumentation.decorator.BaseDecorator; +import datadog.trace.bootstrap.instrumentation.decorator.ConfiguredResponseStatusExceptions; import jakarta.ws.rs.HttpMethod; import jakarta.ws.rs.Path; +import jakarta.ws.rs.WebApplicationException; import java.lang.annotation.Annotation; import java.lang.reflect.Method; import java.util.Map; @@ -53,6 +57,37 @@ protected CharSequence component() { return JAKARTA_RS_CONTROLLER; } + @Override + protected void doOnError(final AgentSpan span, final Throwable throwable, byte errorPriority) { + // If the mapped status isn't one of the configured "server error" statuses, this isn't really + // an error from the caller's point of view (e.g. a 404 NotFoundException), even though a Java + // exception was thrown to get there. + Integer status = extractResponseStatus(throwable); + if (status == null) { + status = ConfiguredResponseStatusExceptions.extractStatus(throwable); + } + if (status != null) { + span.addThrowable(throwable, ErrorPriorities.HTTP_SERVER_DECORATOR); + span.setError( + Config.get().getHttpServerErrorStatuses().get(status), + ErrorPriorities.HTTP_SERVER_DECORATOR); + return; + } + super.doOnError(span, throwable, errorPriority); + } + + // Walk the cause chain looking for a WebApplicationException, which carries the response the + // framework will actually send. + private static Integer extractResponseStatus(final Throwable throwable) { + Throwable current = throwable; + for (int depth = 0; current != null && depth < 5; depth++, current = current.getCause()) { + if (current instanceof WebApplicationException) { + return ((WebApplicationException) current).getResponse().getStatus(); + } + } + return null; + } + public void onJakartaRsSpan( final AgentSpan span, final AgentSpan parent, final Class target, final Method method) { diff --git a/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/test/groovy/JakartaRsAnnotations3InstrumentationTest.groovy b/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/test/groovy/JakartaRsAnnotations3InstrumentationTest.groovy index 2d68787a746..933f79bc3fd 100644 --- a/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/test/groovy/JakartaRsAnnotations3InstrumentationTest.groovy +++ b/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/test/groovy/JakartaRsAnnotations3InstrumentationTest.groovy @@ -11,6 +11,8 @@ import jakarta.ws.rs.PATCH import jakarta.ws.rs.POST import jakarta.ws.rs.PUT import jakarta.ws.rs.Path +import jakarta.ws.rs.WebApplicationException +import jakarta.ws.rs.core.Response import static datadog.trace.agent.test.utils.TraceUtils.runUnderTrace @@ -140,6 +142,185 @@ class JakartaRsAnnotations3InstrumentationTest extends InstrumentationSpecificat className = JakartaRsAnnotationsDecorator.DECORATE.className(obj.class) } + def "resource method exception with an embedded non-5xx status is not flagged as an error"() { + setup: + // jakarta-rs-annotations-3.0's test classpath has no JAX-RS runtime implementation, so + // WebApplicationException's convenience constructors (which build a Response via + // RuntimeDelegate) can't be used here. Stub the Response instead. + def response = Stub(Response) { + getStatus() >> 404 + getStatusInfo() >> Response.Status.NOT_FOUND + } + def obj = new Jakarta() { + @GET + @Path("/not-found") + void call() { + throw new WebApplicationException(response) + } + } + + when: + obj.call() + + then: + thrown(WebApplicationException) + assertTraces(1) { + trace(1) { + span { + operationName "jakarta-rs.request" + resourceName "GET /not-found" + spanType "web" + errored false + tags { + "$Tags.COMPONENT" "jakarta-rs-controller" + "$Tags.HTTP_ROUTE" "/not-found" + errorTags(WebApplicationException, "HTTP 404 Not Found") + defaultTags() + } + } + } + } + } + + def "resource method exception with an embedded 5xx status is flagged as an error"() { + setup: + def response = Stub(Response) { + getStatus() >> 500 + getStatusInfo() >> Response.Status.INTERNAL_SERVER_ERROR + } + def obj = new Jakarta() { + @GET + @Path("/internal-error") + void call() { + throw new WebApplicationException(response) + } + } + + when: + obj.call() + + then: + thrown(WebApplicationException) + assertTraces(1) { + trace(1) { + span { + operationName "jakarta-rs.request" + resourceName "GET /internal-error" + spanType "web" + errored true + tags { + "$Tags.COMPONENT" "jakarta-rs-controller" + "$Tags.HTTP_ROUTE" "/internal-error" + errorTags(WebApplicationException, "HTTP 500 Internal Server Error") + defaultTags() + } + } + } + } + } + + def "resource method with a plain exception is still flagged as an error"() { + setup: + def obj = new Jakarta() { + @GET + @Path("/boom") + void call() { + throw new IllegalStateException("boom") + } + } + + when: + obj.call() + + then: + thrown(IllegalStateException) + assertTraces(1) { + trace(1) { + span { + operationName "jakarta-rs.request" + resourceName "GET /boom" + spanType "web" + errored true + tags { + "$Tags.COMPONENT" "jakarta-rs-controller" + "$Tags.HTTP_ROUTE" "/boom" + errorTags(IllegalStateException, "boom") + defaultTags() + } + } + } + } + } + + + def "resource method exception recognized via configured custom accessor with a non-5xx status is not flagged as an error"() { + setup: + injectSysConfig(TraceInstrumentationConfig.RESPONSE_STATUS_EXCEPTIONS, "${CustomStatusException.name}#httpCode") + def obj = new Jakarta() { + @GET + @Path("/custom-not-found") + void call() { + throw new CustomStatusException(404) + } + } + + when: + obj.call() + + then: + thrown(CustomStatusException) + assertTraces(1) { + trace(1) { + span { + operationName "jakarta-rs.request" + resourceName "GET /custom-not-found" + spanType "web" + errored false + tags { + "$Tags.COMPONENT" "jakarta-rs-controller" + "$Tags.HTTP_ROUTE" "/custom-not-found" + errorTags(CustomStatusException) + defaultTags() + } + } + } + } + } + + def "resource method exception recognized via configured custom accessor with a 5xx status is flagged as an error"() { + setup: + injectSysConfig(TraceInstrumentationConfig.RESPONSE_STATUS_EXCEPTIONS, "${CustomStatusException.name}#httpCode") + def obj = new Jakarta() { + @GET + @Path("/custom-internal-error") + void call() { + throw new CustomStatusException(500) + } + } + + when: + obj.call() + + then: + thrown(CustomStatusException) + assertTraces(1) { + trace(1) { + span { + operationName "jakarta-rs.request" + resourceName "GET /custom-internal-error" + spanType "web" + errored true + tags { + "$Tags.COMPONENT" "jakarta-rs-controller" + "$Tags.HTTP_ROUTE" "/custom-internal-error" + errorTags(CustomStatusException) + defaultTags() + } + } + } + } + } + def "no annotations has no effect"() { setup: def obj = new Jakarta() { @@ -164,6 +345,18 @@ class JakartaRsAnnotations3InstrumentationTest extends InstrumentationSpecificat } } + static class CustomStatusException extends RuntimeException { + private final int status + + CustomStatusException(int status) { + this.status = status + } + + int httpCode() { + return status + } + } + interface Jakarta { void call() } diff --git a/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-1.1.1/src/main/java/datadog/trace/instrumentation/jaxrs1/JaxRsAnnotationsDecorator.java b/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-1.1.1/src/main/java/datadog/trace/instrumentation/jaxrs1/JaxRsAnnotationsDecorator.java index 7186c507c13..b440b5b6f4b 100644 --- a/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-1.1.1/src/main/java/datadog/trace/instrumentation/jaxrs1/JaxRsAnnotationsDecorator.java +++ b/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-1.1.1/src/main/java/datadog/trace/instrumentation/jaxrs1/JaxRsAnnotationsDecorator.java @@ -2,21 +2,25 @@ import static datadog.trace.bootstrap.instrumentation.decorator.http.HttpResourceDecorator.HTTP_RESOURCE_DECORATOR; +import datadog.trace.api.Config; import datadog.trace.api.GenericClassValue; import datadog.trace.api.Pair; import datadog.trace.bootstrap.ClassHierarchyIterable; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; +import datadog.trace.bootstrap.instrumentation.api.ErrorPriorities; import datadog.trace.bootstrap.instrumentation.api.InternalSpanTypes; import datadog.trace.bootstrap.instrumentation.api.ResourceNamePriorities; import datadog.trace.bootstrap.instrumentation.api.Tags; import datadog.trace.bootstrap.instrumentation.api.UTF8BytesString; import datadog.trace.bootstrap.instrumentation.decorator.BaseDecorator; +import datadog.trace.bootstrap.instrumentation.decorator.ConfiguredResponseStatusExceptions; import java.lang.annotation.Annotation; import java.lang.reflect.Method; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import javax.ws.rs.HttpMethod; import javax.ws.rs.Path; +import javax.ws.rs.WebApplicationException; public class JaxRsAnnotationsDecorator extends BaseDecorator { public static JaxRsAnnotationsDecorator DECORATE = new JaxRsAnnotationsDecorator(); @@ -41,6 +45,37 @@ protected CharSequence component() { return JAX_RS_CONTROLLER; } + @Override + protected void doOnError(final AgentSpan span, final Throwable throwable, byte errorPriority) { + // If the mapped status isn't one of the configured "server error" statuses, this isn't really + // an error from the caller's point of view (e.g. a 404 NotFoundException), even though a Java + // exception was thrown to get there. + Integer status = extractResponseStatus(throwable); + if (status == null) { + status = ConfiguredResponseStatusExceptions.extractStatus(throwable); + } + if (status != null) { + span.addThrowable(throwable, ErrorPriorities.HTTP_SERVER_DECORATOR); + span.setError( + Config.get().getHttpServerErrorStatuses().get(status), + ErrorPriorities.HTTP_SERVER_DECORATOR); + return; + } + super.doOnError(span, throwable, errorPriority); + } + + // Walk the cause chain looking for a WebApplicationException, which carries the response the + // framework will actually send. + private static Integer extractResponseStatus(final Throwable throwable) { + Throwable current = throwable; + for (int depth = 0; current != null && depth < 5; depth++, current = current.getCause()) { + if (current instanceof WebApplicationException) { + return ((WebApplicationException) current).getResponse().getStatus(); + } + } + return null; + } + public void onJaxRsSpan( final AgentSpan span, final AgentSpan parent, final Class target, final Method method) { diff --git a/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-1.1.1/src/test/groovy/JaxRsAnnotations1InstrumentationTest.groovy b/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-1.1.1/src/test/groovy/JaxRsAnnotations1InstrumentationTest.groovy index f54ddde50f5..83c2f9f70aa 100644 --- a/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-1.1.1/src/test/groovy/JaxRsAnnotations1InstrumentationTest.groovy +++ b/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-1.1.1/src/test/groovy/JaxRsAnnotations1InstrumentationTest.groovy @@ -13,6 +13,7 @@ import javax.ws.rs.OPTIONS import javax.ws.rs.POST import javax.ws.rs.PUT import javax.ws.rs.Path +import javax.ws.rs.WebApplicationException class JaxRsAnnotations1InstrumentationTest extends InstrumentationSpecification { @@ -140,6 +141,173 @@ class JaxRsAnnotations1InstrumentationTest extends InstrumentationSpecification className = JaxRsAnnotationsDecorator.DECORATE.className(obj.class) } + def "resource method exception with an embedded non-5xx status is not flagged as an error"() { + setup: + def obj = new Jax() { + @GET + @Path("/not-found") + void call() { + throw new WebApplicationException(404) + } + } + + when: + obj.call() + + then: + thrown(WebApplicationException) + assertTraces(1) { + trace(1) { + span { + operationName "jax-rs.request" + resourceName "GET /not-found" + spanType "web" + errored false + tags { + "$Tags.COMPONENT" "jax-rs-controller" + "$Tags.HTTP_ROUTE" "/not-found" + errorTags(WebApplicationException) + defaultTags() + } + } + } + } + } + + def "resource method exception with an embedded 5xx status is flagged as an error"() { + setup: + def obj = new Jax() { + @GET + @Path("/internal-error") + void call() { + throw new WebApplicationException(500) + } + } + + when: + obj.call() + + then: + thrown(WebApplicationException) + assertTraces(1) { + trace(1) { + span { + operationName "jax-rs.request" + resourceName "GET /internal-error" + spanType "web" + errored true + tags { + "$Tags.COMPONENT" "jax-rs-controller" + "$Tags.HTTP_ROUTE" "/internal-error" + errorTags(WebApplicationException) + defaultTags() + } + } + } + } + } + + def "resource method with a plain exception is still flagged as an error"() { + setup: + def obj = new Jax() { + @GET + @Path("/boom") + void call() { + throw new IllegalStateException("boom") + } + } + + when: + obj.call() + + then: + thrown(IllegalStateException) + assertTraces(1) { + trace(1) { + span { + operationName "jax-rs.request" + resourceName "GET /boom" + spanType "web" + errored true + tags { + "$Tags.COMPONENT" "jax-rs-controller" + "$Tags.HTTP_ROUTE" "/boom" + errorTags(IllegalStateException, "boom") + defaultTags() + } + } + } + } + } + + def "resource method exception recognized via configured custom accessor with a non-5xx status is not flagged as an error"() { + setup: + injectSysConfig(TraceInstrumentationConfig.RESPONSE_STATUS_EXCEPTIONS, "${CustomStatusException.name}#httpCode") + def obj = new Jax() { + @GET + @Path("/custom-not-found") + void call() { + throw new CustomStatusException(404) + } + } + + when: + obj.call() + + then: + thrown(CustomStatusException) + assertTraces(1) { + trace(1) { + span { + operationName "jax-rs.request" + resourceName "GET /custom-not-found" + spanType "web" + errored false + tags { + "$Tags.COMPONENT" "jax-rs-controller" + "$Tags.HTTP_ROUTE" "/custom-not-found" + errorTags(CustomStatusException) + defaultTags() + } + } + } + } + } + + def "resource method exception recognized via configured custom accessor with a 5xx status is flagged as an error"() { + setup: + injectSysConfig(TraceInstrumentationConfig.RESPONSE_STATUS_EXCEPTIONS, "${CustomStatusException.name}#httpCode") + def obj = new Jax() { + @GET + @Path("/custom-internal-error") + void call() { + throw new CustomStatusException(500) + } + } + + when: + obj.call() + + then: + thrown(CustomStatusException) + assertTraces(1) { + trace(1) { + span { + operationName "jax-rs.request" + resourceName "GET /custom-internal-error" + spanType "web" + errored true + tags { + "$Tags.COMPONENT" "jax-rs-controller" + "$Tags.HTTP_ROUTE" "/custom-internal-error" + errorTags(CustomStatusException) + defaultTags() + } + } + } + } + } + def "no annotations has no effect"() { setup: def obj = new Jax() { @@ -164,6 +332,18 @@ class JaxRsAnnotations1InstrumentationTest extends InstrumentationSpecification } } + static class CustomStatusException extends RuntimeException { + private final int status + + CustomStatusException(int status) { + this.status = status + } + + int httpCode() { + return status + } + } + interface Jax { void call() } diff --git a/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsDecorator.java b/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsDecorator.java index a7c8e82f2d8..397665b59ab 100644 --- a/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsDecorator.java +++ b/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsDecorator.java @@ -2,21 +2,25 @@ import static datadog.trace.bootstrap.instrumentation.decorator.http.HttpResourceDecorator.HTTP_RESOURCE_DECORATOR; +import datadog.trace.api.Config; import datadog.trace.api.GenericClassValue; import datadog.trace.api.Pair; import datadog.trace.bootstrap.ClassHierarchyIterable; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; +import datadog.trace.bootstrap.instrumentation.api.ErrorPriorities; import datadog.trace.bootstrap.instrumentation.api.InternalSpanTypes; import datadog.trace.bootstrap.instrumentation.api.ResourceNamePriorities; import datadog.trace.bootstrap.instrumentation.api.Tags; import datadog.trace.bootstrap.instrumentation.api.UTF8BytesString; import datadog.trace.bootstrap.instrumentation.decorator.BaseDecorator; +import datadog.trace.bootstrap.instrumentation.decorator.ConfiguredResponseStatusExceptions; import java.lang.annotation.Annotation; import java.lang.reflect.Method; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import javax.ws.rs.HttpMethod; import javax.ws.rs.Path; +import javax.ws.rs.WebApplicationException; public class JaxRsAnnotationsDecorator extends BaseDecorator { @@ -52,6 +56,37 @@ protected CharSequence component() { return JAX_RS_CONTROLLER; } + @Override + protected void doOnError(final AgentSpan span, final Throwable throwable, byte errorPriority) { + // If the mapped status isn't one of the configured "server error" statuses, this isn't really + // an error from the caller's point of view (e.g. a 404 NotFoundException), even though a Java + // exception was thrown to get there. + Integer status = extractResponseStatus(throwable); + if (status == null) { + status = ConfiguredResponseStatusExceptions.extractStatus(throwable); + } + if (status != null) { + span.addThrowable(throwable, ErrorPriorities.HTTP_SERVER_DECORATOR); + span.setError( + Config.get().getHttpServerErrorStatuses().get(status), + ErrorPriorities.HTTP_SERVER_DECORATOR); + return; + } + super.doOnError(span, throwable, errorPriority); + } + + // Walk the cause chain looking for a WebApplicationException, which carries the response the + // framework will actually send. + private static Integer extractResponseStatus(final Throwable throwable) { + Throwable current = throwable; + for (int depth = 0; current != null && depth < 5; depth++, current = current.getCause()) { + if (current instanceof WebApplicationException) { + return ((WebApplicationException) current).getResponse().getStatus(); + } + } + return null; + } + public void onJaxRsSpan( final AgentSpan span, final AgentSpan parent, final Class target, final Method method) { diff --git a/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/test/groovy/JaxRsAnnotations2InstrumentationTest.groovy b/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/test/groovy/JaxRsAnnotations2InstrumentationTest.groovy index fe542a6baac..e2c7074e4b1 100644 --- a/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/test/groovy/JaxRsAnnotations2InstrumentationTest.groovy +++ b/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/test/groovy/JaxRsAnnotations2InstrumentationTest.groovy @@ -7,10 +7,12 @@ import io.dropwizard.jersey.PATCH import javax.ws.rs.DELETE import javax.ws.rs.GET import javax.ws.rs.HEAD +import javax.ws.rs.NotFoundException import javax.ws.rs.OPTIONS import javax.ws.rs.POST import javax.ws.rs.PUT import javax.ws.rs.Path +import javax.ws.rs.WebApplicationException import static datadog.trace.agent.test.utils.TraceUtils.runUnderTrace @@ -140,6 +142,173 @@ class JaxRsAnnotations2InstrumentationTest extends InstrumentationSpecification className = JaxRsAnnotationsDecorator.DECORATE.className(obj.class) } + def "resource method exception with an embedded non-5xx status is not flagged as an error"() { + setup: + def obj = new Jax() { + @GET + @Path("/not-found") + void call() { + throw new NotFoundException() + } + } + + when: + obj.call() + + then: + thrown(NotFoundException) + assertTraces(1) { + trace(1) { + span { + operationName "jax-rs.request" + resourceName "GET /not-found" + spanType "web" + errored false + tags { + "$Tags.COMPONENT" "jax-rs-controller" + "$Tags.HTTP_ROUTE" "/not-found" + errorTags(NotFoundException, "HTTP 404 Not Found") + defaultTags() + } + } + } + } + } + + def "resource method exception with an embedded 5xx status is flagged as an error"() { + setup: + def obj = new Jax() { + @GET + @Path("/internal-error") + void call() { + throw new WebApplicationException(500) + } + } + + when: + obj.call() + + then: + thrown(WebApplicationException) + assertTraces(1) { + trace(1) { + span { + operationName "jax-rs.request" + resourceName "GET /internal-error" + spanType "web" + errored true + tags { + "$Tags.COMPONENT" "jax-rs-controller" + "$Tags.HTTP_ROUTE" "/internal-error" + errorTags(WebApplicationException, "HTTP 500 Internal Server Error") + defaultTags() + } + } + } + } + } + + def "resource method with a plain exception is still flagged as an error"() { + setup: + def obj = new Jax() { + @GET + @Path("/boom") + void call() { + throw new IllegalStateException("boom") + } + } + + when: + obj.call() + + then: + thrown(IllegalStateException) + assertTraces(1) { + trace(1) { + span { + operationName "jax-rs.request" + resourceName "GET /boom" + spanType "web" + errored true + tags { + "$Tags.COMPONENT" "jax-rs-controller" + "$Tags.HTTP_ROUTE" "/boom" + errorTags(IllegalStateException, "boom") + defaultTags() + } + } + } + } + } + + def "resource method exception recognized via configured custom accessor with a non-5xx status is not flagged as an error"() { + setup: + injectSysConfig(TraceInstrumentationConfig.RESPONSE_STATUS_EXCEPTIONS, "${CustomStatusException.name}#httpCode") + def obj = new Jax() { + @GET + @Path("/custom-not-found") + void call() { + throw new CustomStatusException(404) + } + } + + when: + obj.call() + + then: + thrown(CustomStatusException) + assertTraces(1) { + trace(1) { + span { + operationName "jax-rs.request" + resourceName "GET /custom-not-found" + spanType "web" + errored false + tags { + "$Tags.COMPONENT" "jax-rs-controller" + "$Tags.HTTP_ROUTE" "/custom-not-found" + errorTags(CustomStatusException) + defaultTags() + } + } + } + } + } + + def "resource method exception recognized via configured custom accessor with a 5xx status is flagged as an error"() { + setup: + injectSysConfig(TraceInstrumentationConfig.RESPONSE_STATUS_EXCEPTIONS, "${CustomStatusException.name}#httpCode") + def obj = new Jax() { + @GET + @Path("/custom-internal-error") + void call() { + throw new CustomStatusException(500) + } + } + + when: + obj.call() + + then: + thrown(CustomStatusException) + assertTraces(1) { + trace(1) { + span { + operationName "jax-rs.request" + resourceName "GET /custom-internal-error" + spanType "web" + errored true + tags { + "$Tags.COMPONENT" "jax-rs-controller" + "$Tags.HTTP_ROUTE" "/custom-internal-error" + errorTags(CustomStatusException) + defaultTags() + } + } + } + } + } + def "no annotations has no effect"() { setup: def obj = new Jax() { @@ -164,6 +333,18 @@ class JaxRsAnnotations2InstrumentationTest extends InstrumentationSpecification } } + static class CustomStatusException extends RuntimeException { + private final int status + + CustomStatusException(int status) { + this.status = status + } + + int httpCode() { + return status + } + } + interface Jax { void call() } diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/latestDepTest/groovy/datadog/trace/instrumentation/springweb/SpringWebHttpServerDecoratorLatestDepTest.groovy b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/latestDepTest/groovy/datadog/trace/instrumentation/springweb/SpringWebHttpServerDecoratorLatestDepTest.groovy new file mode 100644 index 00000000000..a9342137ca7 --- /dev/null +++ b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/latestDepTest/groovy/datadog/trace/instrumentation/springweb/SpringWebHttpServerDecoratorLatestDepTest.groovy @@ -0,0 +1,77 @@ +package datadog.trace.instrumentation.springweb + +import datadog.trace.agent.test.InstrumentationSpecification +import datadog.trace.bootstrap.instrumentation.api.AgentTracer +import datadog.trace.bootstrap.instrumentation.api.Tags +import org.springframework.http.HttpStatus +import org.springframework.web.server.ResponseStatusException + +import static datadog.trace.instrumentation.springweb.SpringWebHttpServerDecorator.DECORATE + +// ResponseStatusException was added in Spring 5.0, which is only guaranteed to be on the +// classpath for the latestDepTest suite (this module's base test classpath is Spring 3.1). This +// exercises the reflective ResponseStatusException handling in SpringWebHttpServerDecorator. +class SpringWebHttpServerDecoratorLatestDepTest extends InstrumentationSpecification { + + def "ResponseStatusException with a non-5xx status is not flagged as an error"() { + setup: + def testSpan = AgentTracer.startSpan("spring-web-controller", "spring.handler") + def scope = AgentTracer.activateSpan(testSpan) + DECORATE.afterStart(testSpan) + def throwable = new ResponseStatusException(HttpStatus.NOT_FOUND) + + when: + DECORATE.onError(testSpan, throwable) + DECORATE.beforeFinish(testSpan) + scope.close() + testSpan.finish() + + then: + assertTraces(1) { + trace(1) { + span { + operationName "spring.handler" + spanType "web" + errored false + tags { + "$Tags.COMPONENT" "spring-web-controller" + "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER + errorTags(ResponseStatusException, throwable.message) + defaultTags() + } + } + } + } + } + + def "ResponseStatusException with a 5xx status is flagged as an error"() { + setup: + def testSpan = AgentTracer.startSpan("spring-web-controller", "spring.handler") + def scope = AgentTracer.activateSpan(testSpan) + DECORATE.afterStart(testSpan) + def throwable = new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR) + + when: + DECORATE.onError(testSpan, throwable) + DECORATE.beforeFinish(testSpan) + scope.close() + testSpan.finish() + + then: + assertTraces(1) { + trace(1) { + span { + operationName "spring.handler" + spanType "web" + errored true + tags { + "$Tags.COMPONENT" "spring-web-controller" + "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER + errorTags(ResponseStatusException, throwable.message) + defaultTags() + } + } + } + } + } +} diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/SpringWebHttpServerDecorator.java b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/SpringWebHttpServerDecorator.java index 473b7b2b050..96c33e8f3cc 100644 --- a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/SpringWebHttpServerDecorator.java +++ b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/SpringWebHttpServerDecorator.java @@ -3,16 +3,22 @@ import static datadog.trace.bootstrap.instrumentation.decorator.http.HttpResourceDecorator.HTTP_RESOURCE_DECORATOR; import datadog.context.Context; +import datadog.trace.api.Config; +import datadog.trace.bootstrap.ClassHierarchyIterable; import datadog.trace.bootstrap.instrumentation.api.AgentPropagation; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; +import datadog.trace.bootstrap.instrumentation.api.ErrorPriorities; import datadog.trace.bootstrap.instrumentation.api.URIDataAdapter; import datadog.trace.bootstrap.instrumentation.api.UTF8BytesString; +import datadog.trace.bootstrap.instrumentation.decorator.ConfiguredResponseStatusExceptions; import datadog.trace.bootstrap.instrumentation.decorator.HttpServerDecorator; import java.lang.reflect.Method; import javax.servlet.Servlet; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import org.springframework.http.HttpStatus; import org.springframework.web.HttpRequestHandler; +import org.springframework.web.bind.annotation.ResponseStatus; import org.springframework.web.method.HandlerMethod; import org.springframework.web.servlet.HandlerMapping; import org.springframework.web.servlet.ModelAndView; @@ -21,6 +27,38 @@ public class SpringWebHttpServerDecorator extends HttpServerDecorator { + // ResponseStatusException was added in Spring 5.0; this module also supports Spring 3.1-4.x, + // so it can't be referenced directly. Resolved reflectively, once, and reused. + private static final Method RESPONSE_STATUS_EXCEPTION_GET_STATUS = + findResponseStatusExceptionGetStatus(); + + private static Method findResponseStatusExceptionGetStatus() { + try { + Class responseStatusExceptionClass = + Class.forName( + "org.springframework.web.server.ResponseStatusException", + false, + SpringWebHttpServerDecorator.class.getClassLoader()); + return responseStatusExceptionClass.getMethod("getStatus"); + } catch (ClassNotFoundException | NoSuchMethodException e) { + return null; + } + } + + // ResponseStatus#code() was added in Spring 4.2 as an alias of value(); this module compiles + // against Spring 3.1, so it can't be referenced directly. Resolved reflectively, once, and + // reused. Plain reflection doesn't resolve Spring's @AliasFor, so if a caller only set code(), + // value() still reports its own default rather than the value mirrored from code(). + private static final Method RESPONSE_STATUS_CODE = findResponseStatusCode(); + + private static Method findResponseStatusCode() { + try { + return ResponseStatus.class.getMethod("code"); + } catch (NoSuchMethodException e) { + return null; + } + } + private static final CharSequence SPRING_HANDLER = UTF8BytesString.create("spring.handler"); public static final CharSequence RESPONSE_RENDER = UTF8BytesString.create("response.render"); @@ -97,6 +135,64 @@ protected String getRequestHeader(final HttpServletRequest request, String key) return request.getHeader(key); } + @Override + protected void doOnError(final AgentSpan span, final Throwable throwable, byte errorPriority) { + // Walk the cause chain looking for a status the exception itself carries (@ResponseStatus, or + // a ResponseStatusException on Spring 5+). If the mapped status isn't one of the configured + // "server error" statuses, this isn't really an error from the caller's point of view (e.g. a + // 404 mapping), even though a Java exception was thrown to get there. + Integer status = extractResponseStatus(throwable); + if (status == null) { + status = ConfiguredResponseStatusExceptions.extractStatus(throwable); + } + if (status != null) { + span.addThrowable(throwable, ErrorPriorities.HTTP_SERVER_DECORATOR); + span.setError( + Config.get().getHttpServerErrorStatuses().get(status), + ErrorPriorities.HTTP_SERVER_DECORATOR); + return; + } + super.doOnError(span, throwable, errorPriority); + } + + private static Integer extractResponseStatus(final Throwable throwable) { + Throwable current = throwable; + for (int depth = 0; current != null && depth < 5; depth++, current = current.getCause()) { + if (RESPONSE_STATUS_EXCEPTION_GET_STATUS != null + && RESPONSE_STATUS_EXCEPTION_GET_STATUS.getDeclaringClass().isInstance(current)) { + try { + Object httpStatus = RESPONSE_STATUS_EXCEPTION_GET_STATUS.invoke(current); + if (httpStatus instanceof HttpStatus) { + return ((HttpStatus) httpStatus).value(); + } + } catch (Throwable ignored) { + // fall through to the @ResponseStatus check below + } + } + for (Class type : new ClassHierarchyIterable(current.getClass())) { + ResponseStatus responseStatus = type.getAnnotation(ResponseStatus.class); + if (responseStatus != null) { + return responseStatusCode(responseStatus); + } + } + } + return null; + } + + private static int responseStatusCode(final ResponseStatus responseStatus) { + if (RESPONSE_STATUS_CODE != null) { + try { + HttpStatus code = (HttpStatus) RESPONSE_STATUS_CODE.invoke(responseStatus); + if (code != HttpStatus.INTERNAL_SERVER_ERROR) { + return code.value(); + } + } catch (Throwable ignored) { + // fall through to value() below + } + } + return responseStatus.value().value(); + } + @Override protected void doOnRequest( final AgentSpan span, diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/test/groovy/datadog/trace/instrumentation/springweb/SpringWebHttpServerDecoratorTest.groovy b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/test/groovy/datadog/trace/instrumentation/springweb/SpringWebHttpServerDecoratorTest.groovy new file mode 100644 index 00000000000..a49c90d9132 --- /dev/null +++ b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/test/groovy/datadog/trace/instrumentation/springweb/SpringWebHttpServerDecoratorTest.groovy @@ -0,0 +1,235 @@ +package datadog.trace.instrumentation.springweb + +import datadog.trace.agent.test.InstrumentationSpecification +import datadog.trace.api.config.TraceInstrumentationConfig +import datadog.trace.bootstrap.instrumentation.api.AgentTracer +import datadog.trace.bootstrap.instrumentation.api.Tags +import org.springframework.http.HttpStatus +import org.springframework.web.bind.annotation.ResponseStatus + +import static datadog.trace.instrumentation.springweb.SpringWebHttpServerDecorator.DECORATE + +// ResponseStatusException was added in Spring 5.0 and isn't available on this module's base +// (Spring 3.1) test classpath. Its handling is covered separately, on the latestDepTest +// classpath, by SpringWebHttpServerDecoratorLatestDepTest. +class SpringWebHttpServerDecoratorTest extends InstrumentationSpecification { + + @ResponseStatus(HttpStatus.NOT_FOUND) + static class CustomNotFoundException extends RuntimeException { + } + + @ResponseStatus(HttpStatus.INTERNAL_SERVER_ERROR) + static class CustomServerErrorException extends RuntimeException { + } + + // Spring's @AliasFor("value") on ResponseStatus#code isn't honored by plain reflection, so + // setting only code() (rather than value()) exercises a separate code path. + @ResponseStatus(code = HttpStatus.NOT_FOUND) + static class CustomNotFoundExceptionUsingCodeAttribute extends RuntimeException { + } + + static class CustomStatusException extends RuntimeException { + private final int status + + CustomStatusException(int status) { + this.status = status + } + + int httpCode() { + return status + } + } + + def "exception with an embedded non-5xx status is not flagged as an error"() { + setup: + def testSpan = AgentTracer.startSpan("spring-web-controller", "spring.handler") + def scope = AgentTracer.activateSpan(testSpan) + DECORATE.afterStart(testSpan) + + when: + DECORATE.onError(testSpan, throwable) + DECORATE.beforeFinish(testSpan) + scope.close() + testSpan.finish() + + then: + assertTraces(1) { + trace(1) { + span { + operationName "spring.handler" + spanType "web" + errored false + tags { + "$Tags.COMPONENT" "spring-web-controller" + "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER + errorTags(throwable.class) + defaultTags() + } + } + } + } + + where: + throwable << [new CustomNotFoundException(), new CustomNotFoundExceptionUsingCodeAttribute()] + } + + def "exception with an embedded 5xx status is flagged as an error"() { + setup: + def testSpan = AgentTracer.startSpan("spring-web-controller", "spring.handler") + def scope = AgentTracer.activateSpan(testSpan) + DECORATE.afterStart(testSpan) + def throwable = new CustomServerErrorException() + + when: + DECORATE.onError(testSpan, throwable) + DECORATE.beforeFinish(testSpan) + scope.close() + testSpan.finish() + + then: + assertTraces(1) { + trace(1) { + span { + operationName "spring.handler" + spanType "web" + errored true + tags { + "$Tags.COMPONENT" "spring-web-controller" + "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER + errorTags(CustomServerErrorException) + defaultTags() + } + } + } + } + } + + def "exception recognized via configured custom accessor with a non-5xx status is not flagged as an error"() { + setup: + injectSysConfig(TraceInstrumentationConfig.RESPONSE_STATUS_EXCEPTIONS, "${CustomStatusException.name}#httpCode") + def testSpan = AgentTracer.startSpan("spring-web-controller", "spring.handler") + def scope = AgentTracer.activateSpan(testSpan) + DECORATE.afterStart(testSpan) + def throwable = new CustomStatusException(404) + + when: + DECORATE.onError(testSpan, throwable) + DECORATE.beforeFinish(testSpan) + scope.close() + testSpan.finish() + + then: + assertTraces(1) { + trace(1) { + span { + operationName "spring.handler" + spanType "web" + errored false + tags { + "$Tags.COMPONENT" "spring-web-controller" + "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER + errorTags(CustomStatusException) + defaultTags() + } + } + } + } + } + + def "exception recognized via configured custom accessor with a 5xx status is flagged as an error"() { + setup: + injectSysConfig(TraceInstrumentationConfig.RESPONSE_STATUS_EXCEPTIONS, "${CustomStatusException.name}#httpCode") + def testSpan = AgentTracer.startSpan("spring-web-controller", "spring.handler") + def scope = AgentTracer.activateSpan(testSpan) + DECORATE.afterStart(testSpan) + def throwable = new CustomStatusException(500) + + when: + DECORATE.onError(testSpan, throwable) + DECORATE.beforeFinish(testSpan) + scope.close() + testSpan.finish() + + then: + assertTraces(1) { + trace(1) { + span { + operationName "spring.handler" + spanType "web" + errored true + tags { + "$Tags.COMPONENT" "spring-web-controller" + "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER + errorTags(CustomStatusException) + defaultTags() + } + } + } + } + } + + def "exception recognized via configured custom accessor with an out-of-range status falls back to normal error handling"() { + setup: + injectSysConfig(TraceInstrumentationConfig.RESPONSE_STATUS_EXCEPTIONS, "${CustomStatusException.name}#httpCode") + def testSpan = AgentTracer.startSpan("spring-web-controller", "spring.handler") + def scope = AgentTracer.activateSpan(testSpan) + DECORATE.afterStart(testSpan) + // -1 is a plausible "unknown status" sentinel a misconfigured accessor could return; it must + // not be used to index into the configured server-error statuses. + def throwable = new CustomStatusException(-1) + + when: + DECORATE.onError(testSpan, throwable) + DECORATE.beforeFinish(testSpan) + scope.close() + testSpan.finish() + + then: + assertTraces(1) { + trace(1) { + span { + operationName "spring.handler" + spanType "web" + errored true + tags { + "$Tags.COMPONENT" "spring-web-controller" + "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER + errorTags(CustomStatusException) + defaultTags() + } + } + } + } + } + + def "plain exception is still flagged as an error"() { + setup: + def testSpan = AgentTracer.startSpan("spring-web-controller", "spring.handler") + def scope = AgentTracer.activateSpan(testSpan) + DECORATE.afterStart(testSpan) + def throwable = new IllegalStateException("boom") + + when: + DECORATE.onError(testSpan, throwable) + DECORATE.beforeFinish(testSpan) + scope.close() + testSpan.finish() + + then: + assertTraces(1) { + trace(1) { + span { + operationName "spring.handler" + spanType "web" + errored true + tags { + "$Tags.COMPONENT" "spring-web-controller" + "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER + errorTags(IllegalStateException, "boom") + defaultTags() + } + } + } + } + } +} diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/SpringWebHttpServerDecorator.java b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/SpringWebHttpServerDecorator.java index 17f0e93bbe2..14252a48042 100644 --- a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/SpringWebHttpServerDecorator.java +++ b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/SpringWebHttpServerDecorator.java @@ -3,16 +3,24 @@ import static datadog.trace.bootstrap.instrumentation.decorator.http.HttpResourceDecorator.HTTP_RESOURCE_DECORATOR; import datadog.context.Context; +import datadog.trace.api.Config; +import datadog.trace.bootstrap.ClassHierarchyIterable; import datadog.trace.bootstrap.instrumentation.api.AgentPropagation; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; +import datadog.trace.bootstrap.instrumentation.api.ErrorPriorities; import datadog.trace.bootstrap.instrumentation.api.URIDataAdapter; import datadog.trace.bootstrap.instrumentation.api.UTF8BytesString; +import datadog.trace.bootstrap.instrumentation.decorator.ConfiguredResponseStatusExceptions; import datadog.trace.bootstrap.instrumentation.decorator.HttpServerDecorator; import jakarta.servlet.Servlet; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; import java.lang.reflect.Method; +import org.springframework.http.HttpStatus; +import org.springframework.http.HttpStatusCode; +import org.springframework.web.ErrorResponse; import org.springframework.web.HttpRequestHandler; +import org.springframework.web.bind.annotation.ResponseStatus; import org.springframework.web.method.HandlerMethod; import org.springframework.web.servlet.HandlerMapping; import org.springframework.web.servlet.ModelAndView; @@ -101,6 +109,57 @@ protected int status(final HttpServletResponse httpServletResponse) { return httpServletResponse.getStatus(); } + @Override + protected void doOnError(final AgentSpan span, final Throwable throwable, byte errorPriority) { + // Walk the cause chain looking for a status the exception itself carries (@ResponseStatus, + // ResponseStatusException, or any other ErrorResponse such as NoResourceFoundException). If + // the mapped status isn't one of the configured "server error" statuses, this isn't really an + // error from the caller's point of view (e.g. a 404 mapping), even though a Java exception + // was thrown to get there. + Integer status = extractResponseStatus(throwable); + if (status == null) { + status = ConfiguredResponseStatusExceptions.extractStatus(throwable); + } + if (status != null) { + span.addThrowable(throwable, ErrorPriorities.HTTP_SERVER_DECORATOR); + span.setError( + Config.get().getHttpServerErrorStatuses().get(status), + ErrorPriorities.HTTP_SERVER_DECORATOR); + return; + } + super.doOnError(span, throwable, errorPriority); + } + + private static Integer extractResponseStatus(final Throwable throwable) { + Throwable current = throwable; + for (int depth = 0; current != null && depth < 5; depth++, current = current.getCause()) { + if (current instanceof ErrorResponse) { + HttpStatusCode statusCode = ((ErrorResponse) current).getStatusCode(); + if (statusCode != null) { + return statusCode.value(); + } + } + for (Class type : new ClassHierarchyIterable(current.getClass())) { + ResponseStatus responseStatus = type.getAnnotation(ResponseStatus.class); + if (responseStatus != null) { + return responseStatusCode(responseStatus); + } + } + } + return null; + } + + private static int responseStatusCode(final ResponseStatus responseStatus) { + // value() and code() are @AliasFor each other, but plain reflection doesn't resolve + // @AliasFor: if a caller only set code(), value() still reports its own default rather than + // the value mirrored from code(). Prefer code() whenever it was explicitly set. + HttpStatus code = responseStatus.code(); + if (code != HttpStatus.INTERNAL_SERVER_ERROR) { + return code.value(); + } + return responseStatus.value().value(); + } + @Override protected void doOnRequest( final AgentSpan span, diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/test/groovy/datadog/trace/instrumentation/springweb6/SpringWebHttpServerDecoratorTest.groovy b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/test/groovy/datadog/trace/instrumentation/springweb6/SpringWebHttpServerDecoratorTest.groovy new file mode 100644 index 00000000000..3e139b8bb7b --- /dev/null +++ b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/test/groovy/datadog/trace/instrumentation/springweb6/SpringWebHttpServerDecoratorTest.groovy @@ -0,0 +1,211 @@ +package datadog.trace.instrumentation.springweb6 + +import datadog.trace.agent.test.InstrumentationSpecification +import datadog.trace.api.config.TraceInstrumentationConfig +import datadog.trace.bootstrap.instrumentation.api.AgentTracer +import datadog.trace.bootstrap.instrumentation.api.Tags +import org.springframework.http.HttpStatus +import org.springframework.web.ErrorResponseException +import org.springframework.web.bind.annotation.ResponseStatus +import org.springframework.web.server.ResponseStatusException + +import static datadog.trace.instrumentation.springweb6.SpringWebHttpServerDecorator.DECORATE + +class SpringWebHttpServerDecoratorTest extends InstrumentationSpecification { + + @ResponseStatus(HttpStatus.NOT_FOUND) + static class CustomNotFoundException extends RuntimeException { + } + + @ResponseStatus(HttpStatus.INTERNAL_SERVER_ERROR) + static class CustomServerErrorException extends RuntimeException { + } + + // Spring's @AliasFor("value") on ResponseStatus#code isn't honored by plain reflection, so + // setting only code() (rather than value()) exercises a separate code path. + @ResponseStatus(code = HttpStatus.NOT_FOUND) + static class CustomNotFoundExceptionUsingCodeAttribute extends RuntimeException { + } + + static class CustomStatusException extends RuntimeException { + private final int status + + CustomStatusException(int status) { + this.status = status + } + + int httpCode() { + return status + } + } + + def "exception with an embedded non-5xx status is not flagged as an error"() { + setup: + def testSpan = AgentTracer.startSpan("spring-web-controller", "spring.handler") + def scope = AgentTracer.activateSpan(testSpan) + DECORATE.afterStart(testSpan) + + when: + DECORATE.onError(testSpan, throwable) + DECORATE.beforeFinish(testSpan) + scope.close() + testSpan.finish() + + then: + assertTraces(1) { + trace(1) { + span { + operationName "spring.handler" + spanType "web" + errored false + tags { + "$Tags.COMPONENT" "spring-web-controller" + "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER + errorTags(throwable.class, throwable.message) + defaultTags() + } + } + } + } + + where: + throwable << [ + new CustomNotFoundException(), + new CustomNotFoundExceptionUsingCodeAttribute(), + new ResponseStatusException(HttpStatus.NOT_FOUND), + new ErrorResponseException(HttpStatus.NOT_FOUND) + ] + } + + def "exception with an embedded 5xx status is flagged as an error"() { + setup: + def testSpan = AgentTracer.startSpan("spring-web-controller", "spring.handler") + def scope = AgentTracer.activateSpan(testSpan) + DECORATE.afterStart(testSpan) + + when: + DECORATE.onError(testSpan, throwable) + DECORATE.beforeFinish(testSpan) + scope.close() + testSpan.finish() + + then: + assertTraces(1) { + trace(1) { + span { + operationName "spring.handler" + spanType "web" + errored true + tags { + "$Tags.COMPONENT" "spring-web-controller" + "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER + errorTags(throwable.class, throwable.message) + defaultTags() + } + } + } + } + + where: + throwable << [ + new CustomServerErrorException(), + new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR), + new ErrorResponseException(HttpStatus.INTERNAL_SERVER_ERROR) + ] + } + + def "exception recognized via configured custom accessor with a non-5xx status is not flagged as an error"() { + setup: + injectSysConfig(TraceInstrumentationConfig.RESPONSE_STATUS_EXCEPTIONS, "${CustomStatusException.name}#httpCode") + def testSpan = AgentTracer.startSpan("spring-web-controller", "spring.handler") + def scope = AgentTracer.activateSpan(testSpan) + DECORATE.afterStart(testSpan) + def throwable = new CustomStatusException(404) + + when: + DECORATE.onError(testSpan, throwable) + DECORATE.beforeFinish(testSpan) + scope.close() + testSpan.finish() + + then: + assertTraces(1) { + trace(1) { + span { + operationName "spring.handler" + spanType "web" + errored false + tags { + "$Tags.COMPONENT" "spring-web-controller" + "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER + errorTags(CustomStatusException) + defaultTags() + } + } + } + } + } + + def "exception recognized via configured custom accessor with a 5xx status is flagged as an error"() { + setup: + injectSysConfig(TraceInstrumentationConfig.RESPONSE_STATUS_EXCEPTIONS, "${CustomStatusException.name}#httpCode") + def testSpan = AgentTracer.startSpan("spring-web-controller", "spring.handler") + def scope = AgentTracer.activateSpan(testSpan) + DECORATE.afterStart(testSpan) + def throwable = new CustomStatusException(500) + + when: + DECORATE.onError(testSpan, throwable) + DECORATE.beforeFinish(testSpan) + scope.close() + testSpan.finish() + + then: + assertTraces(1) { + trace(1) { + span { + operationName "spring.handler" + spanType "web" + errored true + tags { + "$Tags.COMPONENT" "spring-web-controller" + "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER + errorTags(CustomStatusException) + defaultTags() + } + } + } + } + } + + def "plain exception is still flagged as an error"() { + setup: + def testSpan = AgentTracer.startSpan("spring-web-controller", "spring.handler") + def scope = AgentTracer.activateSpan(testSpan) + DECORATE.afterStart(testSpan) + def throwable = new IllegalStateException("boom") + + when: + DECORATE.onError(testSpan, throwable) + DECORATE.beforeFinish(testSpan) + scope.close() + testSpan.finish() + + then: + assertTraces(1) { + trace(1) { + span { + operationName "spring.handler" + spanType "web" + errored true + tags { + "$Tags.COMPONENT" "spring-web-controller" + "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER + errorTags(IllegalStateException, "boom") + defaultTags() + } + } + } + } + } +} diff --git a/dd-trace-api/src/main/java/datadog/trace/api/config/TraceInstrumentationConfig.java b/dd-trace-api/src/main/java/datadog/trace/api/config/TraceInstrumentationConfig.java index 855a7243caf..b18ce173231 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/config/TraceInstrumentationConfig.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/config/TraceInstrumentationConfig.java @@ -198,6 +198,15 @@ public final class TraceInstrumentationConfig { "trace.jax-rs.exception-as-error.enabled"; public static final String JAX_RS_ADDITIONAL_ANNOTATIONS = "trace.jax-rs.additional.annotations"; + /** + * Comma-separated list of {@code fully.qualified.ExceptionClass#accessorMethod} entries. When a + * JAX-RS/Spring MVC handler throws an exception matching one of these entries (or a subclass of + * one), the named no-arg accessor method is invoked reflectively and its numeric return value is + * used as the HTTP status for deciding whether the span should be flagged as an error, instead of + * unconditionally marking it as an error. + */ + public static final String RESPONSE_STATUS_EXCEPTIONS = "trace.response-status.exceptions"; + /** If set, the instrumentation will set its resource name on the local root too. */ public static final String AXIS_PROMOTE_RESOURCE_NAME = "trace.axis.promote.resource-name"; diff --git a/internal-api/src/main/java/datadog/trace/api/Config.java b/internal-api/src/main/java/datadog/trace/api/Config.java index fade2b4c417..dcc68f1a99d 100644 --- a/internal-api/src/main/java/datadog/trace/api/Config.java +++ b/internal-api/src/main/java/datadog/trace/api/Config.java @@ -624,6 +624,7 @@ import static datadog.trace.api.config.TraceInstrumentationConfig.RABBIT_PROPAGATION_DISABLED_QUEUES; import static datadog.trace.api.config.TraceInstrumentationConfig.RESILIENCE4J_MEASURED_ENABLED; import static datadog.trace.api.config.TraceInstrumentationConfig.RESILIENCE4J_TAG_METRICS_ENABLED; +import static datadog.trace.api.config.TraceInstrumentationConfig.RESPONSE_STATUS_EXCEPTIONS; import static datadog.trace.api.config.TraceInstrumentationConfig.SERVLET_ASYNC_TIMEOUT_ERROR; import static datadog.trace.api.config.TraceInstrumentationConfig.SERVLET_PRINCIPAL_ENABLED; import static datadog.trace.api.config.TraceInstrumentationConfig.SERVLET_ROOT_CONTEXT_SERVICE_NAME; @@ -1305,6 +1306,8 @@ public static String getHostName() { private final Set jmsPropagationDisabledQueues; private final int jmsUnacknowledgedMaxAge; + private final Map responseStatusExceptionAccessors; + private final boolean rabbitPropagationEnabled; private final Set rabbitPropagationDisabledQueues; private final Set rabbitPropagationDisabledExchanges; @@ -3097,6 +3100,8 @@ PROFILING_DATADOG_PROFILER_ENABLED, isDatadogProfilerSafeInCurrentEnvironment()) tryMakeImmutableSet(configProvider.getList(JMS_PROPAGATION_DISABLED_QUEUES)); jmsUnacknowledgedMaxAge = configProvider.getInteger(JMS_UNACKNOWLEDGED_MAX_AGE, 3600); + responseStatusExceptionAccessors = configProvider.getMergedMap(RESPONSE_STATUS_EXCEPTIONS, '#'); + rabbitPropagationEnabled = isPropagationEnabled(true, "rabbit", "rabbitmq"); rabbitPropagationDisabledQueues = tryMakeImmutableSet(configProvider.getList(RABBIT_PROPAGATION_DISABLED_QUEUES)); @@ -3690,6 +3695,10 @@ public Map getResponseHeaderTags() { return responseHeaderTags; } + public Map getResponseStatusExceptionAccessors() { + return responseStatusExceptionAccessors; + } + public boolean isRequestHeaderTagsCommaAllowed() { return requestHeaderTagsCommaAllowed; } @@ -6809,6 +6818,8 @@ public String toString() { + jmsPropagationDisabledTopics + ", jmsPropagationDisabledQueues=" + jmsPropagationDisabledQueues + + ", responseStatusExceptionAccessors=" + + responseStatusExceptionAccessors + ", rabbitPropagationEnabled=" + rabbitPropagationEnabled + ", rabbitPropagationDisabledQueues=" diff --git a/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy b/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy index ef95d5e902c..9d649d55ce8 100644 --- a/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy @@ -101,6 +101,7 @@ import static datadog.trace.api.config.TraceInstrumentationConfig.DB_CLIENT_HOST import static datadog.trace.api.config.TraceInstrumentationConfig.DB_CLIENT_HOST_SPLIT_BY_INSTANCE import static datadog.trace.api.config.TraceInstrumentationConfig.DB_CLIENT_HOST_SPLIT_BY_INSTANCE_TYPE_SUFFIX import static datadog.trace.api.config.TraceInstrumentationConfig.HTTP_CLIENT_HOST_SPLIT_BY_DOMAIN +import static datadog.trace.api.config.TraceInstrumentationConfig.RESPONSE_STATUS_EXCEPTIONS import static datadog.trace.api.config.TraceInstrumentationConfig.RUNTIME_CONTEXT_FIELD_INJECTION import static datadog.trace.api.config.TraceInstrumentationConfig.TRACE_ENABLED import static datadog.trace.api.config.TracerConfig.AGENT_HOST @@ -1523,6 +1524,29 @@ class ConfigTest extends DDSpecification { // spotless:on } + def "verify response status exceptions config on tracer"() { + setup: + System.setProperty(PREFIX + RESPONSE_STATUS_EXCEPTIONS, propString) + def props = new Properties() + props.setProperty(RESPONSE_STATUS_EXCEPTIONS, propString) + + when: + def config = new Config() + def propConfig = Config.get(props) + + then: + config.responseStatusExceptionAccessors == expected + propConfig.responseStatusExceptionAccessors == expected + + where: + // spotless:off + propString | expected + "" | [:] + "some.pkg.MyException#httpCode" | ["some.pkg.MyException": "httpCode"] + "some.pkg.MyException#httpCode,other.pkg.OtherException#getStatusCode" | ["some.pkg.MyException": "httpCode", "other.pkg.OtherException": "getStatusCode"] + // spotless:on + } + def "verify integer range configs on tracer"() { setup: System.setProperty(PREFIX + HTTP_SERVER_ERROR_STATUSES, value) diff --git a/metadata/supported-configurations.json b/metadata/supported-configurations.json index a1459dc3453..487230022c3 100644 --- a/metadata/supported-configurations.json +++ b/metadata/supported-configurations.json @@ -9577,6 +9577,14 @@ "aliases": [] } ], + "DD_TRACE_RESPONSE_STATUS_EXCEPTIONS": [ + { + "version": "A", + "type": "map", + "default": null, + "aliases": [] + } + ], "DD_TRACE_RESTEASY_ENABLED": [ { "version": "A",