From a98c22ac3a1c82816bb596db94641bca703c0d01 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Wed, 20 Feb 2019 08:46:47 -0800 Subject: [PATCH] Introduce base decorators --- .../agent-tooling/agent-tooling.gradle | 1 + .../trace/agent/decorator/BaseDecorator.java | 64 ++++++++ .../agent/decorator/ClientDecorator.java | 20 +++ .../agent/decorator/HttpClientDecorator.java | 54 ++++++ .../agent/decorator/HttpServerDecorator.java | 67 ++++++++ .../agent/decorator/ServerDecorator.java | 14 ++ .../agent/decorator/BaseDecoratorTest.groovy | 154 ++++++++++++++++++ .../decorator/ClientDecoratorTest.groovy | 72 ++++++++ .../decorator/HttpClientDecoratorTest.groovy | 136 ++++++++++++++++ .../decorator/HttpServerDecoratorTest.groovy | 114 +++++++++++++ .../decorator/ServerDecoratorTest.groovy | 57 +++++++ .../ConfiguredTraceAnnotationsTest.groovy | 9 +- .../main/java/datadog/trace/api/Config.java | 24 ++- .../datadog/trace/api/ConfigTest.groovy | 58 +++++++ 14 files changed, 832 insertions(+), 12 deletions(-) create mode 100644 dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/BaseDecorator.java create mode 100644 dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/ClientDecorator.java create mode 100644 dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpClientDecorator.java create mode 100644 dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpServerDecorator.java create mode 100644 dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/ServerDecorator.java create mode 100644 dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/BaseDecoratorTest.groovy create mode 100644 dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/ClientDecoratorTest.groovy create mode 100644 dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/HttpClientDecoratorTest.groovy create mode 100644 dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/HttpServerDecoratorTest.groovy create mode 100644 dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/ServerDecoratorTest.groovy diff --git a/dd-java-agent/agent-tooling/agent-tooling.gradle b/dd-java-agent/agent-tooling/agent-tooling.gradle index 609bbee3c7..701ab60c23 100644 --- a/dd-java-agent/agent-tooling/agent-tooling.gradle +++ b/dd-java-agent/agent-tooling/agent-tooling.gradle @@ -1,5 +1,6 @@ apply from: "${rootDir}/gradle/java.gradle" +minimumBranchCoverage = 0.6 excludedClassesConverage += ['datadog.trace.agent.tooling.*'] configurations { diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/BaseDecorator.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/BaseDecorator.java new file mode 100644 index 0000000000..6a265a88ac --- /dev/null +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/BaseDecorator.java @@ -0,0 +1,64 @@ +package datadog.trace.agent.decorator; + +import static io.opentracing.log.Fields.ERROR_OBJECT; + +import datadog.trace.api.Config; +import datadog.trace.api.DDTags; +import io.opentracing.Span; +import io.opentracing.tag.Tags; +import java.util.Arrays; +import java.util.Collections; +import java.util.TreeSet; + +public abstract class BaseDecorator { + + protected final boolean traceAnalyticsEnabled; + protected final float traceAnalyticsSampleRate; + + protected BaseDecorator() { + traceAnalyticsEnabled = + Config.traceAnalyticsIntegrationEnabled( + new TreeSet<>(Arrays.asList(instrumentationNames())), traceAnalyticsDefault()); + float rate = 1.0f; + for (final String name : instrumentationNames()) { + rate = + Config.getFloatSettingFromEnvironment( + "integration." + name + ".analytics.sample-rate", rate); + } + traceAnalyticsSampleRate = rate; + } + + protected abstract String[] instrumentationNames(); + + protected abstract String spanType(); + + protected abstract String component(); + + protected boolean traceAnalyticsDefault() { + return false; + } + + public Span afterStart(final Span span) { + assert span != null; + span.setTag(DDTags.SPAN_TYPE, spanType()); + Tags.COMPONENT.set(span, component()); + if (traceAnalyticsEnabled) { + span.setTag(DDTags.ANALYTICS_SAMPLE_RATE, traceAnalyticsSampleRate); + } + return span; + } + + public Span beforeFinish(final Span span) { + assert span != null; + return span; + } + + public Span onError(final Span span, final Throwable throwable) { + assert span != null; + if (throwable != null) { + Tags.ERROR.set(span, true); + span.log(Collections.singletonMap(ERROR_OBJECT, throwable)); + } + return span; + } +} diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/ClientDecorator.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/ClientDecorator.java new file mode 100644 index 0000000000..70d687207d --- /dev/null +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/ClientDecorator.java @@ -0,0 +1,20 @@ +package datadog.trace.agent.decorator; + +import datadog.trace.api.DDTags; +import io.opentracing.Span; +import io.opentracing.tag.Tags; + +public abstract class ClientDecorator extends BaseDecorator { + + protected abstract String service(); + + @Override + public Span afterStart(final Span span) { + assert span != null; + if (service() != null) { + span.setTag(DDTags.SERVICE_NAME, service()); + } + Tags.SPAN_KIND.set(span, Tags.SPAN_KIND_CLIENT); + return super.afterStart(span); + } +} diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpClientDecorator.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpClientDecorator.java new file mode 100644 index 0000000000..9ce20149f8 --- /dev/null +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpClientDecorator.java @@ -0,0 +1,54 @@ +package datadog.trace.agent.decorator; + +import datadog.trace.api.Config; +import datadog.trace.api.DDSpanTypes; +import datadog.trace.api.DDTags; +import io.opentracing.Span; +import io.opentracing.tag.Tags; + +public abstract class HttpClientDecorator extends ClientDecorator { + + protected abstract String method(REQUEST request); + + protected abstract String url(REQUEST request); + + protected abstract String hostname(REQUEST request); + + protected abstract Integer port(REQUEST request); + + protected abstract Integer status(RESPONSE response); + + @Override + protected String spanType() { + return DDSpanTypes.HTTP_CLIENT; + } + + public Span onRequest(final Span span, final REQUEST request) { + assert span != null; + if (request != null) { + Tags.HTTP_METHOD.set(span, method(request)); + Tags.HTTP_URL.set(span, url(request)); + Tags.PEER_HOSTNAME.set(span, hostname(request)); + Tags.PEER_PORT.set(span, port(request)); + + if (Config.get().isHttpClientSplitByDomain()) { + span.setTag(DDTags.SERVICE_NAME, hostname(request)); + } + } + return span; + } + + public Span onResponse(final Span span, final RESPONSE response) { + assert span != null; + if (response != null) { + final Integer status = status(response); + if (status != null) { + Tags.HTTP_STATUS.set(span, status); + if (400 <= status && status < 500) { + Tags.ERROR.set(span, true); + } + } + } + return span; + } +} diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpServerDecorator.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpServerDecorator.java new file mode 100644 index 0000000000..d29a9f423c --- /dev/null +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpServerDecorator.java @@ -0,0 +1,67 @@ +package datadog.trace.agent.decorator; + +import datadog.trace.api.Config; +import datadog.trace.api.DDSpanTypes; +import io.opentracing.Span; +import io.opentracing.tag.Tags; + +public abstract class HttpServerDecorator extends ServerDecorator { + + protected abstract String method(REQUEST request); + + protected abstract String url(REQUEST request); + + protected abstract String hostname(REQUEST request); + + protected abstract Integer port(REQUEST request); + + protected abstract Integer status(RESPONSE response); + + @Override + protected String spanType() { + return DDSpanTypes.HTTP_SERVER; + } + + @Override + protected boolean traceAnalyticsDefault() { + return Config.getBooleanSettingFromEnvironment(Config.TRACE_ANALYTICS_ENABLED, false); + } + + public Span onRequest(final Span span, final REQUEST request) { + assert span != null; + if (request != null) { + Tags.HTTP_METHOD.set(span, method(request)); + Tags.HTTP_URL.set(span, url(request)); + Tags.PEER_HOSTNAME.set(span, hostname(request)); + Tags.PEER_PORT.set(span, port(request)); + // TODO set resource name from URL. + } + return span; + } + + public Span onResponse(final Span span, final RESPONSE response) { + assert span != null; + if (response != null) { + final Integer status = status(response); + if (status != null) { + Tags.HTTP_STATUS.set(span, status); + if (status >= 500) { + Tags.ERROR.set(span, true); + } + } + } + return span; + } + + // @Override + // public Span onError(final Span span, final Throwable throwable) { + // assert span != null; + // // FIXME + // final Object status = span.getTag("http.status"); + // if (status == null || status.equals(200)) { + // // Ensure status set correctly + // span.setTag("http.status", 500); + // } + // return super.onError(span, throwable); + // } +} diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/ServerDecorator.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/ServerDecorator.java new file mode 100644 index 0000000000..653382e6e3 --- /dev/null +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/ServerDecorator.java @@ -0,0 +1,14 @@ +package datadog.trace.agent.decorator; + +import io.opentracing.Span; +import io.opentracing.tag.Tags; + +public abstract class ServerDecorator extends BaseDecorator { + + @Override + public Span afterStart(final Span span) { + assert span != null; + Tags.SPAN_KIND.set(span, Tags.SPAN_KIND_SERVER); + return super.afterStart(span); + } +} diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/BaseDecoratorTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/BaseDecoratorTest.groovy new file mode 100644 index 0000000000..7d87742b44 --- /dev/null +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/BaseDecoratorTest.groovy @@ -0,0 +1,154 @@ +package datadog.trace.agent.decorator + +import datadog.trace.api.DDTags +import io.opentracing.Span +import io.opentracing.tag.Tags +import spock.lang.Shared +import spock.lang.Specification + +import static datadog.trace.agent.test.utils.TraceUtils.withSystemProperty +import static io.opentracing.log.Fields.ERROR_OBJECT + +class BaseDecoratorTest extends Specification { + + @Shared + def decorator = newDecorator() + + def span = Mock(Span) + + def "test afterStart"() { + when: + decorator.afterStart(span) + + then: + 1 * span.setTag(DDTags.SPAN_TYPE, decorator.spanType()) + 1 * span.setTag(Tags.COMPONENT.key, "test-component") + _ * span.setTag(_, _) // Want to allow other calls from child implementations. + 0 * _ + } + + def "test onError"() { + when: + decorator.onError(span, error) + + then: + if (error) { + 1 * span.setTag(Tags.ERROR.key, true) + 1 * span.log([(ERROR_OBJECT): error]) + } + 0 * _ + + where: + error << [new Exception(), null] + } + + def "test beforeFinish"() { + when: + decorator.beforeFinish(span) + + then: + 0 * _ + } + + def "test assert null span"() { + when: + decorator.afterStart(null) + + then: + thrown(AssertionError) + + when: + decorator.onError(null, null) + + then: + thrown(AssertionError) + + when: + decorator.beforeFinish(null) + + then: + thrown(AssertionError) + } + + def "test analytics rate default disabled"() { + when: + BaseDecorator dec = newDecorator(defaultEnabled) + + then: + dec.traceAnalyticsEnabled == defaultEnabled + dec.traceAnalyticsSampleRate == sampleRate + + where: + defaultEnabled | sampleRate + true | 1.0 + false | 1.0 + } + + def "test analytics rate enabled"() { + when: + BaseDecorator dec = withSystemProperty("dd.integration.${integName}.analytics.enabled", "true") { + withSystemProperty("dd.integration.${integName}.analytics.sample-rate", "$sampleRate") { + newDecorator(enabled) + } + } + + then: + dec.traceAnalyticsEnabled == expectedEnabled + dec.traceAnalyticsSampleRate == (Float) expectedRate + + where: + enabled | integName | sampleRate | expectedEnabled | expectedRate + false | "" | "" | false | 1.0 + true | "" | "" | true | 1.0 + false | "test1" | 0.5 | true | 0.5 + false | "test2" | 0.75 | true | 0.75 + true | "test1" | 0.2 | true | 0.2 + true | "test2" | 0.4 | true | 0.4 + true | "test1" | "" | true | 1.0 + true | "test2" | "" | true | 1.0 + } + + def newDecorator() { + return newDecorator(false) + } + + def newDecorator(final Boolean analyticsEnabledDefault) { + return analyticsEnabledDefault ? + new BaseDecorator() { + @Override + protected String[] instrumentationNames() { + return ["test1", "test2"] + } + + @Override + protected String spanType() { + return "test-type" + } + + @Override + protected String component() { + return "test-component" + } + + protected boolean traceAnalyticsDefault() { + return true + } + } : + new BaseDecorator() { + @Override + protected String[] instrumentationNames() { + return ["test1", "test2"] + } + + @Override + protected String spanType() { + return "test-type" + } + + @Override + protected String component() { + return "test-component" + } + } + } +} diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/ClientDecoratorTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/ClientDecoratorTest.groovy new file mode 100644 index 0000000000..cb20cb43c2 --- /dev/null +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/ClientDecoratorTest.groovy @@ -0,0 +1,72 @@ +package datadog.trace.agent.decorator + +import datadog.trace.api.DDTags +import io.opentracing.Span +import io.opentracing.tag.Tags + +class ClientDecoratorTest extends BaseDecoratorTest { + + def span = Mock(Span) + + def "test afterStart"() { + setup: + def decorator = newDecorator((String) serviceName) + + when: + decorator.afterStart(span) + + then: + if (serviceName != null) { + 1 * span.setTag(DDTags.SERVICE_NAME, serviceName) + } + 1 * span.setTag(Tags.COMPONENT.key, "test-component") + 1 * span.setTag(Tags.SPAN_KIND.key, "client") + 1 * span.setTag(DDTags.SPAN_TYPE, decorator.spanType()) + 1 * span.setTag(DDTags.ANALYTICS_SAMPLE_RATE, 1.0) + 0 * _ + + where: + serviceName << ["test-service", "other-service", null] + } + + def "test beforeFinish"() { + when: + newDecorator("test-service").beforeFinish(span) + + then: + 0 * _ + } + + @Override + def newDecorator() { + return newDecorator("test-service") + } + + def newDecorator(String serviceName) { + return new ClientDecorator() { + @Override + protected String[] instrumentationNames() { + return ["test1", "test2"] + } + + @Override + protected String service() { + return serviceName + } + + @Override + protected String spanType() { + return "test-type" + } + + @Override + protected String component() { + return "test-component" + } + + protected boolean traceAnalyticsDefault() { + return true + } + } + } +} diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/HttpClientDecoratorTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/HttpClientDecoratorTest.groovy new file mode 100644 index 0000000000..90e17ed08f --- /dev/null +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/HttpClientDecoratorTest.groovy @@ -0,0 +1,136 @@ +package datadog.trace.agent.decorator + +import datadog.trace.api.Config +import datadog.trace.api.DDTags +import io.opentracing.Span +import io.opentracing.tag.Tags + +import static datadog.trace.agent.test.utils.TraceUtils.withConfigOverride + +class HttpClientDecoratorTest extends ClientDecoratorTest { + + def span = Mock(Span) + + def "test onRequest"() { + setup: + def decorator = newDecorator() + + when: + withConfigOverride(Config.HTTP_CLIENT_HOST_SPLIT_BY_DOMAIN, "$renameService") { + decorator.onRequest(span, req) + } + + then: + if (req) { + 1 * span.setTag(Tags.HTTP_METHOD.key, "test-method") + 1 * span.setTag(Tags.HTTP_URL.key, "test-url") + 1 * span.setTag(Tags.PEER_HOSTNAME.key, "test-host") + 1 * span.setTag(Tags.PEER_PORT.key, 555) + if (renameService) { + 1 * span.setTag(DDTags.SERVICE_NAME, "test-host") + } + } + 0 * _ + + where: + renameService | req + false | null + true | null + false | [method: "test-method", url: "test-url", host: "test-host", port: 555] + true | [method: "test-method", url: "test-url", host: "test-host", port: 555] + } + + def "test onResponse"() { + setup: + def decorator = newDecorator() + + when: + decorator.onResponse(span, resp) + + then: + if (status) { + 1 * span.setTag(Tags.HTTP_STATUS.key, status) + } + if (error) { + 1 * span.setTag(Tags.ERROR.key, true) + } + 0 * _ + + where: + status | error | resp + 200 | false | [status: 200] + 399 | false | [status: 399] + 400 | true | [status: 400] + 499 | true | [status: 499] + 500 | false | [status: 500] + 600 | false | [status: 600] + null | false | [status: null] + null | false | null + } + + def "test assert null span"() { + setup: + def decorator = newDecorator() + + when: + decorator.onRequest(null, null) + + then: + thrown(AssertionError) + + when: + decorator.onResponse(null, null) + + then: + thrown(AssertionError) + } + + @Override + def newDecorator(String serviceName = "test-service") { + return new HttpClientDecorator() { + @Override + protected String[] instrumentationNames() { + return ["test1", "test2"] + } + + @Override + protected String service() { + return serviceName + } + + @Override + protected String component() { + return "test-component" + } + + @Override + protected String method(Map m) { + return m.method + } + + @Override + protected String url(Map m) { + return m.url + } + + @Override + protected String hostname(Map m) { + return m.host + } + + @Override + protected Integer port(Map m) { + return m.port + } + + @Override + protected Integer status(Map m) { + return m.status + } + + protected boolean traceAnalyticsDefault() { + return true + } + } + } +} diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/HttpServerDecoratorTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/HttpServerDecoratorTest.groovy new file mode 100644 index 0000000000..1c1e3c01ec --- /dev/null +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/HttpServerDecoratorTest.groovy @@ -0,0 +1,114 @@ +package datadog.trace.agent.decorator + +import io.opentracing.Span +import io.opentracing.tag.Tags + +class HttpServerDecoratorTest extends ServerDecoratorTest { + + def span = Mock(Span) + + def "test onRequest"() { + setup: + def decorator = newDecorator() + + when: + decorator.onRequest(span, req) + + then: + if (req) { + 1 * span.setTag(Tags.HTTP_METHOD.key, "test-method") + 1 * span.setTag(Tags.HTTP_URL.key, "test-url") + 1 * span.setTag(Tags.PEER_HOSTNAME.key, "test-host") + 1 * span.setTag(Tags.PEER_PORT.key, 555) + } + 0 * _ + + where: + req << [null, [method: "test-method", url: "test-url", host: "test-host", port: 555]] + } + + def "test onResponse"() { + setup: + def decorator = newDecorator() + + when: + decorator.onResponse(span, resp) + + then: + if (status) { + 1 * span.setTag(Tags.HTTP_STATUS.key, status) + } + if (error) { + 1 * span.setTag(Tags.ERROR.key, true) + } + 0 * _ + + where: + status | error | resp + 200 | false | [status: 200] + 399 | false | [status: 399] + 400 | false | [status: 400] + 499 | false | [status: 499] + 500 | true | [status: 500] + 600 | true | [status: 600] + null | false | [status: null] + null | false | null + } + + def "test assert null span"() { + setup: + def decorator = newDecorator() + + when: + decorator.onRequest(null, null) + + then: + thrown(AssertionError) + + when: + decorator.onResponse(null, null) + + then: + thrown(AssertionError) + } + + @Override + def newDecorator() { + return new HttpServerDecorator() { + @Override + protected String[] instrumentationNames() { + return ["test1", "test2"] + } + + @Override + protected String component() { + return "test-component" + } + + @Override + protected String method(Map m) { + return m.method + } + + @Override + protected String url(Map m) { + return m.url + } + + @Override + protected String hostname(Map m) { + return m.host + } + + @Override + protected Integer port(Map m) { + return m.port + } + + @Override + protected Integer status(Map m) { + return m.status + } + } + } +} diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/ServerDecoratorTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/ServerDecoratorTest.groovy new file mode 100644 index 0000000000..cbae6a6757 --- /dev/null +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/ServerDecoratorTest.groovy @@ -0,0 +1,57 @@ +package datadog.trace.agent.decorator + +import datadog.trace.api.DDTags +import io.opentracing.Span +import io.opentracing.tag.Tags + +class ServerDecoratorTest extends BaseDecoratorTest { + + def span = Mock(Span) + + def "test afterStart"() { + def decorator = newDecorator() + when: + decorator.afterStart(span) + + then: + 1 * span.setTag(Tags.COMPONENT.key, "test-component") + 1 * span.setTag(Tags.SPAN_KIND.key, "server") + 1 * span.setTag(DDTags.SPAN_TYPE, decorator.spanType()) + if (decorator.traceAnalyticsEnabled) { + 1 * span.setTag(DDTags.ANALYTICS_SAMPLE_RATE, 1.0) + } + 0 * _ + } + + def "test beforeFinish"() { + when: + newDecorator().beforeFinish(span) + + then: + 0 * _ + } + + @Override + def newDecorator() { + return new ServerDecorator() { + @Override + protected String[] instrumentationNames() { + return ["test1", "test2"] + } + + @Override + protected String spanType() { + return "test-type" + } + + @Override + protected String component() { + return "test-component" + } + + protected boolean traceAnalyticsDefault() { + return true + } + } + } +} diff --git a/dd-java-agent/instrumentation/trace-annotation/src/test/groovy/ConfiguredTraceAnnotationsTest.groovy b/dd-java-agent/instrumentation/trace-annotation/src/test/groovy/ConfiguredTraceAnnotationsTest.groovy index b974086801..80a0e3c79a 100644 --- a/dd-java-agent/instrumentation/trace-annotation/src/test/groovy/ConfiguredTraceAnnotationsTest.groovy +++ b/dd-java-agent/instrumentation/trace-annotation/src/test/groovy/ConfiguredTraceAnnotationsTest.groovy @@ -4,8 +4,8 @@ import dd.test.trace.annotation.SayTracedHello import java.util.concurrent.Callable -import static TraceAnnotationsInstrumentation.DEFAULT_ANNOTATIONS import static datadog.trace.agent.test.utils.TraceUtils.withSystemProperty +import static datadog.trace.instrumentation.trace_annotation.TraceAnnotationsInstrumentation.DEFAULT_ANNOTATIONS class ConfiguredTraceAnnotationsTest extends AgentTestRunner { @@ -46,11 +46,10 @@ class ConfiguredTraceAnnotationsTest extends AgentTestRunner { def "test configuration #value"() { setup: - def config = null - withSystemProperty("dd.trace.annotations", value) { - def instrumentation = new TraceAnnotationsInstrumentation() - config = instrumentation.additionalTraceAnnotations + def config = withSystemProperty("dd.trace.annotations", value) { + new TraceAnnotationsInstrumentation().additionalTraceAnnotations } + expect: config == expected.toSet() diff --git a/dd-trace-api/src/main/java/datadog/trace/api/Config.java b/dd-trace-api/src/main/java/datadog/trace/api/Config.java index 746367542c..0eddb28a59 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/Config.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/Config.java @@ -332,7 +332,7 @@ public class Config { public static Boolean getBooleanSettingFromEnvironment( final String name, final Boolean defaultValue) { final String value = getSettingFromEnvironment(name, null); - return value == null ? defaultValue : Boolean.valueOf(value); + return value == null || value.trim().isEmpty() ? defaultValue : Boolean.valueOf(value); } /** @@ -344,13 +344,23 @@ public class Config { */ public static Float getFloatSettingFromEnvironment(final String name, final Float defaultValue) { final String value = getSettingFromEnvironment(name, null); - return value == null ? defaultValue : Float.valueOf(value); + try { + return value == null ? defaultValue : Float.valueOf(value); + } catch (final NumberFormatException e) { + log.warn("Invalid configuration for " + name, e); + return defaultValue; + } } private static Integer getIntegerSettingFromEnvironment( final String name, final Integer defaultValue) { final String value = getSettingFromEnvironment(name, null); - return value == null ? defaultValue : Integer.valueOf(value); + try { + return value == null ? defaultValue : Integer.valueOf(value); + } catch (final NumberFormatException e) { + log.warn("Invalid configuration for " + name, e); + return defaultValue; + } } private static String propertyToEnvironmentName(final String name) { @@ -360,25 +370,25 @@ public class Config { private static Map getPropertyMapValue( final Properties properties, final String name, final Map defaultValue) { final String value = properties.getProperty(name); - return value == null ? defaultValue : parseMap(value, name); + return value == null || value.trim().isEmpty() ? defaultValue : parseMap(value, name); } private static List getPropertyListValue( final Properties properties, final String name, final List defaultValue) { final String value = properties.getProperty(name); - return value == null ? defaultValue : parseList(value); + return value == null || value.trim().isEmpty() ? defaultValue : parseList(value); } private static Boolean getPropertyBooleanValue( final Properties properties, final String name, final Boolean defaultValue) { final String value = properties.getProperty(name); - return value == null ? defaultValue : Boolean.valueOf(value); + return value == null || value.trim().isEmpty() ? defaultValue : Boolean.valueOf(value); } private static Integer getPropertyIntegerValue( final Properties properties, final String name, final Integer defaultValue) { final String value = properties.getProperty(name); - return value == null ? defaultValue : Integer.valueOf(value); + return value == null || value.trim().isEmpty() ? defaultValue : Integer.valueOf(value); } private static Map parseMap(final String str, final String settingName) { diff --git a/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy b/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy index f74e2d1fce..7c81dad4a2 100644 --- a/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy +++ b/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy @@ -160,6 +160,36 @@ class ConfigTest extends Specification { config.agentPort == 123 } + def "default when configured incorrectly"() { + setup: + System.setProperty(PREFIX + SERVICE_NAME, " ") + System.setProperty(PREFIX + WRITER_TYPE, " ") + System.setProperty(PREFIX + AGENT_HOST, " ") + System.setProperty(PREFIX + TRACE_AGENT_PORT, " ") + System.setProperty(PREFIX + AGENT_PORT_LEGACY, "invalid") + System.setProperty(PREFIX + PRIORITY_SAMPLING, "3") + System.setProperty(PREFIX + TRACE_RESOLVER_ENABLED, " ") + System.setProperty(PREFIX + SERVICE_MAPPING, " ") + System.setProperty(PREFIX + HEADER_TAGS, "1") + System.setProperty(PREFIX + SPAN_TAGS, "invalid") + System.setProperty(PREFIX + HTTP_CLIENT_HOST_SPLIT_BY_DOMAIN, "invalid") + + when: + def config = new Config() + + then: + config.serviceName == " " + config.writerType == " " + config.agentHost == " " + config.agentPort == 8126 + config.prioritySamplingEnabled == false + config.traceResolverEnabled == true + config.serviceMapping == [:] + config.mergedSpanTags == [:] + config.headerTags == [:] + config.httpClientSplitByDomain == false + } + def "sys props and env vars overrides for trace_agent_port and agent_port_legacy as expected"() { setup: if (overridePortEnvVar) { @@ -349,6 +379,33 @@ class ConfigTest extends Specification { integrationNames = new TreeSet<>(names) } + def "test getFloatSettingFromEnvironment(#name)"() { + setup: + environmentVariables.set("DD_ENV_ZERO_TEST", "0.0") + environmentVariables.set("DD_ENV_FLOAT_TEST", "1.0") + environmentVariables.set("DD_FLOAT_TEST", "0.2") + + System.setProperty("dd.prop.zero.test", "0") + System.setProperty("dd.prop.float.test", "0.3") + System.setProperty("dd.float.test", "0.4") + System.setProperty("dd.negative.test", "-1") + + expect: + Config.getFloatSettingFromEnvironment(name, defaultValue) == (float) expected + + where: + name | expected + "env.zero.test" | 0.0 + "prop.zero.test" | 0 + "env.float.test" | 1.0 + "prop.float.test" | 0.3 + "float.test" | 0.4 + "negative.test" | -1.0 + "default.test" | 10.0 + + defaultValue = 10.0 + } + def "verify mapping configs on tracer"() { setup: System.setProperty(PREFIX + SERVICE_MAPPING, mapString) @@ -402,6 +459,7 @@ class ConfigTest extends Specification { where: mapString | map null | [:] + "" | [:] } def "verify empty value list configs on tracer"() {