Merge pull request quarkusio#14859 from ebullient/match

Verify behavior of micrometer matchPattern

extensions/micrometer/runtime/src/main/java/io/quarkus/micrometer/runtime/binder/HttpBinderConfiguration.java

@@ -2,7 +2,7 @@
 
 import java.util.ArrayList;
 import java.util.Collections;
-import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
@@ -21,7 +21,7 @@
  * other extension configuration).
  *
  * This is a synthetic bean created at runtime init (see MicrometerRecorder),
- * so references to this bean must be deferred until runtime.
+ * it cannot be referenced during build or static initialization.
  */
 public class HttpBinderConfiguration {
     private static final Logger log = Logger.getLogger(HttpBinderConfiguration.class);
@@ -35,6 +35,7 @@ public class HttpBinderConfiguration {
     List<Pattern> clientIgnorePatterns = Collections.emptyList();
     Map<Pattern, String> clientMatchPatterns = Collections.emptyMap();
 
+    @SuppressWarnings("deprecation")
     public HttpBinderConfiguration(boolean httpServerMetrics, boolean httpClientMetrics,
             HttpServerConfig serverConfig, HttpClientConfig clientConfig, VertxConfig vertxConfig) {
 
@@ -94,7 +95,7 @@ List<Pattern> getIgnorePatterns(Optional<List<String>> configInput) {
     Map<Pattern, String> getMatchPatterns(Optional<List<String>> configInput) {
         if (configInput.isPresent()) {
             List<String> input = configInput.get();
-            Map<Pattern, String> matchPatterns = new HashMap<>(input.size());
+            Map<Pattern, String> matchPatterns = new LinkedHashMap<>(input.size());
             for (String s : input) {
                 int pos = s.indexOf("=");
                 if (pos > 0 && s.length() > 2) {

extensions/micrometer/runtime/src/main/java/io/quarkus/micrometer/runtime/binder/HttpRequestMetric.java

@@ -24,13 +24,13 @@ public class HttpRequestMetric {
     volatile RoutingContext routingContext;
 
     /** Do not measure requests until/unless a uri path is set */
-    boolean measure = false;
+    final boolean measure;
 
     /** URI path used as a tag value for non-error requests */
-    String path;
+    final String path;
 
     /** True IFF the path was revised by a matcher expression */
-    boolean pathMatched = false;
+    final boolean pathMatched;
 
     /** Store the sample used to measure the request */
     Timer.Sample sample;
@@ -41,55 +41,72 @@ public class HttpRequestMetric {
      */
     Tags tags = Tags.empty();
 
-    public Timer.Sample getSample() {
-        return sample;
-    }
-
-    public void setSample(Timer.Sample sample) {
-        this.sample = sample;
-    }
-
-    public Tags getTags() {
-        return tags;
-    }
-
-    public void setTags(Tags tags) {
-        this.tags = tags;
-    }
-
     /**
      * Extract the path out of the uri. Return null if the path should be
      * ignored.
      */
-    public void parseUriPath(Map<Pattern, String> matchPattern, List<Pattern> ignorePatterns,
+    public HttpRequestMetric(Map<Pattern, String> matchPattern, List<Pattern> ignorePatterns,
             String uri) {
         if (uri == null) {
+            this.measure = false;
+            this.pathMatched = false;
+            this.path = null;
             return;
         }
 
-        String path = "/" + extractPath(uri);
-        path = HttpMetricsCommon.MULTIPLE_SLASH_PATTERN.matcher(path).replaceAll("/");
-        path = HttpMetricsCommon.TRAILING_SLASH_PATTERN.matcher(path).replaceAll("");
-
-        if (path.isEmpty()) {
-            path = "/";
-        }
-        this.path = path;
-        for (Map.Entry<Pattern, String> mp : matchPattern.entrySet()) {
-            this.path = mp.getKey().matcher(this.path).replaceAll(mp.getValue());
+        boolean matched = false;
+        String workingPath = extractPath(uri);
+        String finalPath = workingPath;
+        if ("/".equals(workingPath) || workingPath.isEmpty()) {
+            finalPath = "/";
+        } else {
+            // Label value consistency: result should begin with a '/' and should not end with one
+            workingPath = HttpMetricsCommon.MULTIPLE_SLASH_PATTERN.matcher('/' + workingPath).replaceAll("/");
+            workingPath = HttpMetricsCommon.TRAILING_SLASH_PATTERN.matcher(workingPath).replaceAll("");
+            if (workingPath.isEmpty()) {
+                finalPath = "/";
+            } else {
+                finalPath = workingPath;
+                // test path against configured patterns (whole path)
+                for (Map.Entry<Pattern, String> mp : matchPattern.entrySet()) {
+                    if (mp.getKey().matcher(workingPath).matches()) {
+                        finalPath = mp.getValue();
+                        matched = true;
+                        break;
+                    }
+                }
+            }
         }
-        this.pathMatched = !path.equals(this.path);
+        this.path = finalPath;
+        this.pathMatched = matched;
 
         // Compare path against "ignore this path" patterns
         for (Pattern p : ignorePatterns) {
             if (p.matcher(this.path).matches()) {
                 log.debugf("Path %s ignored; matches pattern %s", uri, p.pattern());
+                this.measure = false;
                 return;
             }
         }
         this.measure = true;
     }
 
+    public Timer.Sample getSample() {
+        return sample;
+    }
+
+    public void setSample(Timer.Sample sample) {
+        this.sample = sample;
+    }
+
+    public Tags getTags() {
+        return tags;
+    }
+
+    public void setTags(Tags tags) {
+        this.tags = tags;
+    }
+
     public String getPath() {
         return path;
     }
@@ -166,4 +183,13 @@ public RoutingContext getRoutingContext() {
     public void setRoutingContext(RoutingContext routingContext) {
         this.routingContext = routingContext;
     }
+
+    @Override
+    public String toString() {
+        return "HttpRequestMetric{path=" + path
+                + ",pathMatched=" + pathMatched
+                + ",measure=" + measure
+                + ",tags=" + tags
+                + '}';
+    }
 }

extensions/micrometer/runtime/src/main/java/io/quarkus/micrometer/runtime/binder/RestClientMetrics.java

@@ -62,9 +62,11 @@ class MetricsClientRequestFilter implements ClientRequestFilter {
 
         @Override
         public void filter(ClientRequestContext requestContext) throws IOException {
-            HttpRequestMetric requestMetric = new HttpRequestMetric();
-            requestMetric.parseUriPath(binderConfiguration.getClientMatchPatterns(),
-                    binderConfiguration.getClientIgnorePatterns(), requestContext.getUri().getPath());
+            HttpRequestMetric requestMetric = new HttpRequestMetric(
+                    binderConfiguration.getClientMatchPatterns(),
+                    binderConfiguration.getClientIgnorePatterns(),
+                    requestContext.getUri().getPath());
+
             if (requestMetric.isMeasure()) {
                 requestMetric.setSample(Timer.start(registry));
                 requestContext.setProperty(REQUEST_METRIC_PROPERTY, requestMetric);

extensions/micrometer/runtime/src/main/java/io/quarkus/micrometer/runtime/binder/vertx/VertxHttpServerMetrics.java

@@ -93,8 +93,7 @@ public static HttpRequestMetric retrieveRequestMetric(Context context) {
     @Override
     public HttpRequestMetric responsePushed(Map<String, Object> socketMetric, HttpMethod method, String uri,
             HttpServerResponse response) {
-        HttpRequestMetric requestMetric = new HttpRequestMetric();
-        requestMetric.parseUriPath(matchPatterns, ignorePatterns, uri);
+        HttpRequestMetric requestMetric = new HttpRequestMetric(matchPatterns, ignorePatterns, uri);
         if (requestMetric.isMeasure()) {
             registry.counter(nameHttpServerPush, Tags.of(
                     HttpMetricsCommon.uri(requestMetric.getPath(), response.getStatusCode()),
@@ -118,11 +117,10 @@ public HttpRequestMetric responsePushed(Map<String, Object> socketMetric, HttpMe
      */
     @Override
     public HttpRequestMetric requestBegin(Map<String, Object> socketMetric, HttpServerRequest request) {
-        HttpRequestMetric requestMetric = new HttpRequestMetric();
+        // evaluate and remember the path to monitor for use later (maybe a 404 or redirect..)
+        HttpRequestMetric requestMetric = new HttpRequestMetric(matchPatterns, ignorePatterns, request.path());
         setRequestMetric(Vertx.currentContext(), requestMetric);
 
-        // evaluate and remember the path to monitor for use later (maybe a 404 or redirect..)
-        requestMetric.parseUriPath(matchPatterns, ignorePatterns, request.path());
         if (requestMetric.isMeasure()) {
             // If we're measuring this request, create/remember the sample
             requestMetric.setSample(Timer.start(registry));

extensions/micrometer/runtime/src/main/java/io/quarkus/micrometer/runtime/config/runtime/HttpClientConfig.java

@@ -20,9 +20,11 @@ public class HttpClientConfig {
      * Patterns specified here will take precedence over those computed
      * values.
      *
-     * For example, if `/item/\\d+=/item/custom` is specified in this list,
+     * For example, if `/item/\\\\d+=/item/custom` or
+     * `/item/[0-9]+=/item/custom` is specified in this list,
      * a request to a matching path (`/item/123`) will use the specified
      * replacement value (`/item/custom`) as the value for the uri label.
+     * Note that backslashes must be double escaped as {@code \\\\}.
      *
      * @asciidoclet
      */

extensions/micrometer/runtime/src/main/java/io/quarkus/micrometer/runtime/config/runtime/HttpServerConfig.java

@@ -20,9 +20,11 @@ public class HttpServerConfig {
      * Patterns specified here will take precedence over those computed
      * values.
      *
-     * For example, if `/item/\\d+=/item/custom` is specified in this list,
+     * For example, if `/item/\\\\d+=/item/custom` or
+     * `/item/[0-9]+=/item/custom` is specified in this list,
      * a request to a matching path (`/item/123`) will use the specified
      * replacement value (`/item/custom`) as the value for the uri label.
+     * Note that backslashes must be double escaped as {@code \\\\}.
      *
      * @asciidoclet
      */
@@ -37,17 +39,11 @@ public class HttpServerConfig {
     public Optional<List<String>> ignorePatterns = Optional.empty();
 
     public void mergeDeprecatedConfig(VertxConfig config) {
-        System.out.println("BEFORE");
-        System.out.println(ignorePatterns);
-        System.out.println(matchPatterns);
         if (!ignorePatterns.isPresent()) {
             ignorePatterns = config.ignorePatterns;
         }
         if (!matchPatterns.isPresent()) {
             matchPatterns = config.matchPatterns;
         }
-        System.out.println("AFTER");
-        System.out.println(ignorePatterns);
-        System.out.println(matchPatterns);
     }
 }

extensions/micrometer/runtime/src/test/java/io/quarkus/micrometer/runtime/binder/HttpRequestMetricTest.java

@@ -29,7 +29,7 @@ public class HttpRequestMetricTest {
 
     @Test
     public void testReturnPathFromHttpRequestPath() {
-        HttpRequestMetric requestMetric = new HttpRequestMetric();
+        HttpRequestMetric requestMetric = new HttpRequestMetric(NO_MATCH_PATTERNS, NO_IGNORE_PATTERNS, "/");
         requestMetric.routingContext = Mockito.mock(RoutingContext.class);
 
         Mockito.when(requestMetric.routingContext.get(HttpRequestMetric.HTTP_REQUEST_PATH))
@@ -40,7 +40,7 @@ public void testReturnPathFromHttpRequestPath() {
 
     @Test
     public void testReturnPathFromRoutingContext() {
-        HttpRequestMetric requestMetric = new HttpRequestMetric();
+        HttpRequestMetric requestMetric = new HttpRequestMetric(NO_MATCH_PATTERNS, NO_IGNORE_PATTERNS, "/");
         requestMetric.routingContext = Mockito.mock(RoutingContext.class);
         Route currentRoute = Mockito.mock(Route.class);
 
@@ -52,7 +52,7 @@ public void testReturnPathFromRoutingContext() {
 
     @Test
     public void testReturnGenericPathFromRoutingContext() {
-        HttpRequestMetric requestMetric = new HttpRequestMetric();
+        HttpRequestMetric requestMetric = new HttpRequestMetric(NO_MATCH_PATTERNS, NO_IGNORE_PATTERNS, "/");
         requestMetric.routingContext = Mockito.mock(RoutingContext.class);
         Route currentRoute = Mockito.mock(Route.class);
 
@@ -66,79 +66,75 @@ public void testReturnGenericPathFromRoutingContext() {
 
     @Test
     public void testParsePathDoubleSlash() {
-        HttpRequestMetric requestMetric = new HttpRequestMetric();
-        requestMetric.parseUriPath(NO_MATCH_PATTERNS, NO_IGNORE_PATTERNS, "//");
+        HttpRequestMetric requestMetric = new HttpRequestMetric(NO_MATCH_PATTERNS, NO_IGNORE_PATTERNS, "//");
         Assertions.assertEquals("/", requestMetric.path);
-        Assertions.assertTrue(requestMetric.measure);
-        Assertions.assertFalse(requestMetric.pathMatched);
+        Assertions.assertTrue(requestMetric.measure, "Path should be measured");
+        Assertions.assertFalse(requestMetric.pathMatched, "Path should not be marked as matched");
     }
 
     @Test
     public void testParseEmptyPath() {
-        HttpRequestMetric requestMetric = new HttpRequestMetric();
-        requestMetric.parseUriPath(NO_MATCH_PATTERNS, NO_IGNORE_PATTERNS, "");
-        Assertions.assertTrue(requestMetric.measure);
-        Assertions.assertFalse(requestMetric.pathMatched);
+        HttpRequestMetric requestMetric = new HttpRequestMetric(NO_MATCH_PATTERNS, NO_IGNORE_PATTERNS, "");
         Assertions.assertEquals("/", requestMetric.path);
+        Assertions.assertTrue(requestMetric.measure, "Path should be measured");
+        Assertions.assertFalse(requestMetric.pathMatched, "Path should not be marked as matched");
     }
 
     @Test
     public void testParsePathNoLeadingSlash() {
-        HttpRequestMetric requestMetric = new HttpRequestMetric();
-        requestMetric.parseUriPath(NO_MATCH_PATTERNS, NO_IGNORE_PATTERNS, "path/with/no/leading/slash");
+        HttpRequestMetric requestMetric = new HttpRequestMetric(NO_MATCH_PATTERNS, NO_IGNORE_PATTERNS,
+                "path/with/no/leading/slash");
         Assertions.assertEquals("/path/with/no/leading/slash", requestMetric.path);
-        Assertions.assertTrue(requestMetric.measure);
-        Assertions.assertFalse(requestMetric.pathMatched);
+        Assertions.assertTrue(requestMetric.measure, "Path should be measured");
+        Assertions.assertFalse(requestMetric.pathMatched, "Path should not be marked as matched");
     }
 
     @Test
     public void testParsePathWithQueryString() {
-        HttpRequestMetric requestMetric = new HttpRequestMetric();
-        requestMetric.parseUriPath(NO_MATCH_PATTERNS, NO_IGNORE_PATTERNS, "/path/with/query/string?stuff");
+        HttpRequestMetric requestMetric = new HttpRequestMetric(NO_MATCH_PATTERNS, NO_IGNORE_PATTERNS,
+                "/path/with/query/string?stuff");
         Assertions.assertEquals("/path/with/query/string", requestMetric.path);
-        Assertions.assertTrue(requestMetric.measure);
-        Assertions.assertFalse(requestMetric.pathMatched);
+        Assertions.assertTrue(requestMetric.measure, "Path should be measured");
+        Assertions.assertFalse(requestMetric.pathMatched, "Path should not be marked as matched");
     }
 
     @Test
     public void testParsePathIgnoreNoLeadingSlash() {
-        HttpRequestMetric requestMetric = new HttpRequestMetric();
-        requestMetric.parseUriPath(NO_MATCH_PATTERNS, ignorePatterns, "ignore/me/with/no/leading/slash");
+        HttpRequestMetric requestMetric = new HttpRequestMetric(NO_MATCH_PATTERNS, ignorePatterns,
+                "ignore/me/with/no/leading/slash");
         Assertions.assertEquals("/ignore/me/with/no/leading/slash", requestMetric.path);
-        Assertions.assertFalse(requestMetric.measure);
-        Assertions.assertFalse(requestMetric.pathMatched);
+        Assertions.assertFalse(requestMetric.measure, "Path should be measured");
+        Assertions.assertFalse(requestMetric.pathMatched, "Path should not be marked as matched");
     }
 
     @Test
     public void testParsePathIgnoreWithQueryString() {
-        HttpRequestMetric requestMetric = new HttpRequestMetric();
-        requestMetric.parseUriPath(NO_MATCH_PATTERNS, ignorePatterns, "/ignore/me/with/query/string?stuff");
+        HttpRequestMetric requestMetric = new HttpRequestMetric(NO_MATCH_PATTERNS, ignorePatterns,
+                "/ignore/me/with/query/string?stuff");
         Assertions.assertEquals("/ignore/me/with/query/string", requestMetric.path);
-        Assertions.assertFalse(requestMetric.measure);
-        Assertions.assertFalse(requestMetric.pathMatched);
+        Assertions.assertFalse(requestMetric.measure, "Path should be measured");
+        Assertions.assertFalse(requestMetric.pathMatched, "Path should not be marked as matched");
     }
 
     @Test
     public void testParsePathMatchReplaceNoLeadingSlash() {
         final Map<Pattern, String> matchPatterns = new HashMap<>();
         matchPatterns.put(Pattern.compile("/item/\\d+"), "/item/{id}");
 
-        HttpRequestMetric requestMetric = new HttpRequestMetric();
-        requestMetric.parseUriPath(matchPatterns, NO_IGNORE_PATTERNS, "item/123");
+        HttpRequestMetric requestMetric = new HttpRequestMetric(matchPatterns, NO_IGNORE_PATTERNS, "item/123");
         Assertions.assertEquals("/item/{id}", requestMetric.path);
-        Assertions.assertTrue(requestMetric.measure);
-        Assertions.assertTrue(requestMetric.pathMatched);
+        Assertions.assertTrue(requestMetric.measure, "Path should be measured");
+        Assertions.assertTrue(requestMetric.pathMatched, "Path should be marked as matched");
     }
 
     @Test
     public void testParsePathMatchReplaceLeadingSlash() {
         final Map<Pattern, String> matchPatterns = new HashMap<>();
         matchPatterns.put(Pattern.compile("/item/\\d+"), "/item/{id}");
 
-        HttpRequestMetric requestMetric = new HttpRequestMetric();
-        requestMetric.parseUriPath(matchPatterns, NO_IGNORE_PATTERNS, "/item/123");
+        HttpRequestMetric requestMetric = new HttpRequestMetric(matchPatterns, NO_IGNORE_PATTERNS, "/item/123");
         Assertions.assertEquals("/item/{id}", requestMetric.path);
-        Assertions.assertTrue(requestMetric.measure);
-        Assertions.assertTrue(requestMetric.pathMatched);
+        Assertions.assertTrue(requestMetric.measure, "Path should be measured");
+        Assertions.assertTrue(requestMetric.pathMatched, "Path should be marked as matched");
     }
 }