Reduce overhead of URLAsResourceName decorator
Using `new URL()` just to parse the path out was rather expensive.  This should improve the situation.
MODIFY
dd-java-agent/instrumentation/akka-http-10.0/src/lagomTest/groovy/LagomTest.groovy
dd-java-agent/instrumentation/akka-http-10.0/src/lagomTest/groovy/LagomTest.groovy
@@ -61,7 +61,7 @@ class LagomTest extends AgentTestRunner {
span(0) {
serviceName "unnamed-java-app"
operationName "akka-http.request"
- resourceName "GET ws://?/echo"
+ resourceName "GET /echo"
spanType DDSpanTypes.HTTP_SERVER
errored false
tags {
@@ -100,7 +100,7 @@ class LagomTest extends AgentTestRunner {
span(0) {
serviceName "unnamed-java-app"
operationName "akka-http.request"
- resourceName "GET ws://?/error"
+ resourceName "GET /error"
spanType DDSpanTypes.HTTP_SERVER
errored true
tags {

MODIFY
dd-java-agent/instrumentation/http-url-connection/src/test/groovy/UrlConnectionTest.groovy
dd-java-agent/instrumentation/http-url-connection/src/test/groovy/UrlConnectionTest.groovy
@@ -98,7 +98,7 @@ class UrlConnectionTest extends AgentTestRunner {
span(1) {
serviceName "unnamed-java-app"
operationName "file.request"
- resourceName "$url.path"
+ resourceName "file:$url.path"
spanType DDSpanTypes.HTTP_CLIENT
childOf span(0)
errored true

MODIFY
dd-trace-ot/dd-trace-ot.gradle
dd-trace-ot/dd-trace-ot.gradle
plugins {
- id "me.champeau.gradle.jmh" version "0.4.8"
+ id "me.champeau.gradle.jmh" version "0.5.0-rc-2"
}
description = 'dd-trace-ot'
@@ -54,27 +54,44 @@ dependencies {
}
jmh {
- timeUnit = 'us' // Output time unit. Available time units are: [m, s, ms, us, ns].
- benchmarkMode = ['thrpt', 'avgt']
- timeOnIteration = '1s'
+// include = [".*URLAsResourceNameBenchmark"]
+// include = ['some regular expression'] // include pattern (regular expression) for benchmarks to be executed
+// exclude = ['some regular expression'] // exclude pattern (regular expression) for benchmarks to be executed
iterations = 1 // Number of measurement iterations to do.
-// fork = 2 // How many times to forks a single benchmark. Use 0 to disable forking altogether
- jvmArgs = []
- failOnError = true // Should JMH fail immediately if any benchmark had experienced the unrecoverable error?
- warmup = '2s' // Time to spend at each warmup iteration.
- warmupIterations = 1 // Number of warmup iterations to do.
-// warmupForks = 0 // How many warmup forks to make for a single benchmark. 0 to disable warmup forks.
-
-// profilers = ['stack']
- // Use profilers to collect additional data. Supported profilers: [cl, comp, gc, stack, perf, perfnorm, perfasm, xperf, xperfasm, hs_cl, hs_comp, hs_gc, hs_rt, hs_thr]
-
+ benchmarkMode = ['thrpt', 'avgt', 'ss']
+ // Benchmark mode. Available modes are: [Throughput/thrpt, AverageTime/avgt, SampleTime/sample, SingleShotTime/ss, All/all]
+ batchSize = 1
+ // Batch size: number of benchmark method calls per operation. (some benchmark modes can ignore this setting)
+ fork = 1 // How many times to forks a single benchmark. Use 0 to disable forking altogether
+ failOnError = false // Should JMH fail immediately if any benchmark had experienced the unrecoverable error?
+ forceGC = false // Should JMH force GC between iterations?
+// jvm = 'myjvm' // Custom JVM to use when forking.
+// jvmArgs = ['Custom JVM args to use when forking.']
+// jvmArgsAppend = ['Custom JVM args to use when forking (append these)']
+// jvmArgsPrepend =[ 'Custom JVM args to use when forking (prepend these)']
// humanOutputFile = project.file("${project.buildDir}/reports/jmh/human.txt") // human-readable output file
+// resultsFile = project.file("${project.buildDir}/reports/jmh/results.txt") // results file
// operationsPerInvocation = 10 // Operations per invocation.
+// benchmarkParameters = [:] // Benchmark parameters.
+// profilers = ['stack'] // Use profilers to collect additional data. Supported profilers: [cl, comp, gc, stack, perf, perfnorm, perfasm, xperf, xperfasm, hs_cl, hs_comp, hs_gc, hs_rt, hs_thr]
+ timeOnIteration = '1s' // Time to spend at each measurement iteration.
+// resultFormat = 'CSV' // Result format type (one of CSV, JSON, NONE, SCSV, TEXT)
// synchronizeIterations = false // Synchronize iterations?
+// threads = 2 // Number of worker threads to run with.
+// threadGroups = [2,3,4] //Override thread group distribution for asymmetric benchmarks.
// timeout = '1s' // Timeout for benchmark iteration.
-// includeTests = true
- // Allows to include test sources into generate JMH jar, i.e. use it when benchmarks depend on the test classes.
+ timeUnit = 'us' // Output time unit. Available time units are: [m, s, ms, us, ns].
+// verbosity = 'NORMAL' // Verbosity mode. Available modes are: [SILENT, NORMAL, EXTRA]
+ warmup = '2s' // Time to spend at each warmup iteration.
+// warmupBatchSize = 10 // Warmup batch size: number of benchmark method calls per operation.
+ warmupForks = 1 // How many warmup forks to make for a single benchmark. 0 to disable warmup forks.
+ warmupIterations = 1 // Number of warmup iterations to do.
+// warmupMode = 'INDI' // Warmup mode for warming up selected benchmarks. Warmup modes are: [INDI, BULK, BULK_INDI].
+// warmupBenchmarks = ['.*Warmup'] // Warmup benchmarks to include in the run in addition to already selected. JMH will not measure these benchmarks, but only use them for the warmup.
- duplicateClassesStrategy = 'fail'
- jmhVersion = '1.19' // Specifies JMH version
+// zip64 = true // Use ZIP64 format for bigger archives
+ jmhVersion = '1.21' // Specifies JMH version
+// includeTests = true // Allows to include test sources into generate JMH jar, i.e. use it when benchmarks depend on the test classes.
+ duplicateClassesStrategy = 'warn'
+ // Strategy to apply when encountring duplicate classes during creation of the fat jar (i.e. while executing jmhJar task)
}

ADD
dd-trace-ot/src/jmh/java/datadog/opentracing/decorators/URLAsResourceNameBenchmark.java
+package datadog.opentracing.decorators;
+
+import ch.qos.logback.classic.Level;
+import ch.qos.logback.classic.Logger;
+import datadog.opentracing.DDSpanContext;
+import datadog.opentracing.SpanFactory;
+import org.openjdk.jmh.annotations.Benchmark;
+import org.openjdk.jmh.annotations.Scope;
+import org.openjdk.jmh.annotations.State;
+import org.slf4j.LoggerFactory;
+
+public class URLAsResourceNameBenchmark {
+ static {
+ ((Logger) LoggerFactory.getLogger(Logger.ROOT_LOGGER_NAME)).setLevel(Level.WARN);
+ }
+
+ @State(Scope.Benchmark)
+ public static class BenchmarkState {
+ private final AbstractDecorator base = new URLAsResourceName();
+
+ private final DDSpanContext ctx = SpanFactory.newSpanOf(0).context();
+ }
+
+ @Benchmark
+ public Object testPathOnly(final BenchmarkState state) {
+ return state.base.shouldSetTag(state.ctx, null, "/somepath/123/");
+ }
+
+ @Benchmark
+ public Object testFullUrl(final BenchmarkState state) {
+ return state.base.shouldSetTag(
+ state.ctx, null, "http://localhost:8080/somepath/123/?query=123#fragment");
+ }
+}

MODIFY
dd-trace-ot/src/main/java/datadog/opentracing/decorators/URLAsResourceName.java
dd-trace-ot/src/main/java/datadog/opentracing/decorators/URLAsResourceName.java
@@ -3,14 +3,10 @@ package datadog.opentracing.decorators;
import datadog.opentracing.DDSpanContext;
import datadog.trace.api.DDTags;
import io.opentracing.tag.Tags;
-import java.net.MalformedURLException;
import java.util.regex.Pattern;
-/** Decorator for servlet contrib */
public class URLAsResourceName extends AbstractDecorator {
- // Matches everything after the ? character.
- public static final Pattern QUERYSTRING = Pattern.compile("\\?.*$");
// Matches any path segments with numbers in them. (exception for versioning: "/v1/")
public static final Pattern PATH_MIXED_ALPHANUMERICS =
Pattern.compile("(?<=/)(?![vV]\\d{1,2}/)(?:[^\\/\\d\\?]*[\\d]+[^\\/\\?]*)");
@@ -27,42 +23,75 @@ public class URLAsResourceName extends AbstractDecorator {
// do nothing if the status code is already set and equals to 404.
// TODO: it assumes that Status404Decorator is active. If it's not, it will lead to unexpected
// behaviors
- if (statusCode != null && statusCode.equals("404")) {
+ if (value == null || statusCode != null && statusCode.equals("404")) {
return true;
}
+ final String rawPath = rawPathFromUrlString(String.valueOf(value).trim());
+ final String normalizedPath = normalizePath(rawPath);
+ final String resourceName = addMethodIfAvailable(context, normalizedPath);
+
+ context.setResourceName(resourceName);
+ return true;
+ }
+
+ private String rawPathFromUrlString(final String url) {
// Get the path without host:port
- String path = String.valueOf(value);
+ // url may already be just the path.
- try {
- path = new java.net.URL(path).getPath();
- } catch (final MalformedURLException e) {
- // do nothing, use the value instead of the path
+ if (url.isEmpty()) {
+ return "/";
}
- // normalize the path
- path = norm(path);
- // if the verb (GET, POST ...) is present, add it
- final String verb = (String) context.getTags().get(Tags.HTTP_METHOD.getKey());
- if (verb != null && !verb.isEmpty()) {
- path = verb + " " + path;
+ final int queryLoc = url.indexOf("?");
+ final int fragmentLoc = url.indexOf("#");
+ final int endLoc;
+ if (queryLoc < 0) {
+ if (fragmentLoc < 0) {
+ endLoc = url.length();
+ } else {
+ endLoc = fragmentLoc;
+ }
+ } else {
+ if (fragmentLoc < 0) {
+ endLoc = queryLoc;
+ } else {
+ endLoc = Math.min(queryLoc, fragmentLoc);
+ }
}
- context.setResourceName(path);
- return true;
+ final int protoLoc = url.indexOf("://");
+ if (protoLoc < 0) {
+ return url.substring(0, endLoc);
}
- // Method to normalise the url string
- private String norm(final String origin) {
+ final int pathLoc = url.indexOf("/", protoLoc + 3);
+ if (pathLoc < 0) {
+ return "/";
+ }
+
+ if (queryLoc < 0) {
+ return url.substring(pathLoc);
+ } else {
+ return url.substring(pathLoc, endLoc);
+ }
+ }
- String norm = origin;
- norm = QUERYSTRING.matcher(norm).replaceAll("");
- norm = PATH_MIXED_ALPHANUMERICS.matcher(norm).replaceAll("?");
+ // Method to normalise the url string
+ private String normalizePath(final String path) {
+ if (path.isEmpty() || path.equals("/")) {
+ return "/";
+ }
- if (norm.trim().isEmpty()) {
- norm = "/";
+ return PATH_MIXED_ALPHANUMERICS.matcher(path).replaceAll("?");
}
- return norm;
+ private String addMethodIfAvailable(final DDSpanContext context, String path) {
+ // if the verb (GET, POST ...) is present, add it
+ final String verb = (String) context.getTags().get(Tags.HTTP_METHOD.getKey());
+ if (verb != null && !verb.isEmpty()) {
+ path = verb + " " + path;
+ }
+ return path;
}
}

MODIFY
dd-trace-ot/src/test/groovy/datadog/opentracing/SpanFactory.groovy
dd-trace-ot/src/test/groovy/datadog/opentracing/SpanFactory.groovy
@@ -9,7 +9,7 @@ class SpanFactory {
ConfigUtils.makeConfigInstanceModifiable()
}
- static newSpanOf(long timestampMicro, String threadName = Thread.currentThread().name) {
+ static DDSpan newSpanOf(long timestampMicro, String threadName = Thread.currentThread().name) {
def writer = new ListWriter()
def tracer = new DDTracer(writer)
def currentThreadName = Thread.currentThread().getName()
@@ -33,7 +33,7 @@ class SpanFactory {
return new DDSpan(timestampMicro, context)
}
- static newSpanOf(DDTracer tracer) {
+ static DDSpan newSpanOf(DDTracer tracer) {
def context = new DDSpanContext(
"1",
"1",
@@ -52,7 +52,7 @@ class SpanFactory {
return new DDSpan(1, context)
}
- static newSpanOf(PendingTrace trace) {
+ static DDSpan newSpanOf(PendingTrace trace) {
def context = new DDSpanContext(
trace.traceId,
"1",

MODIFY
dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/URLAsResourceNameTest.groovy
dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/URLAsResourceNameTest.groovy
@@ -21,29 +21,43 @@ class URLAsResourceNameTest extends Specification {
@Subject
def decorator = new URLAsResourceName()
- def "remove query params"() {
+ def "pulls path from url #input"() {
when:
- def norm = decorator.norm(input)
+ def path = decorator.rawPathFromUrlString(input)
then:
- norm == output
+ path == expected
where:
- input | output
- "" | "/"
+ input | expected
"" | "/"
- "\t" | "/"
"/" | "/"
"/?asdf" | "/"
"/search" | "/search"
"/search?" | "/search"
"/search?id=100&private=true" | "/search"
"/search?id=100&private=true?" | "/search"
+ "http://localhost" | "/"
+ "http://localhost/" | "/"
+ "http://localhost/?asdf" | "/"
+ "http://local.host:8080/search" | "/search"
+ "https://localhost:443/search?" | "/search"
+ "http://local.host:80/search?id=100&private=true" | "/search"
+ "http://localhost:80/search?id=100&private=true?" | "/search"
+ "http://10.0.0.1/?asdf" | "/"
+ "http://127.0.0.1/?asdf" | "/"
+ "http://[FEDC:BA98:7654:3210:FEDC:BA98:7654:3210]:80/index.html" | "/index.html"
+ "http://[1080:0:0:0:8:800:200C:417A]/index.html" | "/index.html"
+ "http://[3ffe:2a00:100:7031::1]" | "/"
+ "http://[1080::8:800:200C:417A]/foo" | "/foo"
+ "http://[::192.9.5.5]/ipng" | "/ipng"
+ "http://[::FFFF:129.144.52.38]:80/index.html" | "/index.html"
+ "http://[2010:836B:4179::836B:4179]" | "/"
}
def "should replace all digits"() {
when:
- def norm = decorator.norm(input)
+ def norm = decorator.normalizePath(input)
then:
norm == output
@@ -60,7 +74,7 @@ class URLAsResourceNameTest extends Specification {
def "should replace segments with mixed-characters"() {
when:
- def norm = decorator.norm(input)
+ def norm = decorator.normalizePath(input)
then:
norm == output
@@ -69,20 +83,22 @@ class URLAsResourceNameTest extends Specification {
input | output
"/a1/v2" | "/?/?"
"/v3/1a" | "/v3/?"
- "/V01/v9/abc/-1?" | "/V01/v9/abc/?"
+ "/V01/v9/abc/-1" | "/V01/v9/abc/?"
"/ABC/av-1/b_2/c.3/d4d/v5f/v699/7" | "/ABC/?/?/?/?/?/?/?"
"/user/asdf123/repository/01234567-9ABC-DEF0-1234" | "/user/?/repository/?"
}
def "should leave other segments alone"() {
when:
- def norm = decorator.norm(input)
+ def norm = decorator.normalizePath(input)
then:
norm == input
where:
input | _
+ "/v0/" | _
+ "/v10/xyz" | _
"/a-b" | _
"/a_b" | _
"/a.b" | _
@@ -116,6 +132,9 @@ class URLAsResourceNameTest extends Specification {
where:
value | resourceName | tags
+ null | "fakeResource" | [:]
+ " " | "/" | [:]
+ "\t" | "/" | [:]
"/path" | "/path" | [:]
"/ABC/a-1/b_2/c.3/d4d/5f/6" | "/ABC/?/?/?/?/?/?" | [:]
"/not-found" | "fakeResource" | [(Tags.HTTP_STATUS.key): 404]

Reduce overhead of URLAsResourceName decorator

Using `new URL()` just to parse the path out was rather expensive.  This should improve the situation.