Merge pull request #221 from DataDog/tyler/url-resource-name-improvements

Remove configuration of URL to ResourceName Decorator

dd-trace-ot/src/main/java/datadog/opentracing/decorators/URLAsResourceName.java

@@ -1,44 +1,24 @@
 package datadog.opentracing.decorators;
 
-import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
 import datadog.opentracing.DDSpanContext;
 import datadog.trace.api.DDTags;
-import datadog.trace.common.util.ConfigUtils;
 import io.opentracing.tag.Tags;
 import java.net.MalformedURLException;
-import java.util.ArrayList;
+import java.util.regex.Pattern;
-import java.util.List;
-import java.util.Objects;
 
 /** Decorator for servlet contrib */
 public class URLAsResourceName extends AbstractDecorator {
-  public static final String CONFIG_PATH = "dd-trace";
 
-  public static final Config.Rule RULE_QPARAM = new Config.Rule("\\?.*$", "");
+  // Matches everything after the ? character.
-  public static final Config.Rule RULE_DIGIT = new Config.Rule("\\d+", "?");
+  public static final Pattern QUERYSTRING = Pattern.compile("\\?.*$");
-  private List<Config.Rule> patterns = new ArrayList<>();
+  // Matches any path segments with numbers in them.
+  public static final Pattern PATH_MIXED_ALPHANUMERICS =
+      Pattern.compile("/(?:[^\\/\\d\\?]*[\\d]+[^\\/\\?]*)");
 
   public URLAsResourceName() {
-    this(CONFIG_PATH);
-  }
-
-  public URLAsResourceName(final String configPath) {
-
     super();
     this.setMatchingTag(Tags.HTTP_URL.getKey());
     this.setSetTag(DDTags.RESOURCE_NAME);
-
-    try {
-      final Config config = ConfigUtils.loadConfigFromResource(configPath, Config.class);
-      for (final Config.Rule pattern : config.urlResourcePatterns) {
-        patterns.add(pattern);
-      }
-    } catch (final Throwable ex) {
-      // do nothing
-    } finally {
-      patterns.add(RULE_QPARAM);
-      patterns.add(RULE_DIGIT);
-    }
   }
 
   @Override
@@ -76,95 +56,12 @@ public class URLAsResourceName extends AbstractDecorator {
   }
 
   // Method to normalise the url string
-  String norm(final String origin) {
+  private String norm(final String origin) {
 
     String norm = origin;
-
+    norm = QUERYSTRING.matcher(norm).replaceAll("");
-    // Apply rules
+    norm = PATH_MIXED_ALPHANUMERICS.matcher(norm).replaceAll("/?");
-    for (final Config.Rule p : patterns) {
-      norm = norm.replaceAll(p.regex, p.replacement);
-      // if the rule is final, so do not apply others functions
-      if (p.isFinal) {
-        break;
-      }
-    }
 
     return norm;
   }
-
-  // For tests
-  List<Config.Rule> getPatterns() {
-    return patterns;
-  }
-
-  // For tests
-  void setPatterns(final List<Config.Rule> patterns) {
-    this.patterns = patterns;
-  }
-
-  /** Properties concerning the UrlAsResourceDecorator in the YAML config */
-  @JsonIgnoreProperties(ignoreUnknown = true)
-  public static class Config {
-
-    List<Rule> urlResourcePatterns;
-
-    public List<Rule> getUrlResourcePatterns() {
-      return urlResourcePatterns;
-    }
-
-    public void setUrlResourcePatterns(final List<Rule> urlResourcePatterns) {
-      this.urlResourcePatterns = urlResourcePatterns;
-    }
-
-    public static class Rule {
-
-      String regex;
-      String replacement;
-      boolean isFinal = false;
-
-      public Rule() {}
-
-      public Rule(final String regex, final String replacement) {
-        this.regex = regex;
-        this.replacement = replacement;
-      }
-
-      public String getRegex() {
-        return regex;
-      }
-
-      public void setRegex(final String regex) {
-        this.regex = regex;
-      }
-
-      public String getReplacement() {
-        return replacement;
-      }
-
-      public void setReplacement(final String replacement) {
-        this.replacement = replacement;
-      }
-
-      public boolean isFinal() {
-        return isFinal;
-      }
-
-      public void setFinal(final boolean isFinal) {
-        this.isFinal = isFinal;
-      }
-
-      @Override
-      public boolean equals(final Object o) {
-        if (this == o) return true;
-        if (o == null || getClass() != o.getClass()) return false;
-        final Rule rule = (Rule) o;
-        return Objects.equals(regex, rule.regex) && Objects.equals(replacement, rule.replacement);
-      }
-
-      @Override
-      public int hashCode() {
-        return Objects.hash(regex, replacement, isFinal);
-      }
-    }
-  }
 }

dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/URLAsResourceNameTest.groovy

@@ -1,32 +1,17 @@
 package datadog.opentracing.decorators
 
+import datadog.opentracing.DDSpanContext
+import datadog.trace.common.sampling.PrioritySampling
+import io.opentracing.tag.Tags
 import spock.lang.Specification
+import spock.lang.Subject
 
 class URLAsResourceNameTest extends Specification {
 
-  def "load the config from the yaml files"() {
+  @Subject
-    setup:
-    def patterns = new URLAsResourceName("dd-url-patterns").getPatterns()
-
-    expect:
-    patterns.size() == 4
-    // 0 and 1 are from the config file
-    patterns.get(0).regex == ".*"
-    patterns.get(0).replacement == "foo"
-    patterns.get(1).regex == "foo"
-    patterns.get(1).replacement == "bar"
-
-    // the last and before-last are defaults
-    patterns.get(patterns.size() - 2) == URLAsResourceName.RULE_QPARAM
-    patterns.get(patterns.size() - 1) == URLAsResourceName.RULE_DIGIT
-
-  }
-
-  def "remove query params"() {
-
-    setup:
   def decorator = new URLAsResourceName()
 
+  def "remove query params"() {
     when:
     def norm = decorator.norm(input)
 
@@ -34,16 +19,14 @@ class URLAsResourceNameTest extends Specification {
     norm == output
 
     where:
-    input << ["/search?id=100&status=true"]
+    input                          | output
-    output << ["/search"]
+    "/search"                      | "/search"
-
+    "/search?"                     | "/search"
+    "/search?id=100&private=true"  | "/search"
+    "/search?id=100&private=true?" | "/search"
   }
 
   def "should replace all digits"() {
-
-    setup:
-    def decorator = new URLAsResourceName()
-
     when:
     def norm = decorator.norm(input)
 
@@ -51,18 +34,16 @@ class URLAsResourceNameTest extends Specification {
     norm == output
 
     where:
-    input << ["/user/100/repository/50"]
+    input              | output
-    output << ["/user/?/repository/?"]
+    "/1"               | "/?"
-
+    "/9999"            | "/?"
+    "/user/1"          | "/user/?"
+    "/user/1/"         | "/user/?/"
+    "/user/1/repo/50"  | "/user/?/repo/?"
+    "/user/1/repo/50/" | "/user/?/repo/?/"
   }
 
-  def "norm should apply custom rules"() {
+  def "should replace segments with mixed-characters"() {
-
-    setup:
-    def decorator = new URLAsResourceName()
-    def r1 = new URLAsResourceName.Config.Rule(/(\\/users\\/)([^\/]*)/, /$1:id/)
-    decorator.setPatterns(Arrays.asList(r1))
-
     when:
     def norm = decorator.norm(input)
 
@@ -70,37 +51,58 @@ class URLAsResourceNameTest extends Specification {
     norm == output
 
     where:
-    input << ["/users/guillaume/list_repository/"]
+    input                                              | output
-    output << ["/users/:id/list_repository/"]
+    "/a1"                                              | "/?"
-
+    "/1a"                                              | "/?"
+    "/abc/-1?"                                         | "/abc/?"
+    "/ABC/a-1/b_2/c.3/d4d/5f/6"                        | "/ABC/?/?/?/?/?/?"
+    "/user/asdf123/repository/01234567-9ABC-DEF0-1234" | "/user/?/repository/?"
   }
 
-  def "skip others rules if the current is set as final"() {
+  def "should leave other segments alone"() {
-
-    // Same test as above except we replace :id by :id_01
-    // And we want to stop the rule chain just after that.
-
-    setup:
-    def decorator = new URLAsResourceName()
-    def r1 = new URLAsResourceName.Config.Rule(/(\\/users\\/)([^\/]*)/, /$1:id_01/)
-    decorator.setPatterns(Arrays.asList(r1, URLAsResourceName.RULE_DIGIT))
-
     when:
-    r1.setFinal(false)
     def norm = decorator.norm(input)
 
     then:
-    norm == "/users/:id_?/list_repository/"
+    norm == input
-
-    when:
-    r1.setFinal(true)
-    norm = decorator.norm(input)
-
-    then:
-    norm == "/users/:id_01/list_repository/"
 
     where:
-    input = "/users/guillaume/list_repository/"
+    input      | _
+    "/a-b"     | _
+    "/a_b"     | _
+    "/a.b"     | _
+    "/a-b/a-b" | _
+    "/a_b/a_b" | _
+    "/a.b/a.b" | _
+  }
 
+  def "sets the resource name"() {
+    when:
+    final DDSpanContext context =
+      new DDSpanContext(
+        1L,
+        1L,
+        0L,
+        "fakeService",
+        "fakeOperation",
+        "fakeResource",
+        PrioritySampling.UNSET,
+        Collections.<String, String> emptyMap(),
+        false,
+        "fakeType",
+        tags,
+        null,
+        null)
+
+    then:
+    decorator.afterSetTag(context, Tags.HTTP_URL.getKey(), value)
+    context.resourceName == resourceName
+
+    where:
+    value                       | resourceName        | tags
+    "/path"                     | "/path"             | [:]
+    "/ABC/a-1/b_2/c.3/d4d/5f/6" | "/ABC/?/?/?/?/?/?"  | [:]
+    "/not-found"                | "fakeResource"      | [(Tags.HTTP_STATUS.key): 404]
+    "/with-method"              | "Post /with-method" | [(Tags.HTTP_METHOD.key): "Post"]
   }
 }

dd-trace-ot/src/test/resources/dd-url-patterns.yaml

@@ -1,15 +0,0 @@
-decorators:
-  - type: HTTPComponent
-    matchingValue: hello
-    setValue: world
-  - type: HTTPComponent
-    matchingValue: foo
-    setValue: bar
-  - type: URLAsResourceName
-
-# Configuration of the URLAsResourceName
-urlResourcePatterns:
-  - regex: ".*"
-    replacement: "foo"
-  - regex: "foo"
-    replacement: "bar"