From ee37dc1c31e2975a905e2e94bfecec3cf5e1c016 Mon Sep 17 00:00:00 2001 From: Jon Schneider Date: Wed, 18 Jul 2018 17:04:44 -0500 Subject: [PATCH 1/2] Make sure exception tag values are not empty in web metrics See gh-13187 --- .../web/reactive/server/WebFluxTags.java | 3 ++- .../metrics/web/servlet/WebMvcTags.java | 8 +++++--- .../server/MetricsWebFilterTests.java | 20 +++++++++++++++++++ .../web/servlet/WebMvcMetricsFilterTests.java | 18 +++++++++++++++++ 4 files changed, 45 insertions(+), 4 deletions(-) diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/WebFluxTags.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/WebFluxTags.java index 5266508b32..70f5207a8c 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/WebFluxTags.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/WebFluxTags.java @@ -109,7 +109,8 @@ public final class WebFluxTags { */ public static Tag exception(Throwable exception) { if (exception != null) { - return Tag.of("exception", exception.getClass().getSimpleName()); + String simpleName = exception.getClass().getSimpleName(); + return Tag.of("exception", simpleName.isEmpty() ? exception.getClass().getName() : simpleName); } return EXCEPTION_NONE; } diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcTags.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcTags.java index f469b6ba72..3cb0e05f53 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcTags.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcTags.java @@ -134,9 +134,11 @@ public final class WebMvcTags { * @return the exception tag derived from the exception */ public static Tag exception(Throwable exception) { - return (exception != null - ? Tag.of("exception", exception.getClass().getSimpleName()) - : EXCEPTION_NONE); + if (exception != null) { + String simpleName = exception.getClass().getSimpleName(); + return Tag.of("exception", simpleName.isEmpty() ? exception.getClass().getName() : simpleName); + } + return EXCEPTION_NONE; } } diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilterTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilterTests.java index 45d9acbe3e..6e2dbb9e42 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilterTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilterTests.java @@ -77,6 +77,26 @@ public class MetricsWebFilterTests { }).block(); assertMetricsContainsTag("uri", "/projects/{project}"); assertMetricsContainsTag("status", "500"); + assertMetricsContainsTag("exception", "IllegalStateException"); + } + + @Test + public void filterAddsNonEmptyTagsToRegistryForAnonymousExceptions() { + final Exception anonymous = new Exception("test error") {}; + + MockServerWebExchange exchange = createExchange("/projects/spring-boot", + "/projects/{project}"); + this.webFilter + .filter(exchange, + (serverWebExchange) -> Mono + .error(anonymous)) + .onErrorResume((t) -> { + exchange.getResponse().setStatusCodeValue(500); + return exchange.getResponse().setComplete(); + }).block(); + assertMetricsContainsTag("uri", "/projects/{project}"); + assertMetricsContainsTag("status", "500"); + assertMetricsContainsTag("exception", anonymous.getClass().getName()); } @Test diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcMetricsFilterTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcMetricsFilterTests.java index db6a9038dd..2fe5668754 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcMetricsFilterTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcMetricsFilterTests.java @@ -188,6 +188,18 @@ public class WebMvcMetricsFilterTests { .tags("exception", "RuntimeException").timer().count()).isEqualTo(1L); } + @Test + public void anonymousError() throws Exception { + try { + mvc.perform(get("/api/c1/anonymousError/10")); + } catch(Throwable ignore) { + } + + assertThat(this.registry.get("http.server.requests").tag("uri", "/api/c1/anonymousError/{id}").timer().getId() + .getTag("exception")) + .endsWith("$1"); + } + @Test public void asyncCallableRequest() throws Exception { AtomicReference result = new AtomicReference<>(); @@ -440,6 +452,12 @@ public class WebMvcMetricsFilterTests { throw new IllegalStateException("Boom on " + id + "!"); } + @Timed + @GetMapping("/anonymousError/{id}") + public String alwaysThrowsAnonymousException(@PathVariable Long id) throws Exception { + throw new Exception("this exception won't have a simple class name") {}; + } + @Timed @GetMapping("/unhandledError/{id}") public String alwaysThrowsUnhandledException(@PathVariable Long id) { From 592754d806af2ef1d8ecd7c5f45362afe973619e Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Thu, 19 Jul 2018 14:52:42 +0200 Subject: [PATCH 2/2] Polish "Make sure exception tag values are not empty in web metrics" Closes gh-13187 --- .../web/reactive/server/WebFluxTags.java | 4 +++- .../metrics/web/servlet/WebMvcTags.java | 3 ++- .../server/MetricsWebFilterTests.java | 8 +++----- .../web/servlet/WebMvcMetricsFilterTests.java | 20 ++++++++++--------- 4 files changed, 19 insertions(+), 16 deletions(-) diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/WebFluxTags.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/WebFluxTags.java index 70f5207a8c..519d4d3826 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/WebFluxTags.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/WebFluxTags.java @@ -19,6 +19,7 @@ package org.springframework.boot.actuate.metrics.web.reactive.server; import io.micrometer.core.instrument.Tag; import org.springframework.http.HttpStatus; +import org.springframework.util.StringUtils; import org.springframework.web.reactive.HandlerMapping; import org.springframework.web.server.ServerWebExchange; import org.springframework.web.util.pattern.PathPattern; @@ -110,7 +111,8 @@ public final class WebFluxTags { public static Tag exception(Throwable exception) { if (exception != null) { String simpleName = exception.getClass().getSimpleName(); - return Tag.of("exception", simpleName.isEmpty() ? exception.getClass().getName() : simpleName); + return Tag.of("exception", StringUtils.hasText(simpleName) ? simpleName + : exception.getClass().getName()); } return EXCEPTION_NONE; } diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcTags.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcTags.java index 3cb0e05f53..c24fe63097 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcTags.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcTags.java @@ -136,7 +136,8 @@ public final class WebMvcTags { public static Tag exception(Throwable exception) { if (exception != null) { String simpleName = exception.getClass().getSimpleName(); - return Tag.of("exception", simpleName.isEmpty() ? exception.getClass().getName() : simpleName); + return Tag.of("exception", StringUtils.hasText(simpleName) ? simpleName + : exception.getClass().getName()); } return EXCEPTION_NONE; } diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilterTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilterTests.java index 6e2dbb9e42..75d425686b 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilterTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilterTests.java @@ -82,14 +82,12 @@ public class MetricsWebFilterTests { @Test public void filterAddsNonEmptyTagsToRegistryForAnonymousExceptions() { - final Exception anonymous = new Exception("test error") {}; + final Exception anonymous = new Exception("test error") { + }; MockServerWebExchange exchange = createExchange("/projects/spring-boot", "/projects/{project}"); - this.webFilter - .filter(exchange, - (serverWebExchange) -> Mono - .error(anonymous)) + this.webFilter.filter(exchange, (serverWebExchange) -> Mono.error(anonymous)) .onErrorResume((t) -> { exchange.getResponse().setStatusCodeValue(500); return exchange.getResponse().setComplete(); diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcMetricsFilterTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcMetricsFilterTests.java index 2fe5668754..f86847881b 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcMetricsFilterTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcMetricsFilterTests.java @@ -189,15 +189,15 @@ public class WebMvcMetricsFilterTests { } @Test - public void anonymousError() throws Exception { + public void anonymousError() { try { - mvc.perform(get("/api/c1/anonymousError/10")); - } catch(Throwable ignore) { + this.mvc.perform(get("/api/c1/anonymousError/10")); } - - assertThat(this.registry.get("http.server.requests").tag("uri", "/api/c1/anonymousError/{id}").timer().getId() - .getTag("exception")) - .endsWith("$1"); + catch (Throwable ignore) { + } + assertThat(this.registry.get("http.server.requests") + .tag("uri", "/api/c1/anonymousError/{id}").timer().getId() + .getTag("exception")).endsWith("$1"); } @Test @@ -454,8 +454,10 @@ public class WebMvcMetricsFilterTests { @Timed @GetMapping("/anonymousError/{id}") - public String alwaysThrowsAnonymousException(@PathVariable Long id) throws Exception { - throw new Exception("this exception won't have a simple class name") {}; + public String alwaysThrowsAnonymousException(@PathVariable Long id) + throws Exception { + throw new Exception("this exception won't have a simple class name") { + }; } @Timed