Spring Webflux Library Instrumentation

[jamesmoessis]

Resolves #7436

• Created new Module spring-webflux-5.3 which contains only server-side library instrumentation
• Minimum supported version is 5.3 because there are various problems in older versions of reactor and webflux that prevent a few of the tests from passing.
• Moved existing spring-webflux-5.0 (webclient instrumentation) into a common spring-webflux folder next to the 5.3 (server) instrumentation. Moved the README to the parent folder so the docs are cohesive between client/server instrumentation.
• Implemented WebFilter which instruments the server-side
• Depends on the reactor-3.1 instrumentation to pass the context around. Registers the react hook when it creates the WebFilter
• Tests using the standard HTTP server test suite

[trask]

@jamesmoessis it looks like there's a failure with testLatestDeps which runs the tests against the latest version of webflus. you should be able to reproduce locally using:

./gradlew :instrumentation:spring:spring-webflux:spring-webflux-5.3:library:test -PtestLatestDeps=true

[trask]

I think it would be nice to have just a single spring-webflux library instrumentation module (check out the gRPC library instrumentation for *Telemetry class pattern with both client and server library instrumentation)

If you move the client library instrumentation (and the associated shared testing module) over into the new 5.3 module, I think you should still be able to depend on it from the 5.0 javaagent instrumentation.

"muzzle" on the 5.0 javaagent instrumentation will identify which parts of the 5.3 library module are used inside of the javaagent instrumentation and will only inject those parts.

and the "muzzle" build time check on the 5.0 javaagent instrumentation will keep us from accidentally depending on newer webclient apis

(and if we do want to depend on newer webclient apis in the library instrumentation, we can do a big copy-paste, but hopefully still keep the shared testing classes)

[jamesmoessis]

Thanks @trask, I'll have a go at putting it all in the 5.3 module.

[jamesmoessis]

Moved some things around but still need to put it all in the one SpringWebfluxTelemetry. Will push the rest up tomorrow

[jamesmoessis]

@trask seems Muzzle is having an issue with the new setup. I've pushed it as my latest commit too if you want to see the whole run.

> Task :instrumentation:spring:spring-webflux:spring-webflux-5.0:javaagent:muzzle-AssertPass-io.projectreactor.ipc-reactor-netty-0.7.9.RELEASEipc-0-7-0-with-spring-webflux-5-0-0 FAILED
FAILED MUZZLE VALIDATION: io.opentelemetry.javaagent.instrumentation.spring.webflux.v5_0.client.WebfluxClientInstrumentationModule mismatches:
-- io.opentelemetry.javaagent.shaded.instrumentation.spring.webflux.v5_3.WebfluxServerHttpAttributesGetter:35 Missing method org.springframework.http.server.reactive.ServerHttpResponse#getRawStatusCode()Ljava/lang/Integer;

getRawStatusCode() was introduced in Spring 5.2.4 which explains it not being available for the 5.0 instrumentation. However, it's not called at runtime by the agent, as only the server library instrumentation uses it.

Not too familiar with Muzzle, but I am guessing it doesn't ignore it since the server instrumenter is built (but not used) as part of SpringWebfluxTelemetry instantiation. Is there some way to make Muzzle ignore that symbol specifically, or another way around it?

[trask]

the muzzle static analysis detects the call from WebClientHelper -> SpringWebfluxTelemetryBuilder.build(), which uses WebfluxServerHttpAttributesGetter, and I'm not sure if that's enough, or also factor in that spring webflux javaagent instrumentation also makes calls to (its own) HttpServerAttributesHttpGetter, and 💣

I sent atlassian-forks#1 which extracts out a separate (internal) "client builder" that can be used by the javaagent without triggering the problem

[jamesmoessis]

Awesome! Thankyou @trask that has done the trick!

[laurit]

with spring boot /** doesn't always mean 404 but rather the default route that serves static resources like images etc.

[jamesmoessis]

Ah I see. I wasn't aware of that. Would you be happy if I removed the comment //404 and kept the same behaviour?

[laurit]

That would be nice. Ideally we should have consistent handling for /** if we are ignoring it here we should also ignore it in other spring instrumentations.

[jamesmoessis]

Ok, I removed the comment.

The MVC instrumentation is a bit more sophisticated than this when it comes to determining the if there's no handler mappable to the request. Looks like the Filter has access to the application context so it can retrieve the DispatcherHandler bean and therefore query the HandlerMapping to see if there's a handler for the given request (and if there isn't returns null for route). As a result it doesn't need to handle /** specifically.

From what I see, there's no easy equivalent way to get the HandlerMappings in Webflux, since it's not a given that the WebFilter can access the application context(?). It appears that the current method works fine and is less complex, so unsure if there should be any action here to align the two implementations.

[mateuszrzeszutek]

Thanks @jamesmoessis !