From 0a12897d8ebeab21e5f49b7455c865a7f10f303f Mon Sep 17 00:00:00 2001 From: Richard Startin Date: Tue, 26 May 2020 18:31:53 +0100 Subject: [PATCH 1/2] replace Set with BitSet for HTTP statuses --- .../finatra/FinatraInstrumentation.java | 2 +- .../main/java/datadog/trace/api/Config.java | 43 +++++++++---------- .../datadog/trace/api/ConfigTest.groovy | 36 ++++++++++------ .../processor/rule/HttpStatusErrorRule.java | 4 +- 4 files changed, 46 insertions(+), 39 deletions(-) diff --git a/dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java b/dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java index a38f2c33c7a..8935a0e7a13 100644 --- a/dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java +++ b/dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java @@ -124,7 +124,7 @@ public Listener(final AgentScope scope) { @Override public void onSuccess(final Response response) { // Don't use DECORATE.onResponse because this is the controller span - if (Config.get().getHttpServerErrorStatuses().contains(DECORATE.status(response))) { + if (Config.get().getHttpServerErrorStatuses().get(DECORATE.status(response))) { scope.span().setError(true); } 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 0d8c9bb799f..9e9b070e495 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 @@ -15,9 +15,9 @@ import java.nio.file.Paths; import java.util.ArrayList; import java.util.Arrays; +import java.util.BitSet; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; @@ -174,9 +174,9 @@ public class Config { private static final boolean DEFAULT_PRIORITY_SAMPLING_ENABLED = true; private static final boolean DEFAULT_TRACE_RESOLVER_ENABLED = true; - private static final Set DEFAULT_HTTP_SERVER_ERROR_STATUSES = + private static final BitSet DEFAULT_HTTP_SERVER_ERROR_STATUSES = parseIntegerRangeSet("500-599", "default"); - private static final Set DEFAULT_HTTP_CLIENT_ERROR_STATUSES = + private static final BitSet DEFAULT_HTTP_CLIENT_ERROR_STATUSES = parseIntegerRangeSet("400-499", "default"); private static final boolean DEFAULT_HTTP_SERVER_TAG_QUERY_STRING = false; private static final boolean DEFAULT_HTTP_CLIENT_TAG_QUERY_STRING = false; @@ -273,8 +273,8 @@ private String profilingProxyPasswordMasker() { private final Map jmxTags; @Getter private final List excludedClasses; @Getter private final Map headerTags; - @Getter private final Set httpServerErrorStatuses; - @Getter private final Set httpClientErrorStatuses; + @Getter private final BitSet httpServerErrorStatuses; + @Getter private final BitSet httpClientErrorStatuses; @Getter private final boolean httpServerTagQueryString; @Getter private final boolean httpClientTagQueryString; @Getter private final boolean httpClientSplitByDomain; @@ -1071,8 +1071,8 @@ private static Set getPropagationStyleSetSettingFromEnvironmen return result; } - private static Set getIntegerRangeSettingFromEnvironment( - final String name, final Set defaultValue) { + private static BitSet getIntegerRangeSettingFromEnvironment( + final String name, final BitSet defaultValue) { final String value = getSettingFromEnvironment(name, null); try { return value == null ? defaultValue : parseIntegerRangeSet(value, name); @@ -1178,8 +1178,8 @@ private static Set getPropagationStyleSetFromPropertyValue( return null; } - private static Set getPropertyIntegerRangeValue( - final Properties properties, final String name, final Set defaultValue) { + private static BitSet getPropertyIntegerRangeValue( + final Properties properties, final String name, final BitSet defaultValue) { final String value = properties.getProperty(name); try { return value == null ? defaultValue : parseIntegerRangeSet(value, name); @@ -1220,7 +1220,7 @@ private static Map parseMap(final String str, final String setti } @NonNull - private static Set parseIntegerRangeSet(@NonNull String str, final String settingName) + private static BitSet parseIntegerRangeSet(@NonNull String str, final String settingName) throws NumberFormatException { str = str.replaceAll("\\s", ""); if (!str.matches("\\d{3}(?:-\\d{3})?(?:,\\d{3}(?:-\\d{3})?)*")) { @@ -1231,24 +1231,23 @@ private static Set parseIntegerRangeSet(@NonNull String str, final Stri throw new NumberFormatException(); } + final int lastSeparator = Math.max(str.lastIndexOf(','), str.lastIndexOf('-')); + final int maxValue = Integer.parseInt(str.substring(lastSeparator + 1)); + final BitSet set = new BitSet(maxValue); final String[] tokens = str.split(",", -1); - final Set set = new HashSet<>(); - for (final String token : tokens) { - final String[] range = token.split("-", -1); - if (range.length == 1) { - set.add(Integer.parseInt(range[0])); - } else if (range.length == 2) { - final int left = Integer.parseInt(range[0]); - final int right = Integer.parseInt(range[1]); + final int separator = token.indexOf('-'); + if (separator == -1) { + set.set(Integer.parseInt(token)); + } else if (separator > 0) { + final int left = Integer.parseInt(token.substring(0, separator)); + final int right = Integer.parseInt(token.substring(separator + 1)); final int min = Math.min(left, right); final int max = Math.max(left, right); - for (int i = min; i <= max; i++) { - set.add(i); - } + set.set(min, max + 1); } } - return Collections.unmodifiableSet(set); + return set; } @NonNull 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 87144f442ed..01c1110f80b 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 @@ -127,8 +127,8 @@ class ConfigTest extends DDSpecification { config.mergedSpanTags == [:] config.mergedJmxTags == [(RUNTIME_ID_TAG): config.getRuntimeId(), (SERVICE_TAG): config.serviceName] config.headerTags == [:] - config.httpServerErrorStatuses == (500..599).toSet() - config.httpClientErrorStatuses == (400..499).toSet() + config.httpServerErrorStatuses == toBitSet((500..599).toSet()) + config.httpClientErrorStatuses == toBitSet((400..499).toSet()) config.httpClientSplitByDomain == false config.dbClientSplitByInstance == false config.splitByTags == [].toSet() @@ -252,8 +252,8 @@ class ConfigTest extends DDSpecification { config.mergedSpanTags == [b: "2", c: "3"] config.mergedJmxTags == [b: "2", d: "4", (RUNTIME_ID_TAG): config.getRuntimeId(), (SERVICE_TAG): config.serviceName] config.headerTags == [e: "5"] - config.httpServerErrorStatuses == (122..457).toSet() - config.httpClientErrorStatuses == (111..111).toSet() + config.httpServerErrorStatuses == toBitSet((122..457).toSet()) + config.httpClientErrorStatuses == toBitSet((111..111).toSet()) config.httpClientSplitByDomain == true config.dbClientSplitByInstance == true config.splitByTags == ["some.tag1", "some.tag2"].toSet() @@ -372,8 +372,8 @@ class ConfigTest extends DDSpecification { config.mergedSpanTags == [b: "2", c: "3"] config.mergedJmxTags == [b: "2", d: "4", (RUNTIME_ID_TAG): config.getRuntimeId(), (SERVICE_TAG): config.serviceName] config.headerTags == [e: "5"] - config.httpServerErrorStatuses == (122..457).toSet() - config.httpClientErrorStatuses == (111..111).toSet() + config.httpServerErrorStatuses == toBitSet((122..457).toSet()) + config.httpClientErrorStatuses == toBitSet((111..111).toSet()) config.httpClientSplitByDomain == true config.dbClientSplitByInstance == true config.splitByTags == ["some.tag3", "some.tag2", "some.tag1"].toSet() @@ -495,8 +495,8 @@ class ConfigTest extends DDSpecification { config.serviceMapping == [:] config.mergedSpanTags == [:] config.headerTags == [:] - config.httpServerErrorStatuses == (500..599).toSet() - config.httpClientErrorStatuses == (400..499).toSet() + config.httpServerErrorStatuses == toBitSet((500..599).toSet()) + config.httpClientErrorStatuses == toBitSet((400..499).toSet()) config.httpClientSplitByDomain == false config.dbClientSplitByInstance == false config.splitByTags == [].toSet() @@ -592,8 +592,8 @@ class ConfigTest extends DDSpecification { config.mergedSpanTags == [b: "2", c: "3"] config.mergedJmxTags == [b: "2", d: "4", (RUNTIME_ID_TAG): config.getRuntimeId(), (SERVICE_TAG): config.serviceName] config.headerTags == [e: "5"] - config.httpServerErrorStatuses == (122..457).toSet() - config.httpClientErrorStatuses == (111..111).toSet() + config.httpServerErrorStatuses == toBitSet((122..457).toSet()) + config.httpClientErrorStatuses == toBitSet((111..111).toSet()) config.httpClientSplitByDomain == true config.dbClientSplitByInstance == true config.splitByTags == [].toSet() @@ -859,10 +859,10 @@ class ConfigTest extends DDSpecification { then: if (expected) { - assert config.httpServerErrorStatuses == expected.toSet() - assert config.httpClientErrorStatuses == expected.toSet() - assert propConfig.httpServerErrorStatuses == expected.toSet() - assert propConfig.httpClientErrorStatuses == expected.toSet() + assert config.httpServerErrorStatuses == toBitSet(expected.toSet()) + assert config.httpClientErrorStatuses == toBitSet(expected.toSet()) + assert propConfig.httpServerErrorStatuses == toBitSet(expected.toSet()) + assert propConfig.httpClientErrorStatuses == toBitSet(expected.toSet()) } else { assert config.httpServerErrorStatuses == Config.DEFAULT_HTTP_SERVER_ERROR_STATUSES assert config.httpClientErrorStatuses == Config.DEFAULT_HTTP_CLIENT_ERROR_STATUSES @@ -1575,4 +1575,12 @@ class ConfigTest extends DDSpecification { throw new Throwable() } } + + static BitSet toBitSet(Set set) { + BitSet bs = new BitSet() + for (Integer i : set) { + bs.set(i) + } + return bs + } } diff --git a/dd-trace-core/src/main/java/datadog/trace/core/processor/rule/HttpStatusErrorRule.java b/dd-trace-core/src/main/java/datadog/trace/core/processor/rule/HttpStatusErrorRule.java index f92ae0148aa..4fd32f6fc54 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/processor/rule/HttpStatusErrorRule.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/processor/rule/HttpStatusErrorRule.java @@ -23,11 +23,11 @@ public void processSpan( final int status = value instanceof Integer ? (int) value : Integer.parseInt(value.toString()); if (DDSpanTypes.HTTP_SERVER.equals(span.getType())) { - if (Config.get().getHttpServerErrorStatuses().contains(status)) { + if (Config.get().getHttpServerErrorStatuses().get(status)) { span.setError(true); } } else if (DDSpanTypes.HTTP_CLIENT.equals((span.getType()))) { - if (Config.get().getHttpClientErrorStatuses().contains(status)) { + if (Config.get().getHttpClientErrorStatuses().get(status)) { span.setError(true); } } From 825786892d2d3b977830843f8c997ad99355ef94 Mon Sep 17 00:00:00 2001 From: Richard Startin Date: Wed, 27 May 2020 16:39:54 +0100 Subject: [PATCH 2/2] remove unnecessary toSet() calls from ConfigTest --- .../datadog/trace/api/ConfigTest.groovy | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) 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 01c1110f80b..af57d2d202d 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 @@ -127,8 +127,8 @@ class ConfigTest extends DDSpecification { config.mergedSpanTags == [:] config.mergedJmxTags == [(RUNTIME_ID_TAG): config.getRuntimeId(), (SERVICE_TAG): config.serviceName] config.headerTags == [:] - config.httpServerErrorStatuses == toBitSet((500..599).toSet()) - config.httpClientErrorStatuses == toBitSet((400..499).toSet()) + config.httpServerErrorStatuses == toBitSet((500..599)) + config.httpClientErrorStatuses == toBitSet((400..499)) config.httpClientSplitByDomain == false config.dbClientSplitByInstance == false config.splitByTags == [].toSet() @@ -252,8 +252,8 @@ class ConfigTest extends DDSpecification { config.mergedSpanTags == [b: "2", c: "3"] config.mergedJmxTags == [b: "2", d: "4", (RUNTIME_ID_TAG): config.getRuntimeId(), (SERVICE_TAG): config.serviceName] config.headerTags == [e: "5"] - config.httpServerErrorStatuses == toBitSet((122..457).toSet()) - config.httpClientErrorStatuses == toBitSet((111..111).toSet()) + config.httpServerErrorStatuses == toBitSet((122..457)) + config.httpClientErrorStatuses == toBitSet((111..111)) config.httpClientSplitByDomain == true config.dbClientSplitByInstance == true config.splitByTags == ["some.tag1", "some.tag2"].toSet() @@ -372,8 +372,8 @@ class ConfigTest extends DDSpecification { config.mergedSpanTags == [b: "2", c: "3"] config.mergedJmxTags == [b: "2", d: "4", (RUNTIME_ID_TAG): config.getRuntimeId(), (SERVICE_TAG): config.serviceName] config.headerTags == [e: "5"] - config.httpServerErrorStatuses == toBitSet((122..457).toSet()) - config.httpClientErrorStatuses == toBitSet((111..111).toSet()) + config.httpServerErrorStatuses == toBitSet((122..457)) + config.httpClientErrorStatuses == toBitSet((111..111)) config.httpClientSplitByDomain == true config.dbClientSplitByInstance == true config.splitByTags == ["some.tag3", "some.tag2", "some.tag1"].toSet() @@ -495,8 +495,8 @@ class ConfigTest extends DDSpecification { config.serviceMapping == [:] config.mergedSpanTags == [:] config.headerTags == [:] - config.httpServerErrorStatuses == toBitSet((500..599).toSet()) - config.httpClientErrorStatuses == toBitSet((400..499).toSet()) + config.httpServerErrorStatuses == toBitSet((500..599)) + config.httpClientErrorStatuses == toBitSet((400..499)) config.httpClientSplitByDomain == false config.dbClientSplitByInstance == false config.splitByTags == [].toSet() @@ -592,8 +592,8 @@ class ConfigTest extends DDSpecification { config.mergedSpanTags == [b: "2", c: "3"] config.mergedJmxTags == [b: "2", d: "4", (RUNTIME_ID_TAG): config.getRuntimeId(), (SERVICE_TAG): config.serviceName] config.headerTags == [e: "5"] - config.httpServerErrorStatuses == toBitSet((122..457).toSet()) - config.httpClientErrorStatuses == toBitSet((111..111).toSet()) + config.httpServerErrorStatuses == toBitSet((122..457)) + config.httpClientErrorStatuses == toBitSet((111..111)) config.httpClientSplitByDomain == true config.dbClientSplitByInstance == true config.splitByTags == [].toSet() @@ -859,10 +859,10 @@ class ConfigTest extends DDSpecification { then: if (expected) { - assert config.httpServerErrorStatuses == toBitSet(expected.toSet()) - assert config.httpClientErrorStatuses == toBitSet(expected.toSet()) - assert propConfig.httpServerErrorStatuses == toBitSet(expected.toSet()) - assert propConfig.httpClientErrorStatuses == toBitSet(expected.toSet()) + assert config.httpServerErrorStatuses == toBitSet(expected) + assert config.httpClientErrorStatuses == toBitSet(expected) + assert propConfig.httpServerErrorStatuses == toBitSet(expected) + assert propConfig.httpClientErrorStatuses == toBitSet(expected) } else { assert config.httpServerErrorStatuses == Config.DEFAULT_HTTP_SERVER_ERROR_STATUSES assert config.httpClientErrorStatuses == Config.DEFAULT_HTTP_CLIENT_ERROR_STATUSES @@ -1576,7 +1576,7 @@ class ConfigTest extends DDSpecification { } } - static BitSet toBitSet(Set set) { + static BitSet toBitSet(Collection set) { BitSet bs = new BitSet() for (Integer i : set) { bs.set(i)