From c18784678df489d06a70e54fcddb5e3821d4b00c Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Mon, 6 Nov 2023 16:14:16 +0100 Subject: [PATCH] Reduce allocations in server conventions This commit optimizes the default observation conventions to reduce `KeyValues` allocations. --- .../ROOT/pages/integration/observability.adoc | 8 ++++---- ...faultServerRequestObservationConvention.java | 17 ++++++++++++++--- ...faultServerRequestObservationConvention.java | 15 ++++++++++++--- ...ServerRequestObservationConventionTests.java | 15 ++++++++++++++- ...ServerRequestObservationConventionTests.java | 14 ++++++++++++++ 5 files changed, 58 insertions(+), 11 deletions(-) diff --git a/framework-docs/modules/ROOT/pages/integration/observability.adoc b/framework-docs/modules/ROOT/pages/integration/observability.adoc index 0620f4913d..c2072e23f8 100644 --- a/framework-docs/modules/ROOT/pages/integration/observability.adoc +++ b/framework-docs/modules/ROOT/pages/integration/observability.adoc @@ -108,7 +108,7 @@ By default, the following `KeyValues` are created: |=== |Name | Description |`exception` _(required)_|Name of the exception thrown during the exchange, or `KeyValue#NONE_VALUE`} if no exception happened. -|`method` _(required)_|Name of HTTP request method or `"none"` if the request was not received properly. +|`method` _(required)_|Name of HTTP request method or `"none"` if not a well-known method. |`outcome` _(required)_|Outcome of the HTTP server exchange. |`status` _(required)_|HTTP response raw status code, or `"UNKNOWN"` if no response was created. |`uri` _(required)_|URI pattern for the matching handler if available, falling back to `REDIRECTION` for 3xx responses, `NOT_FOUND` for 404 responses, `root` for requests with no path info, and `UNKNOWN` for all other requests. @@ -141,7 +141,7 @@ By default, the following `KeyValues` are created: |=== |Name | Description |`exception` _(required)_|Name of the exception thrown during the exchange, or `"none"` if no exception happened. -|`method` _(required)_|Name of HTTP request method or `"none"` if the request was not received properly. +|`method` _(required)_|Name of HTTP request method or `"none"` if not a well-known method. |`outcome` _(required)_|Outcome of the HTTP server exchange. |`status` _(required)_|HTTP response raw status code, or `"UNKNOWN"` if no response was created. |`uri` _(required)_|URI pattern for the matching handler if available, falling back to `REDIRECTION` for 3xx responses, `NOT_FOUND` for 404 responses, `root` for requests with no path info, and `UNKNOWN` for all other requests. @@ -174,7 +174,7 @@ Instrumentation uses the `org.springframework.http.client.observation.ClientRequ [cols="a,a"] |=== |Name | Description -|`method` _(required)_|Name of HTTP request method or `"none"` if the request could not be created. +|`method` _(required)_|Name of HTTP request method or `"none"` if not a well-known method. |`uri` _(required)_|URI template used for HTTP request, or `"none"` if none was provided. Only the path part of the URI is considered. |`client.name` _(required)_|Client name derived from the request URI host. |`status` _(required)_|HTTP response raw status code, or `"IO_ERROR"` in case of `IOException`, or `"CLIENT_ERROR"` if no response was received. @@ -203,7 +203,7 @@ Instrumentation uses the `org.springframework.web.reactive.function.client.Clien [cols="a,a"] |=== |Name | Description -|`method` _(required)_|Name of HTTP request method or `"none"` if the request could not be created. +|`method` _(required)_|Name of HTTP request method or `"none"` if not a well-known method. |`uri` _(required)_|URI template used for HTTP request, or `"none"` if none was provided. Only the path part of the URI is considered. |`client.name` _(required)_|Client name derived from the request URI host. |`status` _(required)_|HTTP response raw status code, or `"IO_ERROR"` in case of `IOException`, or `"CLIENT_ERROR"` if no response was received. diff --git a/spring-web/src/main/java/org/springframework/http/server/observation/DefaultServerRequestObservationConvention.java b/spring-web/src/main/java/org/springframework/http/server/observation/DefaultServerRequestObservationConvention.java index a238f5fb69..8b10008655 100644 --- a/spring-web/src/main/java/org/springframework/http/server/observation/DefaultServerRequestObservationConvention.java +++ b/spring-web/src/main/java/org/springframework/http/server/observation/DefaultServerRequestObservationConvention.java @@ -16,9 +16,14 @@ package org.springframework.http.server.observation; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; + import io.micrometer.common.KeyValue; import io.micrometer.common.KeyValues; +import org.springframework.http.HttpMethod; import org.springframework.http.HttpStatus; import org.springframework.http.HttpStatusCode; import org.springframework.http.server.observation.ServerHttpObservationDocumentation.HighCardinalityKeyNames; @@ -55,6 +60,8 @@ public class DefaultServerRequestObservationConvention implements ServerRequestO private static final KeyValue HTTP_URL_UNKNOWN = KeyValue.of(HighCardinalityKeyNames.HTTP_URL, "UNKNOWN"); + private static final Set HTTP_METHODS = Stream.of(HttpMethod.values()).map(HttpMethod::name).collect(Collectors.toUnmodifiableSet()); + private final String name; @@ -102,9 +109,13 @@ public class DefaultServerRequestObservationConvention implements ServerRequestO } protected KeyValue method(ServerRequestObservationContext context) { - return (context.getCarrier() != null) ? - KeyValue.of(LowCardinalityKeyNames.METHOD, context.getCarrier().getMethod()) : - METHOD_UNKNOWN; + if (context.getCarrier() != null) { + String httpMethod = context.getCarrier().getMethod(); + if (HTTP_METHODS.contains(httpMethod)) { + return KeyValue.of(LowCardinalityKeyNames.METHOD, httpMethod); + } + } + return METHOD_UNKNOWN; } protected KeyValue status(ServerRequestObservationContext context) { diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/observation/DefaultServerRequestObservationConvention.java b/spring-web/src/main/java/org/springframework/http/server/reactive/observation/DefaultServerRequestObservationConvention.java index 43fe7fce32..7b2058b8ec 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/observation/DefaultServerRequestObservationConvention.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/observation/DefaultServerRequestObservationConvention.java @@ -16,9 +16,12 @@ package org.springframework.http.server.reactive.observation; +import java.util.Set; + import io.micrometer.common.KeyValue; import io.micrometer.common.KeyValues; +import org.springframework.http.HttpMethod; import org.springframework.http.HttpStatus; import org.springframework.http.HttpStatusCode; import org.springframework.http.server.reactive.observation.ServerHttpObservationDocumentation.HighCardinalityKeyNames; @@ -55,6 +58,8 @@ public class DefaultServerRequestObservationConvention implements ServerRequestO private static final KeyValue HTTP_URL_UNKNOWN = KeyValue.of(HighCardinalityKeyNames.HTTP_URL, "UNKNOWN"); + private static final Set HTTP_METHODS = Set.of(HttpMethod.values()); + private final String name; @@ -102,9 +107,13 @@ public class DefaultServerRequestObservationConvention implements ServerRequestO } protected KeyValue method(ServerRequestObservationContext context) { - return (context.getCarrier() != null) ? - KeyValue.of(LowCardinalityKeyNames.METHOD, context.getCarrier().getMethod().name()) : - METHOD_UNKNOWN; + if (context.getCarrier() != null) { + HttpMethod method = context.getCarrier().getMethod(); + if (HTTP_METHODS.contains(method)) { + return KeyValue.of(LowCardinalityKeyNames.METHOD, method.name()); + } + } + return METHOD_UNKNOWN; } protected KeyValue status(ServerRequestObservationContext context) { diff --git a/spring-web/src/test/java/org/springframework/http/server/observation/DefaultServerRequestObservationConventionTests.java b/spring-web/src/test/java/org/springframework/http/server/observation/DefaultServerRequestObservationConventionTests.java index 9964f6bf7b..9d27d421a9 100644 --- a/spring-web/src/test/java/org/springframework/http/server/observation/DefaultServerRequestObservationConventionTests.java +++ b/spring-web/src/test/java/org/springframework/http/server/observation/DefaultServerRequestObservationConventionTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2023 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -124,4 +124,17 @@ class DefaultServerRequestObservationConventionTests { .contains(KeyValue.of("http.url", "/test/notFound")); } + @Test + void addsKeyValuesForUnknownHttpMethodExchange() { + this.request.setMethod("SPRING"); + this.request.setRequestURI("/test"); + this.response.setStatus(404); + + assertThat(this.convention.getLowCardinalityKeyValues(this.context)).hasSize(5) + .contains(KeyValue.of("method", "UNKNOWN"), KeyValue.of("uri", "NOT_FOUND"), KeyValue.of("status", "404"), + KeyValue.of("exception", "none"), KeyValue.of("outcome", "CLIENT_ERROR")); + assertThat(this.convention.getHighCardinalityKeyValues(this.context)).hasSize(1) + .contains(KeyValue.of("http.url", "/test")); + } + } diff --git a/spring-web/src/test/java/org/springframework/http/server/reactive/observation/DefaultServerRequestObservationConventionTests.java b/spring-web/src/test/java/org/springframework/http/server/reactive/observation/DefaultServerRequestObservationConventionTests.java index fb0e11e90c..d8d9690dc4 100644 --- a/spring-web/src/test/java/org/springframework/http/server/reactive/observation/DefaultServerRequestObservationConventionTests.java +++ b/spring-web/src/test/java/org/springframework/http/server/reactive/observation/DefaultServerRequestObservationConventionTests.java @@ -20,6 +20,7 @@ import io.micrometer.common.KeyValue; import io.micrometer.observation.Observation; import org.junit.jupiter.api.Test; +import org.springframework.http.HttpMethod; import org.springframework.web.server.ServerWebExchange; import org.springframework.web.testfixture.http.server.reactive.MockServerHttpRequest; import org.springframework.web.testfixture.server.MockServerWebExchange; @@ -172,4 +173,17 @@ class DefaultServerRequestObservationConventionTests { KeyValue.of("exception", "none"), KeyValue.of("outcome", "UNKNOWN")); } + @Test + void addsKeyValuesForUnknownHttpMethodExchange() { + ServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest.method(HttpMethod.valueOf("SPRING"), "/test")); + ServerRequestObservationContext context = new ServerRequestObservationContext(exchange.getRequest(), exchange.getResponse(), exchange.getAttributes()); + exchange.getResponse().setRawStatusCode(404); + + assertThat(this.convention.getLowCardinalityKeyValues(context)).hasSize(5) + .contains(KeyValue.of("method", "UNKNOWN"), KeyValue.of("uri", "NOT_FOUND"), KeyValue.of("status", "404"), + KeyValue.of("exception", "none"), KeyValue.of("outcome", "CLIENT_ERROR")); + assertThat(this.convention.getHighCardinalityKeyValues(context)).hasSize(1) + .contains(KeyValue.of("http.url", "/test")); + } + }