From 5f98cad3017bde4951ada2f9f0f84d7f414e93ab Mon Sep 17 00:00:00 2001 From: Tony Clarke Date: Sun, 22 Jul 2018 22:22:59 -0400 Subject: [PATCH] Update metrics filter with feedback from ryanjbaxter and jkschneider --- .../asciidoc/gateway-grafana-dashboard.json | 20 ++--- .../main/asciidoc/spring-cloud-gateway.adoc | 6 +- .../gateway/filter/GatewayMetricsFilter.java | 67 +++++++++++---- .../filter/GatewayMetricFilterTests.java | 86 ++++++++++++++++--- .../gateway/test/BaseWebClientTests.java | 9 +- 5 files changed, 146 insertions(+), 42 deletions(-) diff --git a/docs/src/main/asciidoc/gateway-grafana-dashboard.json b/docs/src/main/asciidoc/gateway-grafana-dashboard.json index d64c35e5e..95defd61e 100644 --- a/docs/src/main/asciidoc/gateway-grafana-dashboard.json +++ b/docs/src/main/asciidoc/gateway-grafana-dashboard.json @@ -191,7 +191,7 @@ "tableColumn": "Value", "targets": [ { - "expr": "sum(gateway_requests_seconds_count{success=\"true\", job=~\"$gatewayService\"})", + "expr": "sum(gateway_requests_seconds_count{outcome=\"SUCCESSFUL\", job=~\"$gatewayService\"})", "format": "time_series", "interval": "", "intervalFactor": 2, @@ -274,7 +274,7 @@ "tableColumn": "Value", "targets": [ { - "expr": "sum(gateway_requests_seconds_count{success=\"false\", job=~\"$gatewayService\"})", + "expr": "sum(gateway_requests_seconds_count{outcome!=\"SUCCESSFUL\", job=~\"$gatewayService\"})", "format": "time_series", "interval": "", "intervalFactor": 2, @@ -348,7 +348,7 @@ "format": "time_series", "interval": "", "intervalFactor": 2, - "legendFormat": "{{ httpStatus }}", + "legendFormat": "{{ status }}", "metric": "gateway_api_time_seconds_sum", "refId": "A", "step": 240 @@ -437,7 +437,7 @@ "steppedLine": false, "targets": [ { - "expr": "gateway_requests_seconds_count{success=\"true\", routeId=~\"$routeId\", job=~\"$gatewayService\"}", + "expr": "gateway_requests_seconds_count{outcome=\"SUCCESSFUL\", routeId=~\"$routeId\", job=~\"$gatewayService\"}", "format": "time_series", "hide": false, "interval": "", @@ -531,7 +531,7 @@ "steppedLine": false, "targets": [ { - "expr": "gateway_requests_seconds_count{success=\"false\", routeId=~\"$routeId\", job=~\"$gatewayService\"}", + "expr": "gateway_requests_seconds_count{outcome!=\"SUCCESSFUL\", routeId=~\"$routeId\", job=~\"$gatewayService\"}", "format": "time_series", "interval": "", "intervalFactor": 2, @@ -623,11 +623,11 @@ "steppedLine": false, "targets": [ { - "expr": "sum(gateway_requests_seconds_count{routeId=~\"$routeId\", job=~\"$gatewayService\"}) by (job, httpStatus)", + "expr": "sum(gateway_requests_seconds_count{routeId=~\"$routeId\", job=~\"$gatewayService\"}) by (job, status)", "format": "time_series", "interval": "", "intervalFactor": 2, - "legendFormat": "{{ job }} / {{ httpStatus }}", + "legendFormat": "{{ job }} / {{ status }}", "metric": "gateway_requests_seconds_count", "refId": "A", "step": 240 @@ -755,6 +755,6 @@ ] }, "timezone": "", - "title": "APIGW", - "version": 29 -} \ No newline at end of file + "title": "Spring Cloud Gateway", + "version": 1 +} diff --git a/docs/src/main/asciidoc/spring-cloud-gateway.adoc b/docs/src/main/asciidoc/spring-cloud-gateway.adoc index cd2895fb9..2ab6834af 100644 --- a/docs/src/main/asciidoc/spring-cloud-gateway.adoc +++ b/docs/src/main/asciidoc/spring-cloud-gateway.adoc @@ -886,12 +886,12 @@ spring: === Gateway Metrics Filter -The Gateway Metrics Filter runs as long as the property spring.cloud.gateway.metrics.enabled is not set to false. This filter adds a timer metric named "gateway.requests" with the following tags: +The Gateway Metrics Filter runs as long as the property `spring.cloud.gateway.metrics.enabled` is not set to `false`. This filter adds a timer metric named "gateway.requests" with the following tags: * `routeId`: The route id * `routeUri`: The URI that the API will be routed to -* `success`: A boolean indicating if the API was succesfully routed and returned to the client -* `httpStatus`: Http Status of the request returned to the client +* `outcome` |Outcome as classified by link:https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/http/HttpStatus.Series.html[HttpStatus.Series] +* `status`: Http Status of the request returned to the client These metrics are then available to be scraped from ``/actuator/metrics/gateway.requests`` and can be easily integated with Prometheus to create a link:images/gateway-grafana-dashboard.jpeg[Grafana] link:gateway-grafana-dashboard.json[dashboard]. 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 a06c4d9d5..f690f3eaf 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 @@ -3,15 +3,18 @@ package org.springframework.cloud.gateway.filter; import static org.springframework.cloud.gateway.support.ServerWebExchangeUtils.GATEWAY_ROUTE_ATTR; import java.util.Arrays; -import java.util.concurrent.TimeUnit; import org.springframework.cloud.gateway.route.Route; import org.springframework.core.Ordered; import org.springframework.http.HttpStatus; +import org.springframework.http.server.reactive.AbstractServerHttpResponse; +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.Timer; +import io.micrometer.core.instrument.Timer.Sample; import reactor.core.publisher.Mono; public class GatewayMetricsFilter implements GlobalFilter, Ordered { @@ -29,19 +32,53 @@ public class GatewayMetricsFilter implements GlobalFilter, Ordered { @Override public Mono filter(ServerWebExchange exchange, GatewayFilterChain chain) { - long start = System.nanoTime(); - return chain.filter(exchange).then(Mono.defer(() -> { - HttpStatus statusCode = exchange.getResponse().getStatusCode(); - boolean success = statusCode.is2xxSuccessful(); - Route route = exchange.getAttribute(GATEWAY_ROUTE_ATTR); - Iterable iterableTags = Arrays.asList( - Tag.of("success", Boolean.toString(success)), - Tag.of("httpStatus", statusCode.name()), - Tag.of("routeId", route.getId()), - Tag.of("routeUri", route.getUri().toString())); - meterRegistry.timer("gateway.requests", iterableTags) - .record(System.nanoTime() - start, TimeUnit.NANOSECONDS); - return Mono.empty(); - })); + Sample sample = Timer.start(meterRegistry); + return chain.filter(exchange).then(Mono.fromRunnable(() -> { + 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) { + + ServerHttpResponse response = exchange.getResponse(); + if (response.isCommitted()) { + endTimerInner(exchange, sample); + } + else { + response.beforeCommit(() -> { + endTimerInner(exchange, sample); + return Mono.empty(); + }); + } + + } + + private void endTimerInner(ServerWebExchange exchange, Sample sample) { + String outcome = "CUSTOM"; + String status = "CUSTOM"; + HttpStatus statusCode = exchange.getResponse().getStatusCode(); + if (statusCode != null) { + outcome = statusCode.series().name(); + status = statusCode.name(); + } + else { // a non standard HTTPS status could be used. Let's be defensive here + if (exchange.getResponse() instanceof AbstractServerHttpResponse) { + Integer statusInt = ((AbstractServerHttpResponse) exchange.getResponse()) + .getStatusCodeValue(); + if (statusInt != null) { + status = String.valueOf(statusInt); + } + else { + status = "NA"; + } + } + } + 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)); } } 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 737eb6c9a..9c819f9f7 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 @@ -24,15 +24,26 @@ import static org.springframework.boot.test.context.SpringBootTest.WebEnvironmen import org.junit.Test; import org.junit.runner.RunWith; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Value; 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; +import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Import; +import org.springframework.http.HttpEntity; +import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpMethod; import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; 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; @@ -43,36 +54,89 @@ import io.micrometer.core.instrument.MeterRegistry; public class GatewayMetricFilterTests extends BaseWebClientTests { private static final String REQUEST_METRICS_NAME = "gateway.requests"; - + @Autowired private GatewayProperties properties; @Autowired private MeterRegistry meterRegistry; + @Value("${test.uri}") + private String testUri; + @Test public void gatewayRequestsMeterFilterHasTags() throws InterruptedException { assertThat(this.properties.getDefaultFilters()).isNotEmpty(); - - ClientResponse clientResponse = webClient.get().uri("/headers").exchange().block(); - assertEquals(clientResponse.statusCode(), HttpStatus.OK); - Thread.sleep(2000); // simulate scrape interval - assertMetricsContainsTag("success", Boolean.TRUE.toString()); - assertMetricsContainsTag("httpStatus", HttpStatus.OK.name()); + 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 + assertMetricsContainsTag("outcome", HttpStatus.Series.SUCCESSFUL.name()); + assertMetricsContainsTag("status", HttpStatus.OK.name()); assertMetricsContainsTag("routeId", "default_path_to_httpbin"); assertMetricsContainsTag("routeUri", "lb://testservice"); } + @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 + assertMetricsContainsTag("outcome", HttpStatus.Series.SERVER_ERROR.name()); + assertMetricsContainsTag("status", HttpStatus.INTERNAL_SERVER_ERROR.name()); + assertMetricsContainsTag("routeId", "default_path_to_httpbin"); + assertMetricsContainsTag("routeUri", testUri); + } + + @Test + public void hasMetricsForSetStatusFilter() throws InterruptedException { + HttpHeaders headers = new HttpHeaders(); + headers.set(HttpHeaders.HOST, "www.setcustomstatus.org"); + 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"); + assertMetricsContainsTag("routeUri", testUri); + } + private void assertMetricsContainsTag(String tagKey, String tagValue) { - assertThat(this.meterRegistry.get(REQUEST_METRICS_NAME).tag(tagKey, tagValue).timer() - .count()).isEqualTo(1); + assertThat(this.meterRegistry.get(REQUEST_METRICS_NAME).tag(tagKey, tagValue) + .timer().count()).isEqualTo(1); } @EnableAutoConfiguration @SpringBootConfiguration + @RestController @Import(DefaultTestConfig.class) - public static class TestConfig { + public static class CustomConfig { + @Value("${test.uri}") + protected String testUri; - } + @Bean + public RouteLocator myRouteLocator(RouteLocatorBuilder builder) { + // return builder.routes().route("test_bad_target_uri", r -> + // r.host("*.badtarget.org").uri(testUri)) + // .route("test_custom_http_status", + // r -> r.host("*.setcustomstatus.org").filters(f -> + // f.setStatus(432)).uri(testUri)) + // .build(); + return builder.routes() + .route("test_custom_http_status", r -> r.host("*.setcustomstatus.org") + .filters(f -> f.setStatus(432)).uri(testUri)) + .build(); + } + @RequestMapping("/httpbin/badtargeturi") + public String exception() { + throw new RuntimeException("an error"); + } + } } diff --git a/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/test/BaseWebClientTests.java b/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/test/BaseWebClientTests.java index 1473f00c1..603e8bea0 100644 --- a/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/test/BaseWebClientTests.java +++ b/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/test/BaseWebClientTests.java @@ -22,7 +22,6 @@ import java.time.Duration; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.junit.Before; -import org.springframework.boot.SpringApplication; import org.springframework.boot.SpringBootConfiguration; import org.springframework.boot.autoconfigure.EnableAutoConfiguration; import org.springframework.boot.web.server.LocalServerPort; @@ -35,6 +34,8 @@ import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Import; import org.springframework.core.annotation.Order; +import org.springframework.http.client.reactive.ClientHttpConnector; +import org.springframework.http.client.reactive.ReactorClientHttpConnector; import org.springframework.test.web.reactive.server.WebTestClient; import org.springframework.web.reactive.function.client.WebClient; @@ -63,9 +64,11 @@ public class BaseWebClientTests { @Before public void setup() { - //TODO: how to set new ReactorClientHttpConnector() + ClientHttpConnector httpConnector = new ReactorClientHttpConnector(); + baseUri = "http://localhost:" + port; - this.webClient = WebClient.create(baseUri); + this.webClient = WebClient.builder().clientConnector(httpConnector) + .baseUrl(baseUri).build(); this.testClient = WebTestClient.bindToServer().baseUrl(baseUri).build(); }