Spring Webflux Library Instrumentation by jamesmoessis · Pull Request #7899 · open-telemetry/opentelemetry-java-instrumentation

jamesmoessis · 2023-02-23T22:51:58Z

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

…tation

…garding attribute retrieval

fix markdown link fix formatter change

spotless

trask · 2023-02-27T00:14:32Z

@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 · 2023-02-27T03:42:47Z

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

jamesmoessis · 2023-02-27T06:40:14Z

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

…(java agent still having issues) spotless

jamesmoessis · 2023-02-28T00:53:34Z

@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 · 2023-02-28T01:56:53Z

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

Fix muzzle

jamesmoessis · 2023-02-28T02:48:06Z

Awesome! Thankyou @trask that has done the trick!

laurit · 2023-03-01T16:08:35Z

+      route = bestPatternObj.toString();
+    }
+    if (route.equals("/**")) {
+      return null; // 404


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

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

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.

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.

Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>

…ry/src/main/java/io/opentelemetry/instrumentation/spring/webflux/v5_3/SpringWebfluxTelemetryBuilder.java Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>

mateuszrzeszutek

Thanks @jamesmoessis !

jamesmoessis requested a review from a team February 23, 2023 22:51

github-actions Bot requested a review from theletterf February 23, 2023 22:52

jamesmoessis force-pushed the webflux-library-instrumentation branch 3 times, most recently from 60bebec to bbfa43b Compare February 24, 2023 03:12

jamesmoessis added 12 commits February 27, 2023 10:03

rename SpringWebfluxTelemetry to SpringWebClientTelemetry

e5b170c

update docs

8016887

rename to specify server and prevent name clash with client instrumen…

486334b

…tation

add test (but currently broken with dependency issue)

624038d

change to use ServerWebExchange as generic type, and fix some bugs re…

dd29fda

…garding attribute retrieval

move spring-weflux modules under common folder

fdb5b95

upgrade minimum version and rename packages

238a5f2

tests + fixes to pass HTTP test suite

60d6fcb

move README and make it nicer

eef999a

fix markdown link

7287488

fix markdown link fix formatter change

remove changes no longer needed in 5.0 buildscript

9271ab7

spotless, linter changes + cleanup

d69fd50

spotless

jamesmoessis force-pushed the webflux-library-instrumentation branch from bbfa43b to d69fd50 Compare February 26, 2023 23:03

fix latest dep tests

a0f5064

jamesmoessis commented Feb 27, 2023

View reviewed changes

Comment thread ...entelemetry/instrumentation/spring/webflux/v5_3/server/SpringWebfluxNetAttributesGetter.java Outdated

trask reviewed Feb 27, 2023

View reviewed changes

move 5.0 library module into 5.3

d8d0d71

rearrange so client and server are both under SpringWebfluxTelemetry …

ddd788b

…(java agent still having issues) spotless

jamesmoessis force-pushed the webflux-library-instrumentation branch from eb967d2 to ddd788b Compare February 28, 2023 00:13

Fix muzzle

3647eab

Merge pull request #1 from trask/webflux-library-instrumentation

f329fa4

Fix muzzle

jamesmoessis added 2 commits March 1, 2023 09:18

empty commit to retrigger hung build

6a599c3

Merge branch 'main' into webflux-library-instrumentation

24f0ee8

laurit reviewed Mar 1, 2023

View reviewed changes

Comment thread instrumentation/spring/spring-webflux/spring-webflux-5.3/library/README.md Outdated

laurit reviewed Mar 1, 2023

View reviewed changes

Comment thread ...c/main/java/io/opentelemetry/instrumentation/spring/webflux/v5_3/SpringWebfluxTelemetry.java Outdated

laurit reviewed Mar 1, 2023

View reviewed changes

Comment thread ...a/io/opentelemetry/instrumentation/spring/webflux/v5_3/WebfluxServerNetAttributesGetter.java Outdated

jamesmoessis added 2 commits March 2, 2023 11:06

address some of laurit PR comments

b9ed6ba

return null for getHostName() so the backup mechanism kicks in

2ba2fd8

laurit approved these changes Mar 3, 2023

View reviewed changes

mateuszrzeszutek reviewed Mar 7, 2023

View reviewed changes

Comment thread ...java/io/opentelemetry/instrumentation/spring/webflux/v5_3/SpringWebfluxTelemetryBuilder.java

jamesmoessis and others added 7 commits March 8, 2023 09:27

Merge branch 'main' into webflux-library-instrumentation

128f19d

Commit suggestion to add metrics

f6fc0b5

Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>

add missing import from suggestion

3904f97

Update instrumentation/spring/spring-webflux/spring-webflux-5.3/libra…

5afd4e2

…ry/src/main/java/io/opentelemetry/instrumentation/spring/webflux/v5_3/SpringWebfluxTelemetryBuilder.java Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>

extend InetAddressNetServerAttributesGetter

ba1476b

fix compilation error caused by merging main

6024fe3

use HttpRouteHolder

fb8b630

mateuszrzeszutek approved these changes Mar 8, 2023

View reviewed changes

mateuszrzeszutek merged commit 3f45f75 into open-telemetry:main Mar 8, 2023

jamesmoessis deleted the webflux-library-instrumentation branch March 15, 2023 22:35

Conversation

jamesmoessis commented Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trask commented Feb 27, 2023

Uh oh!

Uh oh!

trask left a comment

Choose a reason for hiding this comment

Uh oh!

jamesmoessis commented Feb 27, 2023

Uh oh!

jamesmoessis commented Feb 27, 2023

Uh oh!

jamesmoessis commented Feb 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trask commented Feb 28, 2023

Uh oh!

jamesmoessis commented Feb 28, 2023

Uh oh!

Uh oh!

Uh oh!

laurit Mar 1, 2023

Choose a reason for hiding this comment

Uh oh!

jamesmoessis Mar 2, 2023

Choose a reason for hiding this comment

Uh oh!

laurit Mar 2, 2023

Choose a reason for hiding this comment

Uh oh!

jamesmoessis Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mateuszrzeszutek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jamesmoessis commented Feb 23, 2023 •

edited

Loading

jamesmoessis commented Feb 28, 2023 •

edited

Loading

jamesmoessis Mar 3, 2023 •

edited

Loading