Add missing metrics for cases of client errors

Changes ported from micrometer-metrics/micrometer#4326
eclipse-ee4j · Apr 4, 2024 · d67dc74 · d67dc74
1 parent 9c5b688
commit d67dc74
Show file tree

Hide file tree

Showing 5 changed files with 38 additions and 12 deletions.
diff --git a/ext/micrometer/src/main/java/org/glassfish/jersey/micrometer/server/JerseyTags.java b/ext/micrometer/src/main/java/org/glassfish/jersey/micrometer/server/JerseyTags.java
@@ -43,6 +43,8 @@ public final class JerseyTags {
 
     private static final Tag URI_ROOT = Tag.of("uri", "root");
 
+    private static final Tag URI_UNKNOWN = Tag.of("uri", "UNKNOWN");
+
     private static final Tag EXCEPTION_NONE = Tag.of("exception", "None");
 
     private static final Tag STATUS_SERVER_ERROR = Tag.of("status", "500");
@@ -95,7 +97,10 @@ public static Tag uri(RequestEvent event) {
             }
         }
         String matchingPattern = getMatchingPattern(event);
-        if (matchingPattern.equals("/")) {
+        if (matchingPattern == null) {
+            return URI_UNKNOWN;
+        }
+        else if (matchingPattern.equals("/")) {
             return URI_ROOT;
         }
         return Tag.of("uri", matchingPattern);
@@ -108,7 +113,9 @@ static boolean isRedirection(int status) {
     static String getMatchingPattern(RequestEvent event) {
         ExtendedUriInfo uriInfo = event.getUriInfo();
         List<UriTemplate> templates = uriInfo.getMatchedTemplates();
-
+        if (templates.isEmpty()) {
+            return null;
+        }
         StringBuilder sb = new StringBuilder();
         sb.append(uriInfo.getBaseUri().getPath());
         for (int i = templates.size() - 1; i >= 0; i--) {

diff --git a/...ter/src/main/java/org/glassfish/jersey/micrometer/server/MetricsRequestEventListener.java b/...ter/src/main/java/org/glassfish/jersey/micrometer/server/MetricsRequestEventListener.java
@@ -79,7 +79,7 @@ public void onEvent(RequestEvent event) {
 
         switch (event.getType()) {
             case ON_EXCEPTION:
-                if (!isNotFoundException(event)) {
+                if (!isClientError(event)) {
                     break;
                 }
                 time(event, containerRequest);
@@ -122,13 +122,14 @@ private void time(RequestEvent event, ContainerRequest containerRequest) {
         }
     }
 
-    private boolean isNotFoundException(RequestEvent event) {
+    private boolean isClientError(RequestEvent event) {
         Throwable t = event.getException();
         if (t == null) {
             return false;
         }
-        String className = t.getClass().getCanonicalName();
-        return className.equals("jakarta.ws.rs.NotFoundException") || className.equals("javax.ws.rs.NotFoundException");
+        String className = t.getClass().getSuperclass().getCanonicalName();
+        return className.equals("jakarta.ws.rs.ClientErrorException")
+               || className.equals("javax.ws.rs.ClientErrorException");
     }
 
     private Set<Timer> shortTimers(Set<Timed> timed, RequestEvent event) {

diff --git a/...r/src/test/java/org/glassfish/jersey/micrometer/server/DefaultJerseyTagsProviderTest.java b/...r/src/test/java/org/glassfish/jersey/micrometer/server/DefaultJerseyTagsProviderTest.java
@@ -51,7 +51,7 @@ class DefaultJerseyTagsProviderTest {
 
     @Test
     void testRootPath() {
-        assertThat(tagsProvider.httpRequestTags(event(200, null, "/", (String[]) null)))
+        assertThat(tagsProvider.httpRequestTags(event(200, null, "/", "/")))
             .containsExactlyInAnyOrder(tagsFrom("root", 200, null, "SUCCESS"));
     }
 
@@ -86,21 +86,21 @@ void redirectsAreShunted() {
     @Test
     @SuppressWarnings("serial")
     void exceptionsAreMappedCorrectly() {
-        assertThat(tagsProvider.httpRequestTags(event(500, new IllegalArgumentException(), "/app", (String[]) null)))
+        assertThat(tagsProvider.httpRequestTags(event(500, new IllegalArgumentException(), "/app", "/")))
             .containsExactlyInAnyOrder(tagsFrom("/app", 500, "IllegalArgumentException", "SERVER_ERROR"));
         assertThat(tagsProvider.httpRequestTags(
-                event(500, new IllegalArgumentException(new NullPointerException()), "/app", (String[]) null)))
+                event(500, new IllegalArgumentException(new NullPointerException()), "/app", "/")))
             .containsExactlyInAnyOrder(tagsFrom("/app", 500, "NullPointerException", "SERVER_ERROR"));
-        assertThat(tagsProvider.httpRequestTags(event(406, new NotAcceptableException(), "/app", (String[]) null)))
+        assertThat(tagsProvider.httpRequestTags(event(406, new NotAcceptableException(), "/app", "/")))
             .containsExactlyInAnyOrder(tagsFrom("/app", 406, "NotAcceptableException", "CLIENT_ERROR"));
         assertThat(tagsProvider.httpRequestTags(event(500, new Exception("anonymous") {
-        }, "/app", (String[]) null))).containsExactlyInAnyOrder(tagsFrom("/app", 500,
+        }, "/app", "/"))).containsExactlyInAnyOrder(tagsFrom("/app", 500,
                 "org.glassfish.jersey.micrometer.server.DefaultJerseyTagsProviderTest$1", "SERVER_ERROR"));
     }
 
     @Test
     void longRequestTags() {
-        assertThat(tagsProvider.httpLongRequestTags(event(0, null, "/app", (String[]) null)))
+        assertThat(tagsProvider.httpLongRequestTags(event(0, null, "/app", "/")))
             .containsExactlyInAnyOrder(Tag.of("method", "GET"), Tag.of("uri", "/app"));
     }
 

diff --git a/...src/test/java/org/glassfish/jersey/micrometer/server/MetricsRequestEventListenerTest.java b/...src/test/java/org/glassfish/jersey/micrometer/server/MetricsRequestEventListenerTest.java
@@ -20,6 +20,7 @@
 
 import javax.ws.rs.NotFoundException;
 import javax.ws.rs.core.Application;
+import javax.ws.rs.core.MediaType;
 
 import io.micrometer.core.instrument.MeterRegistry;
 import io.micrometer.core.instrument.Tag;
@@ -149,6 +150,11 @@ void exceptionsAreMappedCorrectly() {
         }
         catch (Exception ignored) {
         }
+        try {
+            target("produces-text-plain").request(MediaType.APPLICATION_JSON).get();
+        }
+        catch (Exception ignored) {
+        }
 
         assertThat(registry.get(METRIC_NAME)
             .tags(tagsFrom("/throws-exception", "500", "SERVER_ERROR", "IllegalArgumentException"))
@@ -164,6 +170,11 @@ void exceptionsAreMappedCorrectly() {
             .tags(tagsFrom("/throws-mappable-exception", "410", "CLIENT_ERROR", "ResourceGoneException"))
             .timer()
             .count()).isEqualTo(1);
+
+        assertThat(registry.get(METRIC_NAME)
+                       .tags(tagsFrom("UNKNOWN", "406", "CLIENT_ERROR", "NotAcceptableException"))
+                       .timer()
+                       .count()).isEqualTo(1);
     }
 
     private static Iterable<Tag> tagsFrom(String uri, String status, String outcome, String exception) {

diff --git a/...crometer/src/test/java/org/glassfish/jersey/micrometer/server/resources/TestResource.java b/...crometer/src/test/java/org/glassfish/jersey/micrometer/server/resources/TestResource.java
@@ -89,6 +89,13 @@ public String throwsMappableException() {
         throw new ResourceGoneException("Resource has been permanently removed.");
     }
 
+    @GET
+    @Path("produces-text-plain")
+    @Produces(MediaType.TEXT_PLAIN)
+    public String producesTextPlain() {
+        return "hello";
+    }
+
     @GET
     @Path("redirect/{status}")
     public Response redirect(@PathParam("status") int status) {