Remove error flag decorator (#37)

This commit is contained in:
Trask Stalnaker 2019-12-11 16:56:21 -08:00 committed by GitHub
parent 38a916f5e9
commit ab3aa3ba3a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
43 changed files with 26 additions and 142 deletions

View File

@ -18,7 +18,6 @@ public class Tags {
public static final String PEER_PORT = "peer.port";
public static final String SPAN_KIND = "span.kind";
public static final String COMPONENT = "component";
public static final String ERROR = "error";
public static final String DB_TYPE = "db.type";
public static final String DB_INSTANCE = "db.instance";
public static final String DB_USER = "db.user";

View File

@ -165,7 +165,11 @@ public final class OpenTracing32 implements TracerAPI {
@Override
public AgentSpan setError(final boolean error) {
Tags.ERROR.set(span, error);
if (span instanceof DDSpan) {
((DDSpan) span).setError(error);
} else {
Tags.ERROR.set(span, error);
}
return this;
}

View File

@ -107,8 +107,8 @@ class HttpClientDecoratorTest extends ClientDecoratorTest {
399 | false | null | [status: 399]
400 | true | null | [status: 400]
499 | true | null | [status: 499]
500 | false | null | [status: 500]
500 | true | "500" | [status: 500]
500 | true | null | [status: 500]
500 | false | "400-499" | [status: 500]
500 | true | "400-500" | [status: 500]
600 | false | null | [status: 600]
null | false | null | [status: null]

View File

@ -110,7 +110,6 @@ class LagomTest extends AgentTestRunner {
"$Tags.HTTP_URL" "ws://localhost:${server.port()}/error"
"$Tags.HTTP_METHOD" "GET"
"$Tags.HTTP_STATUS" 500
"$Tags.ERROR" true
defaultTags()
}
}

View File

@ -123,6 +123,7 @@ public final class AkkaHttpServerInstrumentation extends Instrumenter.Default {
public static void finishSpan(final AgentSpan span, final Throwable t) {
DECORATE.onError(span, t);
span.setTag(Tags.HTTP_STATUS, 500);
span.setError(true);
DECORATE.beforeFinish(span);
final TraceScope scope = activeScope();

View File

@ -73,7 +73,6 @@ class AkkaHttpClientInstrumentationTest extends HttpClientTest<AkkaHttpClientDec
tags {
"$Tags.COMPONENT" "akka-http-client"
"$Tags.SPAN_KIND" Tags.SPAN_KIND_CLIENT
"$Tags.ERROR" true
errorTags(NullPointerException)
defaultTags()
}

View File

@ -53,7 +53,6 @@ abstract class AkkaHttpServerInstrumentationTest extends HttpServerTest<Object,
"$Tags.HTTP_METHOD" method
"$Tags.HTTP_STATUS" endpoint.status
if (endpoint.errored) {
"$Tags.ERROR" endpoint.errored
"error.msg" { it == null || it == EXCEPTION.body }
"error.type" { it == null || it == Exception.name }
"error.stack" { it == null || it instanceof String }

View File

@ -105,7 +105,6 @@ class GlassFishServerTest extends HttpServerTest<GlassFish, Servlet3Decorator> {
"servlet.path" endpoint.path
"span.origin.type" { it.startsWith("TestServlets\$") || it == DefaultServlet.name }
if (endpoint.errored) {
"$Tags.ERROR" endpoint.errored
"error.msg" { it == null || it == EXCEPTION.body }
"error.type" { it == null || it == Exception.name }
"error.stack" { it == null || it instanceof String }

View File

@ -67,7 +67,6 @@ abstract class AbstractGoogleHttpClientTest extends HttpClientTest<GoogleHttpCli
"$Tags.HTTP_URL" "${uri.resolve(uri.path)}"
"$Tags.HTTP_METHOD" method
"$Tags.HTTP_STATUS" 500
"$Tags.ERROR" true
defaultTags()
}
}

View File

@ -139,8 +139,6 @@ class GrpcTest extends AgentTestRunner {
"status.description" description
if (status.cause != null) {
errorTags status.cause.class, status.cause.message
} else {
tag "error", true
}
defaultTags(true)
}
@ -170,7 +168,6 @@ class GrpcTest extends AgentTestRunner {
"$Tags.SPAN_KIND" Tags.SPAN_KIND_CLIENT
"status.code" "${status.code.name()}"
"status.description" description
tag "error", true
defaultTags()
}
}
@ -251,7 +248,6 @@ class GrpcTest extends AgentTestRunner {
"$Tags.COMPONENT" "grpc-client"
"$Tags.SPAN_KIND" Tags.SPAN_KIND_CLIENT
"status.code" "UNKNOWN"
tag "error", true
defaultTags()
}
}

View File

@ -118,7 +118,6 @@ class DropwizardTest extends HttpServerTest<DropwizardTestSupport, Servlet3Decor
"$Tags.HTTP_STATUS" endpoint.status
"span.origin.type" ServletHandler.CachedChain.name
if (endpoint.errored) {
"$Tags.ERROR" endpoint.errored
"error.msg" { it == null || it == EXCEPTION.body }
"error.type" { it == null || it == Exception.name }
"error.stack" { it == null || it instanceof String }

View File

@ -62,6 +62,7 @@ public class JettyHandlerAdvice {
if (resp.getStatus() == HttpServletResponse.SC_OK) {
// exception is thrown in filter chain, but status code is incorrect
span.setTag(Tags.HTTP_STATUS, 500);
span.setError(true);
}
DECORATE.onError(span, throwable);
DECORATE.beforeFinish(span);

View File

@ -46,6 +46,7 @@ public class TagSettingAsyncListener implements AsyncListener {
== HttpServletResponse.SC_OK) {
// exception is thrown in filter chain, but status code is incorrect
span.setTag(Tags.HTTP_STATUS, 500);
span.setError(true);
}
DECORATE.onError(span, event.getThrowable());
DECORATE.beforeFinish(span);

View File

@ -134,7 +134,6 @@ class JettyHandlerTest extends HttpServerTest<Server, JettyDecorator> {
"$Tags.HTTP_STATUS" endpoint.status
"span.origin.type" handlerName
if (endpoint.errored) {
"$Tags.ERROR" endpoint.errored
"error.msg" { it == null || it == EXCEPTION.body }
"error.type" { it == null || it == Exception.name }
"error.stack" { it == null || it instanceof String }

View File

@ -311,7 +311,6 @@ class JSPInstrumentationBasicTests extends AgentTestRunner {
"span.origin.type" "org.apache.catalina.core.ApplicationFilterChain"
"servlet.context" "/$jspWebappContext"
"servlet.path" "/$jspFileName"
"error" true
"error.type" { String tagExceptionType ->
return tagExceptionType == exceptionClass.getName() || tagExceptionType.contains(exceptionClass.getSimpleName())
}
@ -332,7 +331,6 @@ class JSPInstrumentationBasicTests extends AgentTestRunner {
"span.origin.type" jspClassName
"servlet.context" "/$jspWebappContext"
"jsp.requestURL" reqUrl
"error" true
"error.type" { String tagExceptionType ->
return tagExceptionType == exceptionClass.getName() || tagExceptionType.contains(exceptionClass.getSimpleName())
}

View File

@ -27,6 +27,7 @@ public class HttpServerResponseTracingHandler extends ChannelOutboundHandlerAdap
} catch (final Throwable throwable) {
DECORATE.onError(span, throwable);
span.setTag(Tags.HTTP_STATUS, 500);
span.setError(true);
span.finish(); // Finish the span manually since finishSpanOnClose was false
throw throwable;
}

View File

@ -56,6 +56,7 @@ public class PlayAdvice {
} else {
DECORATE.onError(playControllerSpan, throwable);
playControllerSpan.setTag(Tags.HTTP_STATUS, 500);
playControllerSpan.setError(true);
DECORATE.beforeFinish(playControllerSpan);
playControllerSpan.finish();
}

View File

@ -75,6 +75,7 @@ public class PlayHttpServerDecorator extends HttpServerDecorator<Request, Reques
@Override
public AgentSpan onError(final AgentSpan span, Throwable throwable) {
span.setTag(Tags.HTTP_STATUS, 500);
span.setError(true);
if (throwable != null
// This can be moved to instanceof check when using Java 8.
&& throwable.getClass().getName().equals("java.util.concurrent.CompletionException")

View File

@ -92,9 +92,7 @@ class PlayServerTest extends HttpServerTest<Server, NettyHttpServerDecorator> {
"$Tags.HTTP_URL" String
"$Tags.HTTP_METHOD" String
"$Tags.HTTP_STATUS" Integer
if (endpoint == ERROR) {
"$Tags.ERROR" true
} else if (endpoint == EXCEPTION) {
if (endpoint == EXCEPTION) {
errorTags(Exception, EXCEPTION.body)
}
defaultTags()

View File

@ -56,6 +56,7 @@ public class PlayAdvice {
} else {
DECORATE.onError(playControllerSpan, throwable);
playControllerSpan.setTag(Tags.HTTP_STATUS, 500);
playControllerSpan.setError(true);
DECORATE.beforeFinish(playControllerSpan);
playControllerSpan.finish();
}

View File

@ -78,6 +78,7 @@ public class PlayHttpServerDecorator extends HttpServerDecorator<Request, Reques
@Override
public AgentSpan onError(final AgentSpan span, Throwable throwable) {
span.setTag(Tags.HTTP_STATUS, 500);
span.setError(true);
if (throwable != null
// This can be moved to instanceof check when using Java 8.
&& throwable.getClass().getName().equals("java.util.concurrent.CompletionException")

View File

@ -94,9 +94,7 @@ class PlayServerTest extends HttpServerTest<Server, AkkaHttpServerDecorator> {
"$Tags.HTTP_URL" String
"$Tags.HTTP_METHOD" String
"$Tags.HTTP_STATUS" Integer
if (endpoint == ERROR) {
"$Tags.ERROR" true
} else if (endpoint == EXCEPTION) {
if (endpoint == EXCEPTION) {
errorTags(Exception, EXCEPTION.body)
}
defaultTags()
@ -122,7 +120,6 @@ class PlayServerTest extends HttpServerTest<Server, AkkaHttpServerDecorator> {
"$Tags.HTTP_URL" "${endpoint.resolve(address)}"
"$Tags.HTTP_METHOD" method
if (endpoint.errored) {
"$Tags.ERROR" endpoint.errored
"error.msg" { it == null || it == EXCEPTION.body }
"error.type" { it == null || it == Exception.name }
"error.stack" { it == null || it instanceof String }

View File

@ -115,9 +115,7 @@ class RatpackHttpServerTest extends HttpServerTest<EmbeddedApp, NettyHttpServerD
"$Tags.HTTP_URL" String
"$Tags.HTTP_METHOD" String
"$Tags.HTTP_STATUS" Integer
if (endpoint == ERROR) {
"$Tags.ERROR" true
} else if (endpoint == EXCEPTION) {
if (endpoint == EXCEPTION) {
errorTags(Exception, EXCEPTION.body)
}
defaultTags()
@ -146,9 +144,6 @@ class RatpackHttpServerTest extends HttpServerTest<EmbeddedApp, NettyHttpServerD
"$Tags.HTTP_URL" "${endpoint.resolve(address)}"
"$Tags.HTTP_METHOD" method
"$Tags.HTTP_STATUS" endpoint.status
if (endpoint.errored) {
"$Tags.ERROR" endpoint.errored
}
defaultTags(true)
}
}

View File

@ -85,6 +85,7 @@ public class Servlet2Advice {
== HttpServletResponse.SC_OK) {
// exception was thrown but status code wasn't set
span.setTag(Tags.HTTP_STATUS, 500);
span.setError(true);
}
DECORATE.onError(span, throwable);
}

View File

@ -98,7 +98,6 @@ class JettyServlet2Test extends HttpServerTest<Server, Servlet2Decorator> {
"servlet.path" endpoint.path
"span.origin.type" TestServlet2.Sync.name
if (endpoint.errored) {
"$Tags.ERROR" endpoint.errored
"error.msg" { it == null || it == EXCEPTION.body }
"error.type" { it == null || it == Exception.name }
"error.stack" { it == null || it instanceof String }

View File

@ -82,6 +82,7 @@ public class Servlet3Advice {
if (resp.getStatus() == HttpServletResponse.SC_OK) {
// exception is thrown in filter chain, but status code is incorrect
span.setTag(Tags.HTTP_STATUS, 500);
span.setError(true);
}
DECORATE.onError(span, throwable);
DECORATE.beforeFinish(span);

View File

@ -46,6 +46,7 @@ public class TagSettingAsyncListener implements AsyncListener {
== HttpServletResponse.SC_OK) {
// exception is thrown in filter chain, but status code is incorrect
span.setTag(Tags.HTTP_STATUS, 500);
span.setError(true);
}
DECORATE.onError(span, event.getThrowable());
DECORATE.beforeFinish(span);

View File

@ -87,7 +87,6 @@ abstract class AbstractServlet3Test<SERVER, CONTEXT> extends HttpServerTest<SERV
"servlet.path" { it == endpoint.path || it == "/dispatch$endpoint.path" }
"span.origin.type" { it == servlet.name || it == ApplicationFilterChain.name }
if (endpoint.errored) {
"$Tags.ERROR" endpoint.errored
"error.msg" { it == null || it == EXCEPTION.body }
"error.type" { it == null || it == Exception.name }
"error.stack" { it == null || it instanceof String }

View File

@ -257,7 +257,6 @@ abstract class JettyDispatchTest extends JettyServlet3Test {
it == TestServlet3.DispatchImmediate.name || it == TestServlet3.DispatchAsync.name || it == ApplicationFilterChain.name
}
if (endpoint.errored) {
"$Tags.ERROR" endpoint.errored
"error.msg" { it == null || it == EXCEPTION.body }
"error.type" { it == null || it == Exception.name }
"error.stack" { it == null || it instanceof String }

View File

@ -308,7 +308,6 @@ abstract class TomcatDispatchTest extends TomcatServlet3Test {
it == TestServlet3.DispatchImmediate.name || it == TestServlet3.DispatchAsync.name || it == ApplicationFilterChain.name
}
if (endpoint.errored) {
"$Tags.ERROR" endpoint.errored
"error.msg" { it == null || it == EXCEPTION.body }
"error.type" { it == null || it == Exception.name }
"error.stack" { it == null || it instanceof String }

View File

@ -326,7 +326,6 @@ class SpringWebfluxTest extends AgentTestRunner {
"$Tags.HTTP_URL" url
"$Tags.HTTP_METHOD" "GET"
"$Tags.HTTP_STATUS" 500
"error" true
defaultTags()
}
}

View File

@ -134,7 +134,6 @@ class SpringBootBasedTest extends HttpServerTest<ConfigurableApplicationContext,
"span.origin.type" ApplicationFilterChain.name
"servlet.path" endpoint.path
if (endpoint.errored) {
"$Tags.ERROR" endpoint.errored
"error.msg" { it == null || it == EXCEPTION.body }
"error.type" { it == null || it == Exception.name }
"error.stack" { it == null || it instanceof String }

View File

@ -1,4 +1,3 @@
import datadog.opentracing.decorators.ErrorFlag
import datadog.trace.agent.test.AgentTestRunner
import datadog.trace.agent.test.utils.ConfigUtils
import datadog.trace.api.DDTags
@ -251,8 +250,6 @@ class TraceAnnotationsTest extends AgentTestRunner {
def "test exception exit"() {
setup:
TEST_TRACER.addDecorator(new ErrorFlag())
Throwable error = null
try {
SayTracedHello.sayERROR()
@ -280,8 +277,6 @@ class TraceAnnotationsTest extends AgentTestRunner {
def "test exception exit with resource name"() {
setup:
TEST_TRACER.addDecorator(new ErrorFlag())
Throwable error = null
try {
SayTracedHello.sayERRORWithResource()

View File

@ -422,7 +422,6 @@ class TwilioClientTest extends AgentTestRunner {
"$Tags.HTTP_URL" String
"$Tags.HTTP_METHOD" String
"$Tags.HTTP_STATUS" Integer
"$Tags.ERROR" true
defaultTags()
}
}
@ -570,7 +569,6 @@ class TwilioClientTest extends AgentTestRunner {
"$Tags.HTTP_URL" String
"$Tags.HTTP_METHOD" String
"$Tags.HTTP_STATUS" Integer
"$Tags.ERROR" true
defaultTags()
}
}

View File

@ -43,7 +43,6 @@ class TagsAssert {
}
def errorTags(Class<Throwable> errorType, message) {
tag("error", true)
tag("error.type", errorType.name)
tag("error.stack", String)

View File

@ -443,9 +443,6 @@ abstract class HttpServerTest<SERVER, DECORATOR extends HttpServerDecorator> ext
// "$DDTags.HTTP_QUERY" uri.query
// "$DDTags.HTTP_FRAGMENT" { it == null || it == uri.fragment } // Optional
// }
if (endpoint.errored) {
"$Tags.ERROR" endpoint.errored
}
defaultTags(true)
}
}

View File

@ -69,7 +69,7 @@ public class Config {
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");
parseIntegerRangeSet("400-599", "default");
private static final boolean DEFAULT_HTTP_SERVER_TAG_QUERY_STRING = false;
private static final boolean DEFAULT_HTTP_CLIENT_TAG_QUERY_STRING = false;
private static final boolean DEFAULT_HTTP_CLIENT_SPLIT_BY_DOMAIN = false;

View File

@ -35,7 +35,7 @@ class ConfigTest extends DDSpecification {
config.writerType == "LoggingWriter"
config.traceResolverEnabled == true
config.httpServerErrorStatuses == (500..599).toSet()
config.httpClientErrorStatuses == (400..499).toSet()
config.httpClientErrorStatuses == (400..599).toSet()
config.httpClientSplitByDomain == false
config.dbClientSplitByInstance == false
config.partialFlushMinSpans == 1000
@ -149,7 +149,7 @@ class ConfigTest extends DDSpecification {
config.writerType == " "
config.traceResolverEnabled == true
config.httpServerErrorStatuses == (500..599).toSet()
config.httpClientErrorStatuses == (400..499).toSet()
config.httpClientErrorStatuses == (400..599).toSet()
config.httpClientSplitByDomain == false
config.dbClientSplitByInstance == false
}

View File

@ -7,7 +7,6 @@ import java.util.List;
public class DDDecoratorsFactory {
public static List<AbstractDecorator> createBuiltinDecorators() {
return Arrays.asList(
new DBTypeDecorator(), new ErrorFlag(), new SpanTypeDecorator(), new Status5XXDecorator());
return Arrays.asList(new DBTypeDecorator(), new SpanTypeDecorator());
}
}

View File

@ -1,23 +0,0 @@
package datadog.opentracing.decorators;
import datadog.opentracing.DDSpanContext;
import io.opentracing.tag.Tags;
public class ErrorFlag extends AbstractDecorator {
public ErrorFlag() {
super();
this.setMatchingTag(Tags.ERROR.getKey());
}
@Override
public boolean shouldSetTag(final DDSpanContext context, final String tag, final Object value) {
// Assign resource name
try {
context.setErrorFlag(Boolean.parseBoolean(String.valueOf(value)));
} catch (final Throwable t) {
// DO NOTHING
}
// TODO: Do we really want an error tag if the error flag is already set?
return true;
}
}

View File

@ -1,21 +0,0 @@
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 shouldSetTag(final DDSpanContext context, final String tag, final Object value) {
final int responseCode = Integer.parseInt(value.toString());
if (500 <= responseCode && responseCode < 600) {
context.setTag(Tags.ERROR.getKey(), true);
}
return true;
}
}

View File

@ -65,37 +65,6 @@ class SpanDecoratorTest extends DDSpecification {
type = "foo"
}
def "set 5XX status code as an error"() {
when:
Tags.HTTP_STATUS.set(span, status)
then:
span.isError() == error
where:
status | error
400 | false
404 | false
499 | false
500 | true
550 | true
599 | true
600 | false
}
def "set error flag when error tag reported"() {
when:
Tags.ERROR.set(span, error)
then:
span.isError() == error
where:
error | _
true | _
false | _
}
def "#attribute decorators apply to builder too"() {
setup:
def span = tracer.buildSpan("decorator.test").withTag(name, value).start()
@ -107,18 +76,4 @@ class SpanDecoratorTest extends DDSpecification {
attribute | name | value
"spanType" | DDTags.SPAN_TYPE | "my-span-type"
}
def "decorators apply to builder too"() {
when:
span = tracer.buildSpan("decorator.test").withTag("error", "true").start()
then:
span.error
when:
span = tracer.buildSpan("decorator.test").withTag(Tags.HTTP_STATUS.key, 500).start()
then:
span.error
}
}

View File

@ -35,7 +35,7 @@ class DDTracerTest extends DDSpecification {
def tracer = new DDTracer()
then:
tracer.spanContextDecorators.size() == 4
tracer.spanContextDecorators.size() == 2
tracer.injector instanceof DatadogHttpCodec.Injector
tracer.extractor instanceof DatadogHttpCodec.Extractor