From ca915c9b5aac992b3c324b74c4dbcf26f0537e58 Mon Sep 17 00:00:00 2001 From: Tony Clarke Date: Wed, 25 Jul 2018 15:57:38 -0400 Subject: [PATCH] Update metrics with feedback from Ryan and Jon. Metrics are now reported before we send the response back to the client. Hence no need for sleeps in the client side test. --- .../gateway/filter/GatewayMetricsFilter.java | 23 ++++++++----------- .../filter/GatewayMetricFilterTests.java | 22 ++++-------------- 2 files changed, 14 insertions(+), 31 deletions(-) diff --git a/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/filter/GatewayMetricsFilter.java b/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/filter/GatewayMetricsFilter.java index 3788b32d..11ac4981 100644 --- a/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/filter/GatewayMetricsFilter.java +++ b/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/filter/GatewayMetricsFilter.java @@ -19,8 +19,6 @@ package org.springframework.cloud.gateway.filter; import static org.springframework.cloud.gateway.support.ServerWebExchangeUtils.GATEWAY_ROUTE_ATTR; -import java.util.Arrays; - import org.springframework.cloud.gateway.route.Route; import org.springframework.core.Ordered; import org.springframework.http.HttpStatus; @@ -29,7 +27,7 @@ import org.springframework.http.server.reactive.ServerHttpResponse; import org.springframework.web.server.ServerWebExchange; import io.micrometer.core.instrument.MeterRegistry; -import io.micrometer.core.instrument.Tag; +import io.micrometer.core.instrument.Tags; import io.micrometer.core.instrument.Timer; import io.micrometer.core.instrument.Timer.Sample; import reactor.core.publisher.Mono; @@ -44,17 +42,18 @@ public class GatewayMetricsFilter implements GlobalFilter, Ordered { @Override public int getOrder() { - return NettyWriteResponseFilter.WRITE_RESPONSE_FILTER_ORDER - 1; + // start the timer as soon as possible and report the metric event before we write + // response to client + return NettyWriteResponseFilter.WRITE_RESPONSE_FILTER_ORDER + 1; } @Override public Mono filter(ServerWebExchange exchange, GatewayFilterChain chain) { Sample sample = Timer.start(meterRegistry); - return chain.filter(exchange).then(Mono.fromRunnable(() -> { + + return chain.filter(exchange).doOnSuccessOrError((aVoid, ex) -> { endTimerRespectingCommit(exchange, sample); - })).doOnError(t -> { // needed for example when netty routing filter times out - endTimerRespectingCommit(exchange, sample); - }).then(); + }); } private void endTimerRespectingCommit(ServerWebExchange exchange, Sample sample) { @@ -69,7 +68,6 @@ public class GatewayMetricsFilter implements GlobalFilter, Ordered { return Mono.empty(); }); } - } private void endTimerInner(ServerWebExchange exchange, Sample sample) { @@ -93,9 +91,8 @@ public class GatewayMetricsFilter implements GlobalFilter, Ordered { } } Route route = exchange.getAttribute(GATEWAY_ROUTE_ATTR); - Iterable iterableTags = Arrays.asList(Tag.of("outcome", outcome), - Tag.of("status", status), Tag.of("routeId", route.getId()), - Tag.of("routeUri", route.getUri().toString())); - sample.stop(meterRegistry.timer("gateway.requests", iterableTags)); + Tags tags = Tags.of("outcome", outcome, "status", status, "routeId", + route.getId(), "routeUri", route.getUri().toString()); + sample.stop(meterRegistry.timer("gateway.requests", tags)); } } diff --git a/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/filter/GatewayMetricFilterTests.java b/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/filter/GatewayMetricFilterTests.java index 7c77ba64..46a957c9 100644 --- a/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/filter/GatewayMetricFilterTests.java +++ b/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/filter/GatewayMetricFilterTests.java @@ -18,7 +18,6 @@ package org.springframework.cloud.gateway.filter; import static org.assertj.core.api.Assertions.assertThat; -import static org.junit.Assert.assertEquals; import static org.springframework.boot.test.context.SpringBootTest.WebEnvironment.RANDOM_PORT; import org.junit.Test; @@ -29,7 +28,6 @@ import org.springframework.boot.SpringBootConfiguration; import org.springframework.boot.autoconfigure.EnableAutoConfiguration; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.test.web.client.TestRestTemplate; -import org.springframework.cloud.gateway.config.GatewayProperties; import org.springframework.cloud.gateway.route.RouteLocator; import org.springframework.cloud.gateway.route.builder.RouteLocatorBuilder; import org.springframework.cloud.gateway.test.BaseWebClientTests; @@ -44,7 +42,6 @@ import org.springframework.test.annotation.DirtiesContext; import org.springframework.test.context.junit4.SpringRunner; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RestController; -import org.springframework.web.reactive.function.client.ClientResponse; import io.micrometer.core.instrument.MeterRegistry; @@ -55,9 +52,6 @@ public class GatewayMetricFilterTests extends BaseWebClientTests { private static final String REQUEST_METRICS_NAME = "gateway.requests"; - @Autowired - private GatewayProperties properties; - @Autowired private MeterRegistry meterRegistry; @@ -66,11 +60,7 @@ public class GatewayMetricFilterTests extends BaseWebClientTests { @Test public void gatewayRequestsMeterFilterHasTags() throws InterruptedException { - assertThat(this.properties.getDefaultFilters()).isNotEmpty(); - ClientResponse clientResponse = webClient.get().uri("/headers").exchange() - .block(); - assertEquals(HttpStatus.OK, clientResponse.statusCode()); - Thread.sleep(1000); // allow metrics to complete in the mono following the then + testClient.get().uri("/headers").exchange().expectStatus().isOk(); assertMetricsContainsTag("outcome", HttpStatus.Series.SUCCESSFUL.name()); assertMetricsContainsTag("status", HttpStatus.OK.name()); assertMetricsContainsTag("routeId", "default_path_to_httpbin"); @@ -80,12 +70,8 @@ public class GatewayMetricFilterTests extends BaseWebClientTests { @Test public void gatewayRequestsMeterFilterHasTagsForBadTargetUri() throws InterruptedException { - assertThat(this.properties.getDefaultFilters()).isNotEmpty(); - ClientResponse clientResponse = webClient.get().uri("/badtargeturi").exchange() - .block(); - assertEquals("Expecting request to fail with http status internal server error", - HttpStatus.INTERNAL_SERVER_ERROR, clientResponse.statusCode()); - Thread.sleep(1000); // allow metrics to complete in the mono following the then + testClient.get().uri("/badtargeturi").exchange().expectStatus() + .is5xxServerError(); assertMetricsContainsTag("outcome", HttpStatus.Series.SERVER_ERROR.name()); assertMetricsContainsTag("status", HttpStatus.INTERNAL_SERVER_ERROR.name()); assertMetricsContainsTag("routeId", "default_path_to_httpbin"); @@ -96,11 +82,11 @@ public class GatewayMetricFilterTests extends BaseWebClientTests { public void hasMetricsForSetStatusFilter() throws InterruptedException { HttpHeaders headers = new HttpHeaders(); headers.set(HttpHeaders.HOST, "www.setcustomstatus.org"); + // cannot use netty client since we cannot read custom http status ResponseEntity response = new TestRestTemplate().exchange( baseUri + "/headers", HttpMethod.GET, new HttpEntity<>(headers), String.class); assertThat(response.getStatusCodeValue()).isEqualTo(432); - Thread.sleep(1000); // allow metrics to complete in the mono following the then assertMetricsContainsTag("outcome", "CUSTOM"); assertMetricsContainsTag("status", "432"); assertMetricsContainsTag("routeId", "test_custom_http_status");