From 2682c3b3d448a1f7969955be99b83838e0a870db Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Tue, 4 Oct 2022 22:11:10 +0200 Subject: [PATCH] Attaches error to Observation without this change when there is no response and the observation closing web exception handler stops the observation we've forgotten to attach an error to it with this change we're attaching the error --- .../DefaultGatewayObservationConvention.java | 5 +++-- .../headers/observation/GatewayContext.java | 1 + ...ObservationClosingWebExceptionHandler.java | 1 + ...B3BraveObservedHttpHeadersFilterTests.java | 11 +++++----- ...vationClosingWebExceptionHandlerTests.java | 4 +++- .../ObservedHttpHeadersFilterTests.java | 21 +++++++++---------- 6 files changed, 23 insertions(+), 20 deletions(-) diff --git a/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/headers/observation/DefaultGatewayObservationConvention.java b/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/headers/observation/DefaultGatewayObservationConvention.java index cb44f48d7..1bc4e15f9 100644 --- a/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/headers/observation/DefaultGatewayObservationConvention.java +++ b/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/headers/observation/DefaultGatewayObservationConvention.java @@ -50,8 +50,9 @@ public class DefaultGatewayObservationConvention implements GatewayObservationCo return keyValues; } Route route = context.getServerWebExchange().getAttribute(ServerWebExchangeUtils.GATEWAY_ROUTE_ATTR); - keyValues = keyValues.and(ROUTE_URI.withValue(route.getUri().toString()), - METHOD.withValue(context.getRequest().getMethod().name())) + keyValues = keyValues + .and(ROUTE_URI.withValue(route.getUri().toString()), + METHOD.withValue(context.getRequest().getMethod().name())) .and(ROUTE_ID.withValue(route.getId())); ServerHttpResponse response = context.getResponse(); if (response != null && response.getStatusCode() != null) { diff --git a/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/headers/observation/GatewayContext.java b/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/headers/observation/GatewayContext.java index 75ea3b8b2..dbe5b5b95 100644 --- a/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/headers/observation/GatewayContext.java +++ b/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/headers/observation/GatewayContext.java @@ -53,4 +53,5 @@ public class GatewayContext extends RequestReplySenderContext "Observation was not previously stopped, will stop it."); + observation.error(ex); observation.stop(); } } diff --git a/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/filter/headers/observation/B3BraveObservedHttpHeadersFilterTests.java b/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/filter/headers/observation/B3BraveObservedHttpHeadersFilterTests.java index a5e273e22..64de2e598 100644 --- a/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/filter/headers/observation/B3BraveObservedHttpHeadersFilterTests.java +++ b/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/filter/headers/observation/B3BraveObservedHttpHeadersFilterTests.java @@ -104,12 +104,11 @@ class B3BraveObservedHttpHeadersFilterTests { assertThat(headers.get("b3").get(0)).matches("^" + context.traceId() + "-(.*)-1-" + context.spanId() + "$"); List finishedSpans = testSpanHandler.spans().stream().map(BraveFinishedSpan::new) .collect(Collectors.toList()); - SpansAssert.then(finishedSpans).hasASpanWithName("HTTP GET", spanAssert -> spanAssert - .hasTag("spring.cloud.gateway.route.id", "foo") - .hasTag("http.method", "GET") - .hasTag("http.status_code", "200") - .hasTag("spring.cloud.gateway.route.uri", "http://localhost:8080/") - .hasTag("http.uri", "http://localhost:8080/get")); + SpansAssert.then(finishedSpans).hasASpanWithName("HTTP GET", + spanAssert -> spanAssert.hasTag("spring.cloud.gateway.route.id", "foo").hasTag("http.method", "GET") + .hasTag("http.status_code", "200") + .hasTag("spring.cloud.gateway.route.uri", "http://localhost:8080/") + .hasTag("http.uri", "http://localhost:8080/get")); }); } diff --git a/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/filter/headers/observation/ObservationClosingWebExceptionHandlerTests.java b/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/filter/headers/observation/ObservationClosingWebExceptionHandlerTests.java index fe4f70422..c7e6666ac 100644 --- a/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/filter/headers/observation/ObservationClosingWebExceptionHandlerTests.java +++ b/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/filter/headers/observation/ObservationClosingWebExceptionHandlerTests.java @@ -52,8 +52,10 @@ class ObservationClosingWebExceptionHandlerTests { void shouldStopTheObservationIfItWasNotStoppedPreviouslyAndThereWasAnError() { Observation observation = Mockito.mock(Observation.class); exchange.getAttributes().put(ObservedRequestHttpHeadersFilter.CHILD_OBSERVATION, observation); + RuntimeException runtimeException = new RuntimeException(); - assertThatNoException().isThrownBy(() -> handler.handle(exchange, new RuntimeException())); + assertThatNoException().isThrownBy(() -> handler.handle(exchange, runtimeException)); + Mockito.verify(observation).error(runtimeException); Mockito.verify(observation).stop(); } diff --git a/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/filter/headers/observation/ObservedHttpHeadersFilterTests.java b/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/filter/headers/observation/ObservedHttpHeadersFilterTests.java index 5ff602fe6..6c0953e35 100644 --- a/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/filter/headers/observation/ObservedHttpHeadersFilterTests.java +++ b/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/filter/headers/observation/ObservedHttpHeadersFilterTests.java @@ -84,17 +84,16 @@ public class ObservedHttpHeadersFilterTests extends SampleTestRunner { .containsEntry("X-B3-TraceId", Collections.singletonList(context.traceId())) .doesNotContainEntry("X-B3-SpanId", Collections.singletonList(context.spanId())) .containsKey("X-B3-SpanId"); - SpansAssert.then(bb.getFinishedSpans()).hasASpanWithName("HTTP GET", spanAssert -> spanAssert - .hasTag("http.method", "GET").hasTag("http.status_code", "200") - .hasTag("http.uri", "http://localhost:8080/get") - .hasTag("spring.cloud.gateway.route.uri", "http://localhost:8080/") - .hasTag("spring.cloud.gateway.route.id", "foo")); - MeterRegistryAssert.then(meterRegistry) - .hasTimerWithNameAndTags("http.client.requests", - Tags.of("spring.cloud.gateway.route.id", "foo", "error", "none", "http.method", "GET", "http.status_code", "200", "spring.cloud.gateway.route.uri", - "http://localhost:8080/")) - .hasMeterWithNameAndTags("http.client.requests.active", - Tags.of("spring.cloud.gateway.route.id", "foo", "http.method", "GET", "spring.cloud.gateway.route.uri", "http://localhost:8080/")); + SpansAssert.then(bb.getFinishedSpans()).hasASpanWithName("HTTP GET", + spanAssert -> spanAssert.hasTag("http.method", "GET").hasTag("http.status_code", "200") + .hasTag("http.uri", "http://localhost:8080/get") + .hasTag("spring.cloud.gateway.route.uri", "http://localhost:8080/") + .hasTag("spring.cloud.gateway.route.id", "foo")); + MeterRegistryAssert.then(meterRegistry).hasTimerWithNameAndTags("http.client.requests", + Tags.of("spring.cloud.gateway.route.id", "foo", "error", "none", "http.method", "GET", + "http.status_code", "200", "spring.cloud.gateway.route.uri", "http://localhost:8080/")) + .hasMeterWithNameAndTags("http.client.requests.active", Tags.of("spring.cloud.gateway.route.id", + "foo", "http.method", "GET", "spring.cloud.gateway.route.uri", "http://localhost:8080/")); }; }