spring-cloud-gateway support for spring boot 4#15540
spring-cloud-gateway support for spring boot 4#15540laurit merged 11 commits intoopen-telemetry:mainfrom
Conversation
| @Override | ||
| public ElementMatcher<ClassLoader> classLoaderOptimization() { | ||
| return hasClassesNamed( | ||
| "org.springframework.cloud.gateway.server.mvc.handler.GatewayDelegatingRouterFunction"); | ||
| } |
There was a problem hiding this comment.
this is not needed when matching classes based on just name
| } | ||
|
|
||
| try { | ||
| Field routeIdField = gatewayRouterFunction.getClass().getDeclaredField("routeId"); |
There was a problem hiding this comment.
usually when we use reflection we try to do the reflective lookup only once and reuse the Field/Method. Isn't the gatewayRouterFunction here always GatewayDelegatingRouterFunction? If it is you could look up the field in the static initializer of the class. An other option would be to instrument that class and copy route value into a virtual field that you could read here.
|
|
||
| Optional<Object> requestUrlOpt = request.attribute(MvcUtils.GATEWAY_REQUEST_URL_ATTR); | ||
| requestUrlOpt.ifPresent( | ||
| uri -> serverSpan.setAttribute(ROUTE_URI_ATTRIBUTE, ((URI) uri).toASCIIString())); |
There was a problem hiding this comment.
any reason you didn't wish to just use the toString() of the uri?
There was a problem hiding this comment.
no good reason, updated thanks
|
|
||
| @Test | ||
| void gatewayRouteMappingTest() { | ||
| testGatewayRouteMapping(); |
There was a problem hiding this comment.
any reason you didn't add @Test to the super class method?
There was a problem hiding this comment.
at one point i was overriding the method but thats no longer needed, thanks
| compileOnly("io.opentelemetry:opentelemetry-api") | ||
| compileOnly("io.opentelemetry:opentelemetry-api-incubator") |
There was a problem hiding this comment.
aren't these already available through otel.javaagent-instrumentation?
3 participants
Related to #14906
The spring-cloud-starter-gateway artifact split into two:
When I tried to update the muzzle config to include both new artifacts, I hit some muzzle errors, so I split out webmvc into its own module. I also noticed that we didn't have test coverage for webmvc, so I added some, and realized that it wasn't working so I added some new instrumentation. If we would prefer that I undo all of that and handle it in a separate PR, just let me know.