Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix nested http.route #8282

Merged

Conversation

heyams
Copy link
Contributor

@heyams heyams commented Apr 12, 2023

this will replace #8112

I will try my best to explain the fix:

CONTROLLER(3),
  // Some frameworks, e.g. JaxRS, allow for nested controller/paths and we want to select the
  // longest one
NESTED_CONTROLLER(4, false);

HttpRouteHolder.updateHttpRoute will get called twice.
1st is triggered by the RateOnSuccess with route /{id:[A-Z0-9\-]+}
2nd time is triggered by the HandlerAdapterInstrumentation.onMethodEnter with the full route including the nested paths.

When using HttpRouteSource.CONTROLLER, it has useFirst set to true.
2nd time didn't do anything.

When using 'HttpRouteSource.NESTED_CONTROLLER, it has useFirst set to false.
2nd time it will update the route to /api/accounts/{id:[A-Z|0-9|\-]+}.

I will try to come up with a test for it if it's feasible.

here is the test app https://github.com/heyams/otel-http-route-sample

@heyams heyams marked this pull request as ready for review April 12, 2023 21:23
@heyams heyams requested a review from a team April 12, 2023 21:23
@heyams heyams marked this pull request as draft April 13, 2023 16:23
@heyams
Copy link
Contributor Author

heyams commented Apr 14, 2023

WebfluxSingletons.httpRoutGetter

the newly added test will fail because ServerWebExchange.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE) returns null, however ServerWebExchange.getAttribute("org.springframework.web.reactive.function.server.RouterFunctions.request has the full nested route.

BEST_MATCHING_PATTERN_ATTRIBUTE is automatically ingested by webflux. I wonder if someone can help me take a look at why BEST_MATCHING_PATTERN_ATTRIBUTE is always null in this case. Http.route will get updated correctly if this attribute is not null using NESTED_CONTROLLER.

self-resolved :)

@heyams heyams marked this pull request as ready for review April 19, 2023 19:15
Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx for figuring out how to add a test for this!

Comment on lines +25 to +26
"nested path"), // TODO (heya) move it to webflux test module after this has been converted to
// a class
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -66,7 +66,7 @@ public static void methodEnter(
Context parentContext = Context.current();

HttpRouteHolder.updateHttpRoute(
parentContext, HttpRouteSource.CONTROLLER, httpRouteGetter(), exchange);
parentContext, HttpRouteSource.NESTED_CONTROLLER, httpRouteGetter(), exchange);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a brief comment here about why NESTED_CONTROLLER is needed?

@breedx-splk
Copy link
Contributor

I haven't looked into the test code, but there was a single test failure which suggests an edge case maybe:

> Task :instrumentation:spring:spring-webflux:spring-webflux-5.0:javaagent:test
DelayedControllerSpringWebFluxServerTest > nestedPath() FAILED
    org.opentest4j.AssertionFailedError: 
    expected: 200
     but was: 404

@heyams
Copy link
Contributor Author

heyams commented Apr 20, 2023

I haven't looked into the test code, but there was a single test failure which suggests an edge case maybe:

> Task :instrumentation:spring:spring-webflux:spring-webflux-5.0:javaagent:test
DelayedControllerSpringWebFluxServerTest > nestedPath() FAILED
    org.opentest4j.AssertionFailedError: 
    expected: 200
     but was: 404

Thanks! It should be fixed now.

@mateuszrzeszutek mateuszrzeszutek merged commit ffb63d6 into open-telemetry:main Apr 21, 2023
@mateuszrzeszutek
Copy link
Member

Thanks @heyams !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants