From ffb63d68eba044cd1d7b305ab167dbd9c207a1f1 Mon Sep 17 00:00:00 2001 From: Helen <56097766+heyams@users.noreply.github.com> Date: Fri, 21 Apr 2023 00:27:28 -0700 Subject: [PATCH] Fix nested http.route (#8282) Co-authored-by: Trask Stalnaker --- .../server/HandlerAdapterInstrumentation.java | 6 +++++- ...mediateHandlerSpringWebFluxServerTest.java | 21 +++++++++++++++++++ .../server/base/ServerTestController.java | 12 +++++++++++ .../server/base/ServerTestRouteFactory.java | 14 ++++++++++++- .../server/base/SpringWebFluxServerTest.java | 2 ++ .../junit/http/AbstractHttpServerTest.java | 2 +- .../testing/junit/http/ServerEndpoint.java | 5 +++++ 7 files changed, 59 insertions(+), 3 deletions(-) diff --git a/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webflux/v5_0/server/HandlerAdapterInstrumentation.java b/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webflux/v5_0/server/HandlerAdapterInstrumentation.java index 4acd899bbac7..9660a1c3981f 100644 --- a/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webflux/v5_0/server/HandlerAdapterInstrumentation.java +++ b/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webflux/v5_0/server/HandlerAdapterInstrumentation.java @@ -65,8 +65,12 @@ public static void methodEnter( Context parentContext = Context.current(); + // HttpRouteSource.CONTROLLER has useFirst true, and it will update http.route only once + // using the last portion of the nested path. + // HttpRouteSource.NESTED_CONTROLLER has useFirst false, and it will make http.route updated + // twice: 1st using the last portion of the nested path, 2nd time using the full nested path. HttpRouteHolder.updateHttpRoute( - parentContext, HttpRouteSource.CONTROLLER, httpRouteGetter(), exchange); + parentContext, HttpRouteSource.NESTED_CONTROLLER, httpRouteGetter(), exchange); if (handler == null) { return; diff --git a/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/test/java/server/base/ImmediateHandlerSpringWebFluxServerTest.java b/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/test/java/server/base/ImmediateHandlerSpringWebFluxServerTest.java index 1716e7371db5..1b0ed334f1ab 100644 --- a/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/test/java/server/base/ImmediateHandlerSpringWebFluxServerTest.java +++ b/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/test/java/server/base/ImmediateHandlerSpringWebFluxServerTest.java @@ -5,7 +5,14 @@ package server.base; +import static io.opentelemetry.instrumentation.testing.junit.http.ServerEndpoint.NESTED_PATH; +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assumptions.assumeTrue; + import io.opentelemetry.instrumentation.testing.junit.http.ServerEndpoint; +import io.opentelemetry.testing.internal.armeria.common.AggregatedHttpRequest; +import io.opentelemetry.testing.internal.armeria.common.AggregatedHttpResponse; +import org.junit.jupiter.api.Test; import org.springframework.boot.autoconfigure.EnableAutoConfiguration; import org.springframework.boot.web.embedded.netty.NettyReactiveWebServerFactory; import org.springframework.context.annotation.Bean; @@ -54,4 +61,18 @@ protected Mono wrapResponse( }); } } + + @Test + void nestedPath() { + assumeTrue(Boolean.getBoolean("testLatestDeps")); + + String method = "GET"; + AggregatedHttpRequest request = request(NESTED_PATH, method); + AggregatedHttpResponse response = client.execute(request).aggregate().join(); + assertThat(response.status().code()).isEqualTo(NESTED_PATH.getStatus()); + assertThat(response.contentUtf8()).isEqualTo(NESTED_PATH.getBody()); + assertResponseHasCustomizedHeaders(response, NESTED_PATH, null); + + assertTheTraces(1, null, null, null, method, NESTED_PATH, response); + } } diff --git a/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/test/java/server/base/ServerTestController.java b/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/test/java/server/base/ServerTestController.java index 48af18d0896c..c185dcf260cf 100644 --- a/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/test/java/server/base/ServerTestController.java +++ b/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/test/java/server/base/ServerTestController.java @@ -116,6 +116,18 @@ public Mono capture_headers(ServerHttpRequest request, ServerHttpRespons }); } + @GetMapping("/nestedPath") + public Mono nested_path(ServerHttpRequest request, ServerHttpResponse response) { + ServerEndpoint endpoint = ServerEndpoint.NESTED_PATH; + + return wrapControllerMethod( + endpoint, + () -> { + setStatus(response, endpoint); + return endpoint.getBody(); + }); + } + protected abstract Mono wrapControllerMethod(ServerEndpoint endpoint, Supplier handler); private static void setStatus(ServerHttpResponse response, ServerEndpoint endpoint) { diff --git a/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/test/java/server/base/ServerTestRouteFactory.java b/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/test/java/server/base/ServerTestRouteFactory.java index 5be798362686..5da59d721429 100644 --- a/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/test/java/server/base/ServerTestRouteFactory.java +++ b/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/test/java/server/base/ServerTestRouteFactory.java @@ -6,6 +6,8 @@ package server.base; import static org.springframework.web.reactive.function.server.RequestPredicates.GET; +import static org.springframework.web.reactive.function.server.RequestPredicates.path; +import static org.springframework.web.reactive.function.server.RouterFunctions.nest; import static org.springframework.web.reactive.function.server.RouterFunctions.route; import io.opentelemetry.api.trace.Span; @@ -98,7 +100,17 @@ public RouterFunction createRoutes() { request.headers().asHttpHeaders().getFirst("X-Test-Request")), null, null); - }); + }) + .andNest( + path("/nestedPath"), + nest( + path("/hello"), + route( + path("/world"), + request -> { + ServerEndpoint endpoint = ServerEndpoint.NESTED_PATH; + return respond(endpoint, null, null, null); + }))); } protected Mono respond( diff --git a/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/test/java/server/base/SpringWebFluxServerTest.java b/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/test/java/server/base/SpringWebFluxServerTest.java index 6ceb73ae34be..2dc3847bd8ba 100644 --- a/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/test/java/server/base/SpringWebFluxServerTest.java +++ b/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/test/java/server/base/SpringWebFluxServerTest.java @@ -52,6 +52,8 @@ public String expectedHttpRoute(ServerEndpoint endpoint) { return getContextPath() + "/path/{id}/param"; case NOT_FOUND: return "/**"; + case NESTED_PATH: + return "/nestedPath/hello/world"; default: return super.expectedHttpRoute(endpoint); } diff --git a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpServerTest.java b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpServerTest.java index ef45b10999b1..fe48cdc99db4 100644 --- a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpServerTest.java +++ b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpServerTest.java @@ -94,7 +94,7 @@ public static T controller(ServerEndpoint endpoint, Supplier closure) { return GlobalTraceUtil.runWithSpan("controller", () -> closure.get()); } - private AggregatedHttpRequest request(ServerEndpoint uri, String method) { + protected AggregatedHttpRequest request(ServerEndpoint uri, String method) { return AggregatedHttpRequest.of(HttpMethod.valueOf(method), resolveAddress(uri)); } diff --git a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/ServerEndpoint.java b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/ServerEndpoint.java index bccb9b2e323f..cbe628ce926c 100644 --- a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/ServerEndpoint.java +++ b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/ServerEndpoint.java @@ -19,6 +19,11 @@ public enum ServerEndpoint { EXCEPTION("exception", 500, "controller exception"), NOT_FOUND("notFound", 404, "not found"), CAPTURE_HEADERS("captureHeaders", 200, "headers captured"), + NESTED_PATH( + "nestedPath/hello/world", + 200, + "nested path"), // TODO (heya) move it to webflux test module after this has been converted to + // a class CAPTURE_PARAMETERS("captureParameters", 200, "parameters captured"), // TODO: add tests for the following cases: