Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import datadog.trace.bootstrap.instrumentation.api.AgentPropagation;
import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
import datadog.trace.bootstrap.instrumentation.api.AgentTracer;
import datadog.trace.bootstrap.instrumentation.api.ErrorPriorities;
import datadog.trace.bootstrap.instrumentation.api.InternalSpanTypes;
import datadog.trace.bootstrap.instrumentation.api.PathwayContext;
import datadog.trace.bootstrap.instrumentation.api.ResourceNamePriorities;
Expand All @@ -34,6 +35,7 @@
import java.util.BitSet;
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.concurrent.ExecutionException;
import java.util.function.BiFunction;
import java.util.function.Function;
import java.util.function.Supplier;
Expand Down Expand Up @@ -292,7 +294,7 @@ public AgentSpan onResponseStatus(final AgentSpan span, final int status) {
// even if the server chooses not to respond with 5xx to an error.
// Anyway, we def don't want it applied to blocked requests
if (!BlockingException.class.getName().equals(span.getTag("error.type"))) {
span.setError(SERVER_ERROR_STATUSES.get(status));
span.setError(SERVER_ERROR_STATUSES.get(status), ErrorPriorities.HTTP_SERVER_DECORATOR);
}

if (SHOULD_SET_404_RESOURCE_NAME && status == 404) {
Expand Down Expand Up @@ -363,6 +365,16 @@ private AgentSpan.Context.Extracted callIGCallbackStart(AgentSpan.Context.Extrac
return context;
}

@Override
public AgentSpan onError(final AgentSpan span, final Throwable throwable) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't see any usages of this method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's used by Subclasses (e.g. datadog.trace.instrumentation.tomcat.TomcatDecorator#onResponse calls onError).
Being overridded now http server based decorators are inheriting this behavior

if (throwable != null) {
span.addThrowable(
throwable instanceof ExecutionException ? throwable.getCause() : throwable,
ErrorPriorities.HTTP_SERVER_DECORATOR);
}
return span;
}

private Flow<Void> callIGCallbackRequestHeaders(AgentSpan span, REQUEST_CARRIER carrier) {
CallbackProvider cbp = tracer().getCallbackProvider(RequestContextSlot.APPSEC);
RequestContext requestContext = span.getRequestContext();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ class BaseDecoratorTest extends DDSpecification {
@Shared
def decorator = newDecorator()

@Shared
def errorPriority = null as Byte

def span = Mock(AgentSpan)

def "test afterStart"() {
Expand Down Expand Up @@ -59,7 +62,11 @@ class BaseDecoratorTest extends DDSpecification {

then:
if (error) {
1 * span.addThrowable(error)
if (errorPriority != null) {
1 * span.addThrowable(error, errorPriority)
} else {
1 * span.addThrowable(error)
}
}
0 * _

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import datadog.trace.bootstrap.instrumentation.api.AgentSpan
import datadog.trace.bootstrap.instrumentation.api.AgentTracer
import datadog.trace.bootstrap.instrumentation.api.AgentTracer.TracerAPI
import datadog.trace.bootstrap.instrumentation.api.ContextVisitors
import datadog.trace.bootstrap.instrumentation.api.ErrorPriorities
import datadog.trace.bootstrap.instrumentation.api.ResourceNamePriorities
import datadog.trace.bootstrap.instrumentation.api.Tags
import datadog.trace.bootstrap.instrumentation.api.URIDataAdapter
Expand All @@ -35,6 +36,7 @@ class HttpServerDecoratorTest extends ServerDecoratorTest {
void setup() {
origAppSecActive = ActiveSubsystems.APPSEC_ACTIVE
ActiveSubsystems.APPSEC_ACTIVE = true
errorPriority = ErrorPriorities.HTTP_SERVER_DECORATOR
}

void cleanup() {
Expand Down Expand Up @@ -303,7 +305,7 @@ class HttpServerDecoratorTest extends ServerDecoratorTest {
1 * this.span.setHttpStatusCode(status)
}
if (resp) {
1 * this.span.setError(error)
1 * this.span.setError(error, ErrorPriorities.HTTP_SERVER_DECORATOR)
}
if (status == 404) {
1 * this.span.setResourceName({ it as String == "404" }, ResourceNamePriorities.HTTP_404)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import datadog.trace.agent.tooling.Instrumenter;
import datadog.trace.bootstrap.instrumentation.api.AgentScope;
import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
import datadog.trace.bootstrap.instrumentation.api.ErrorPriorities;
import java.util.List;
import net.bytebuddy.asm.Advice;
import org.springframework.context.ApplicationContext;
Expand Down Expand Up @@ -121,7 +122,7 @@ public static void nameResource(@Advice.Argument(3) final Exception exception) {
// We rely on a decorator to set the error state based on response code. (5xx -> error)
// Status code might not be set though if the span isn't the server span.
// Meaning the error won't be set by the status code. (Probably ok since not measured.)
span.setError(alreadyError);
span.setError(alreadyError, ErrorPriorities.HTTP_SERVER_DECORATOR);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeSpan;

import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
import datadog.trace.bootstrap.instrumentation.api.ErrorPriorities;
import net.bytebuddy.asm.Advice;

public class ErrorHandlerAdvice {
Expand All @@ -16,7 +17,7 @@ public static void nameResource(@Advice.Argument(3) final Exception exception) {
// We rely on a decorator to set the error state based on response code. (5xx -> error)
// Status code might not be set though if the span isn't the server span.
// Meaning the error won't be set by the status code. (Probably ok since not measured.)
span.setError(alreadyError);
span.setError(alreadyError, ErrorPriorities.HTTP_SERVER_DECORATOR);
}
}
}
16 changes: 13 additions & 3 deletions dd-trace-core/src/main/java/datadog/trace/core/DDSpan.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import datadog.trace.api.sampling.SamplingMechanism;
import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
import datadog.trace.bootstrap.instrumentation.api.AttachableWrapper;
import datadog.trace.bootstrap.instrumentation.api.ErrorPriorities;
import datadog.trace.bootstrap.instrumentation.api.PathwayContext;
import datadog.trace.bootstrap.instrumentation.api.ResourceNamePriorities;
import java.io.PrintWriter;
Expand Down Expand Up @@ -245,7 +246,12 @@ public final void publish() {

@Override
public DDSpan setError(final boolean error) {
context.setErrorFlag(error);
return setError(error, ErrorPriorities.DEFAULT);
}

@Override
public DDSpan setError(final boolean error, final byte priority) {
context.setErrorFlag(error, priority);
return this;
}

Expand Down Expand Up @@ -313,6 +319,11 @@ public DDSpan setErrorMessage(final String errorMessage) {

@Override
public DDSpan addThrowable(final Throwable error) {
return addThrowable(error, ErrorPriorities.DEFAULT);
}

@Override
public DDSpan addThrowable(Throwable error, byte errorPriority) {
if (null != error) {
String message = error.getMessage();
if (!"broken pipe".equalsIgnoreCase(message)
Expand All @@ -322,7 +333,7 @@ public DDSpan addThrowable(final Throwable error) {
// which might happen because the application is overloaded
// or warming up - capturing the stack trace and keeping
// the trace may exacerbate existing problems.
setError(true);
setError(true, errorPriority);
final StringWriter errorString = new StringWriter();
error.printStackTrace(new PrintWriter(errorString));
setTag(DDTags.ERROR_STACK, errorString.toString());
Expand All @@ -331,7 +342,6 @@ public DDSpan addThrowable(final Throwable error) {
setTag(DDTags.ERROR_MSG, message);
setTag(DDTags.ERROR_TYPE, error.getClass().getName());
}

return this;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import datadog.trace.api.sampling.PrioritySampling;
import datadog.trace.api.sampling.SamplingMechanism;
import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
import datadog.trace.bootstrap.instrumentation.api.ErrorPriorities;
import datadog.trace.bootstrap.instrumentation.api.PathwayContext;
import datadog.trace.bootstrap.instrumentation.api.ProfilerContext;
import datadog.trace.bootstrap.instrumentation.api.ProfilingContextIntegration;
Expand Down Expand Up @@ -113,6 +114,8 @@ public class DDSpanContext
/** True indicates that the span reports an error */
private volatile boolean errorFlag;

private volatile byte errorFlagPriority = ErrorPriorities.UNSET;

private volatile boolean measured;

private volatile boolean topLevel;
Expand Down Expand Up @@ -388,9 +391,10 @@ public boolean getErrorFlag() {
return errorFlag;
}

public void setErrorFlag(final boolean errorFlag) {
if (errorFlag != this.errorFlag) {
public void setErrorFlag(final boolean errorFlag, final byte priority) {
if (priority >= this.errorFlagPriority) {
this.errorFlag = errorFlag;
this.errorFlagPriority = priority;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import datadog.trace.api.env.CapturedEnvironment;
import datadog.trace.api.normalize.HttpResourceNames;
import datadog.trace.api.sampling.SamplingMechanism;
import datadog.trace.bootstrap.instrumentation.api.ErrorPriorities;
import datadog.trace.bootstrap.instrumentation.api.InstrumentationTags;
import datadog.trace.bootstrap.instrumentation.api.ResourceNamePriorities;
import datadog.trace.bootstrap.instrumentation.api.Tags;
Expand Down Expand Up @@ -186,7 +187,7 @@ private boolean interceptDbStatement(DDSpanContext span, Object value) {
}

private boolean interceptError(DDSpanContext span, Object value) {
span.setErrorFlag(asBoolean(value));
span.setErrorFlag(asBoolean(value), ErrorPriorities.DEFAULT);
return true;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package datadog.trace.core
import datadog.trace.api.DDTags
import datadog.trace.api.DDTraceId
import datadog.trace.bootstrap.instrumentation.api.AgentSpan
import datadog.trace.bootstrap.instrumentation.api.ErrorPriorities
import datadog.trace.bootstrap.instrumentation.api.ProfilingContextIntegration
import datadog.trace.common.writer.ListWriter
import datadog.trace.core.propagation.ExtractedContext
Expand Down Expand Up @@ -43,7 +44,7 @@ class DDSpanContextTest extends DDCoreSpecification {
when:
context.setTag("some.tag", "asdf")
context.setTag(name, null)
context.setErrorFlag(true)
context.setErrorFlag(true, ErrorPriorities.DEFAULT)
span.finish()

writer.waitForTraces(1)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import datadog.trace.api.gateway.RequestContextSlot
import datadog.trace.api.sampling.PrioritySampling
import datadog.trace.bootstrap.instrumentation.api.AgentSpan
import datadog.trace.bootstrap.instrumentation.api.AgentTracer.NoopPathwayContext
import datadog.trace.bootstrap.instrumentation.api.ErrorPriorities
import datadog.trace.bootstrap.instrumentation.api.TagContext
import datadog.trace.bootstrap.instrumentation.api.UTF8BytesString
import datadog.trace.common.sampling.RateByServiceTraceSampler
Expand Down Expand Up @@ -431,4 +432,32 @@ class DDSpanTest extends DDCoreSpecification {
0.5 | 100
0.25 | Integer.MAX_VALUE
}

def "error priorities should be respected"() {
setup:
def span = tracer.buildSpan("testSpan").start() as DDSpan

expect:
!span.isError()

when:
span.setError(true)
then:
span.isError()

when:
span.setError(false)
then:
!span.isError()

when:
span.setError(true, ErrorPriorities.HTTP_SERVER_DECORATOR)
then:
!span.isError()

when:
span.setError(true, Byte.MAX_VALUE)
then:
span.isError()
}
}
1 change: 1 addition & 0 deletions internal-api/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ excludedClassesCoverage += [
"datadog.trace.bootstrap.instrumentation.api.TagContext.HttpHeaders",
"datadog.trace.bootstrap.instrumentation.api.ForwardedTagContext",
"datadog.trace.bootstrap.instrumentation.api.ResourceNamePriorities",
"datadog.trace.bootstrap.instrumentation.api.ErrorPriorities",
"datadog.trace.api.civisibility.ci.CIInfo",
"datadog.trace.api.civisibility.ci.CIInfo.Builder",
"datadog.trace.api.civisibility.InstrumentationBridge",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,12 +62,16 @@ public interface AgentSpan extends MutableSpan, IGSpanInfo {
@Override
AgentSpan setError(boolean error);

AgentSpan setError(boolean error, byte priority);

AgentSpan setMeasured(boolean measured);

AgentSpan setErrorMessage(String errorMessage);

AgentSpan addThrowable(Throwable throwable);

AgentSpan addThrowable(Throwable throwable, byte errorPriority);

@Override
AgentSpan getLocalRootSpan();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -669,6 +669,11 @@ public AgentSpan setError(final boolean error) {
return this;
}

@Override
public AgentSpan setError(boolean error, byte priority) {
return this;
}

@Override
public AgentSpan setMeasured(boolean measured) {
return this;
Expand All @@ -689,6 +694,11 @@ public AgentSpan addThrowable(final Throwable throwable) {
return this;
}

@Override
public AgentSpan addThrowable(Throwable throwable, byte errorPriority) {
return this;
}

@Override
public AgentSpan setHttpStatusCode(int statusCode) {
return this;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package datadog.trace.bootstrap.instrumentation.api;

public class ErrorPriorities {
public static final byte UNSET = Byte.MIN_VALUE;
public static final byte HTTP_SERVER_DECORATOR = -1;

public static final byte DEFAULT = 0;
}