3143 add support for vertx web 4 #3557

LuboViluda · 2022-05-16T10:34:18Z

What Does This Do

providing support for Vert.x web 4
Vert.x HttpServerResponseImpl class was replaced between versions 3 and 4. Reflect changes in instrumentation.
Migrate other parts of code to Vert.x 4
Migrate all Vert.x 3 tests to 4.

Motivation

provide instrumentation for Vert.x 4 (currently broken)

Additional Notes / open questions

main instrumentation related change is in HttpServerResponseEndHandlerInstrumentation class when the change in underlying Vert.x classes is reflected
some test fails currently:

./gradlew :dd-java-agent:instrumentation:vertx-web-4.0:test
136 tests completed, 90 failed, 23 skipped

What is a minimal test suite to get this merged? I couldn't really see from run of Vert.x vertx-3.4 tests what is the number of successful tests there as the final output doesn't provide this (the immediate step before the success message is showing around 130). But some constructs seem suspicious for Vert.x 3 test suite (fe. setting Vert.x clustering port for local test server - but maybe I'm lacking some internal Vert.x knowledge, why this is there)

devinsba · 2022-05-17T14:14:16Z

Thanks @LuboViluda, I'm going to try to review this this week but unfortunately the vertx integrations are large so it may take a while

devinsba

The version numbers will need to be addressed. Then just the one question

...adog/trace/instrumentation/vertx_4_0/server/HttpServerResponseEndHandlerInstrumentation.java

...rc/main/java/datadog/trace/instrumentation/vertx_4_0/server/RouteHandlerInstrumentation.java

...0/src/main/java/datadog/trace/instrumentation/vertx_4_0/server/RouteImplInstrumentation.java

...n/java/datadog/trace/instrumentation/vertx_4_0/server/RoutingContextImplInstrumentation.java

...-agent/instrumentation/vertx-web-4.0/src/test/groovy/server/VertxHttpServerForkedTest.groovy

dd-java-agent/instrumentation/vertx-web-4.0/vertx-web-4.0.gradle

devinsba

This will probably fix the tests

...-agent/instrumentation/vertx-web-4.0/src/test/groovy/server/VertxHttpServerForkedTest.groovy

devinsba · 2022-05-22T14:21:27Z

You can see the failing tests here: https://app.circleci.com/pipelines/github/DataDog/dd-trace-java/13590/workflows/64d950e9-c130-4d62-83b3-cecc8415eb39/jobs/298083/tests

Don't worry about the "instrumentation gateway" ones for now. The others like like they might just be a slight difference in how things were implemented

@ValentinZakharov can you see if there's an obvious reason that the instrumentation gateway tests failed?

LuboViluda · 2022-05-22T16:32:42Z

You can see the failing tests here: https://app.circleci.com/pipelines/github/DataDog/dd-trace-java/13590/workflows/64d950e9-c130-4d62-83b3-cecc8415eb39/jobs/298083/tests

Don't worry about the "instrumentation gateway" ones for now. The others like like they might just be a slight difference in how things were implemented

@ValentinZakharov can you see if there's an obvious reason that the instrumentation gateway tests failed?

Thanks, double "/" in span resourceName is probably cause for a lot of failure, not sure if some toString/reporting Vert.x. method changed and broke this
if I find what change, what is the simplest path to fix this? (fe. implement some interceptor?)

assert span.resourceName.toString() == name
       |    |            |          |  |
       |    |            |          |  'GET /sub/success'
       |    |            |          false
       |    |            'GET /sub//success'
       |    GET /sub//success

from server.VertxSubrouterForkedTest

LuboViluda · 2022-05-24T13:41:50Z

Hello devinsba,

I have committed change for correct path concatenation (deduplication of "//" to "/"). It fixed a nice number of tests. Fe. z_test_8_inst went from 32 failed tests to 4 failed tests (3 mentioning Gateway directly). Any idea how to fix Gateway related tests?

...n/java/datadog/trace/instrumentation/vertx_4_0/server/RoutingContextImplInstrumentation.java

bantonsson · 2022-05-25T12:59:13Z

dd-java-agent/instrumentation/vertx-web-4.0/vertx-web-4.0.gradle

+ext {
+  minJavaVersionForTests = JavaVersion.VERSION_1_8
+  // TODO Java 17: This version of vertx-web doesn't support Java 17
+  maxJavaVersionForTests = JavaVersion.VERSION_15


Is this true, or does this version work on Java 17?

seems Java 17 is supported by Vertx web 4.2 and above

https://vertx.io/blog/whats-new-in-vert-x-4-2/#java-17

...src/main/java8/datadog/trace/instrumentation/vertx_4_0/server/RouteHandlerWrapperAdvice.java

LuboViluda · 2022-05-31T12:29:45Z

@bantonsson @devinsba
tests are passing now, thanks for all the suggestions. I fixed the static analysis check as well (was failing on usage String.replaceAll).

Do you have any suggestions for failing the system-tests and muzzle build step?

devinsba · 2022-06-01T15:26:13Z

You can ignore the system-tests but we do need to fix muzzle. What the failure seems to be telling us is that one of the instrumentation classes works for more versions than the range [4.0.0,5)

> Task :dd-java-agent:instrumentation:vertx-web-4.0:muzzle-AssertFail-io.vertx-vertx-web-3.4.2 FAILED
MUZZLE PASSED HttpServerResponseEndHandlerInstrumentation BUT FAILURE WAS EXPECTED

> Task :dd-java-agent:instrumentation:vertx-web-4.0:muzzle-AssertFail-io.vertx-vertx-web-3.8.5 FAILED
MUZZLE PASSED HttpServerResponseEndHandlerInstrumentation BUT FAILURE WAS EXPECTED

> Task :dd-java-agent:instrumentation:vertx-web-4.0:muzzle-AssertFail-io.vertx-vertx-web-3.9.13 FAILED
MUZZLE PASSED HttpServerResponseEndHandlerInstrumentation BUT FAILURE WAS EXPECTED

> Task :dd-java-agent:instrumentation:vertx-web-4.0:muzzle-AssertFail-io.vertx-vertx-web-3.9.0 FAILED
MUZZLE PASSED HttpServerResponseEndHandlerInstrumentation BUT FAILURE WAS EXPECTED

> Task :dd-java-agent:instrumentation:vertx-web-4.0:muzzle-AssertFail-io.vertx-vertx-web-3.5.0 FAILED
MUZZLE PASSED HttpServerResponseEndHandlerInstrumentation BUT FAILURE WAS EXPECTED

> Task :dd-java-agent:instrumentation:vertx-web-4.0:muzzle-AssertFail-io.vertx-vertx-web-3.7.0 FAILED
MUZZLE PASSED HttpServerResponseEndHandlerInstrumentation BUT FAILURE WAS EXPECTED

> Task :dd-java-agent:instrumentation:vertx-web-4.0:muzzle-AssertFail-io.vertx-vertx-web-3.7.1 FAILED
MUZZLE PASSED HttpServerResponseEndHandlerInstrumentation BUT FAILURE WAS EXPECTED

> Task :dd-java-agent:instrumentation:vertx-web-4.0:muzzle-AssertFail-io.vertx-vertx-web-3.5.4 FAILED
MUZZLE PASSED HttpServerResponseEndHandlerInstrumentation BUT FAILURE WAS EXPECTED

> Task :dd-java-agent:instrumentation:vertx-web-4.0:muzzle-AssertFail-io.vertx-vertx-web-3.4.0 FAILED
MUZZLE PASSED HttpServerResponseEndHandlerInstrumentation BUT FAILURE WAS EXPECTED

> Task :dd-java-agent:instrumentation:vertx-web-4.0:muzzle-AssertFail-io.vertx-vertx-web-3.6.0 FAILED
MUZZLE PASSED HttpServerResponseEndHandlerInstrumentation BUT FAILURE WAS EXPECTED

> Task :dd-java-agent:instrumentation:vertx-web-4.0:muzzle-AssertFail-io.vertx-vertx-web-3.6.3 FAILED
MUZZLE PASSED HttpServerResponseEndHandlerInstrumentation BUT FAILURE WAS EXPECTED

> Task :dd-java-agent:instrumentation:vertx-web-4.0:muzzle-AssertFail-io.vertx-vertx-web-3.8.0 FAILED
MUZZLE PASSED HttpServerResponseEndHandlerInstrumentation BUT FAILURE WAS EXPECTED

We usually fix this by adding a method to the advice class that depends on something that was added in the lowest version, see: https://github.com/DataDog/dd-trace-java/blob/master/dd-java-agent/instrumentation/jetty-9/src/main/java/datadog/trace/instrumentation/jetty9/JettyServerInstrumentation.java#L147

LuboViluda · 2022-06-07T12:25:34Z

You can ignore the system-tests but we do need to fix muzzle. What the failure seems to be telling us is that one of the instrumentation classes works for more versions than the range [4.0.0,5)

> Task :dd-java-agent:instrumentation:vertx-web-4.0:muzzle-AssertFail-io.vertx-vertx-web-3.4.2 FAILED
MUZZLE PASSED HttpServerResponseEndHandlerInstrumentation BUT FAILURE WAS EXPECTED

> Task :dd-java-agent:instrumentation:vertx-web-4.0:muzzle-AssertFail-io.vertx-vertx-web-3.8.5 FAILED
MUZZLE PASSED HttpServerResponseEndHandlerInstrumentation BUT FAILURE WAS EXPECTED

> Task :dd-java-agent:instrumentation:vertx-web-4.0:muzzle-AssertFail-io.vertx-vertx-web-3.9.13 FAILED
MUZZLE PASSED HttpServerResponseEndHandlerInstrumentation BUT FAILURE WAS EXPECTED

> Task :dd-java-agent:instrumentation:vertx-web-4.0:muzzle-AssertFail-io.vertx-vertx-web-3.9.0 FAILED
MUZZLE PASSED HttpServerResponseEndHandlerInstrumentation BUT FAILURE WAS EXPECTED

> Task :dd-java-agent:instrumentation:vertx-web-4.0:muzzle-AssertFail-io.vertx-vertx-web-3.5.0 FAILED
MUZZLE PASSED HttpServerResponseEndHandlerInstrumentation BUT FAILURE WAS EXPECTED

> Task :dd-java-agent:instrumentation:vertx-web-4.0:muzzle-AssertFail-io.vertx-vertx-web-3.7.0 FAILED
MUZZLE PASSED HttpServerResponseEndHandlerInstrumentation BUT FAILURE WAS EXPECTED

> Task :dd-java-agent:instrumentation:vertx-web-4.0:muzzle-AssertFail-io.vertx-vertx-web-3.7.1 FAILED
MUZZLE PASSED HttpServerResponseEndHandlerInstrumentation BUT FAILURE WAS EXPECTED

> Task :dd-java-agent:instrumentation:vertx-web-4.0:muzzle-AssertFail-io.vertx-vertx-web-3.5.4 FAILED
MUZZLE PASSED HttpServerResponseEndHandlerInstrumentation BUT FAILURE WAS EXPECTED

> Task :dd-java-agent:instrumentation:vertx-web-4.0:muzzle-AssertFail-io.vertx-vertx-web-3.4.0 FAILED
MUZZLE PASSED HttpServerResponseEndHandlerInstrumentation BUT FAILURE WAS EXPECTED

> Task :dd-java-agent:instrumentation:vertx-web-4.0:muzzle-AssertFail-io.vertx-vertx-web-3.6.0 FAILED
MUZZLE PASSED HttpServerResponseEndHandlerInstrumentation BUT FAILURE WAS EXPECTED

> Task :dd-java-agent:instrumentation:vertx-web-4.0:muzzle-AssertFail-io.vertx-vertx-web-3.6.3 FAILED
MUZZLE PASSED HttpServerResponseEndHandlerInstrumentation BUT FAILURE WAS EXPECTED

> Task :dd-java-agent:instrumentation:vertx-web-4.0:muzzle-AssertFail-io.vertx-vertx-web-3.8.0 FAILED
MUZZLE PASSED HttpServerResponseEndHandlerInstrumentation BUT FAILURE WAS EXPECTED

We usually fix this by adding a method to the advice class that depends on something that was added in the lowest version, see: https://github.com/DataDog/dd-trace-java/blob/master/dd-java-agent/instrumentation/jetty-9/src/main/java/datadog/trace/instrumentation/jetty9/JettyServerInstrumentation.java#L147

Thanks for the suggestion @devinsba. I have already bound instrumentation on the class which was added in Vert.x 4 (similar as Vertx 3 instrumentation is doing it) in VertxVersionMatcher. The class should be in Vertx 4, but not in the Vertx 3. I will take another look, at if I ported changes correctly and what possibly could go wrong (maybe the class exist on other path in Vert.x 3.x)

devinsba · 2022-06-08T14:18:28Z

I have a fix for muzzle if you can allow me push access to the branch: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork#enabling-repository-maintainer-permissions-on-existing-pull-requests

...eb-4.0/src/main/java/datadog/trace/instrumentation/vertx_4_0/server/VertxVersionMatcher.java

...0/src/main/java/datadog/trace/instrumentation/vertx_4_0/server/RouteImplInstrumentation.java

...n/java/datadog/trace/instrumentation/vertx_4_0/server/RoutingContextImplInstrumentation.java

LuboViluda · 2022-06-08T17:46:44Z

I have a fix for muzzle if you can allow me push access to the branch: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork#enabling-repository-maintainer-permissions-on-existing-pull-requests

It's seems it's on by default.

devinsba · 2022-06-08T17:56:47Z

Ah, thanks. My git config was wrong

… fix)

…rver/VertxHttpServerForkedTest.groovy Co-authored-by: Brian Devins-Suresh <badevins@gmail.com>

…nsaformations reflect Vertx method signature changes

…nsaformations reflect Vertx method signature changes (+ code reformating)

…RouterImpl

devinsba

LGTM. Thanks!

LuboViluda requested a review from a team as a code owner May 16, 2022 10:34

LuboViluda mentioned this pull request May 16, 2022

Add support for Vert.x 4.x #3143

Closed

devinsba suggested changes May 17, 2022

View reviewed changes

devinsba suggested changes May 20, 2022

View reviewed changes

...-agent/instrumentation/vertx-web-4.0/src/test/groovy/server/VertxHttpServerForkedTest.groovy Show resolved Hide resolved

bantonsson reviewed May 24, 2022

View reviewed changes

...n/java/datadog/trace/instrumentation/vertx_4_0/server/RoutingContextImplInstrumentation.java Show resolved Hide resolved

bantonsson reviewed May 25, 2022

View reviewed changes

...src/main/java8/datadog/trace/instrumentation/vertx_4_0/server/RouteHandlerWrapperAdvice.java Outdated Show resolved Hide resolved

mcculls reviewed Jun 8, 2022

View reviewed changes

...eb-4.0/src/main/java/datadog/trace/instrumentation/vertx_4_0/server/VertxVersionMatcher.java Show resolved Hide resolved

mcculls reviewed Jun 8, 2022

View reviewed changes

...0/src/main/java/datadog/trace/instrumentation/vertx_4_0/server/RouteImplInstrumentation.java Show resolved Hide resolved

mcculls reviewed Jun 8, 2022

View reviewed changes

...n/java/datadog/trace/instrumentation/vertx_4_0/server/RoutingContextImplInstrumentation.java Show resolved Hide resolved

lubomir,viluda and others added 10 commits June 8, 2022 14:08

3443 - add instrumentation support for Vertx web 4.0 and above

c587ead

3443 - add instrumentation support for Vertx web 4.0 and above (build…

a7b22e7

… fix)

3443 - reformat code

fec2180

3443 - fix tests

164732b

3443 - test improvements

c832c1f

3443 - use 0 port

ffa072a

3443 - change versioning to reflect minimal supported version (4.0.0)

5ea0cd2

3443 - deduplicate backslash when concatenating router path and path

be42ee9

Update dd-java-agent/instrumentation/vertx-web-4.0/src/test/groovy/se…

3d1c4d5

…rver/VertxHttpServerForkedTest.groovy Co-authored-by: Brian Devins-Suresh <badevins@gmail.com>

3443 - apply suggestion - RoutingContextImplInstrumentation adviceTra…

43df101

…nsaformations reflect Vertx method signature changes

lubomir,viluda and others added 5 commits June 8, 2022 14:08

3443 - apply suggestion - RoutingContextImplInstrumentation adviceTra…

bc86958

…nsaformations reflect Vertx method signature changes (+ code reformating)

3443 - fix build missing import (:

ce79aa0

3443 - RouteHandlerWrapperAdvice wrap hander on RouteImpl instead of …

262c765

…RouterImpl

3443 - remove replaceAll usage (fix static analysis check)

38c8b7b

Add muzzle check to other instrumentation classes

662b63f

devinsba force-pushed the 3143-addSupportFor-vertx-web-4 branch from 1f87744 to 662b63f Compare June 8, 2022 18:10

devinsba added inst: others All other instrumentations tag: community Community contribution inst: vertx Eclipse Vert.x instrumentation labels Jun 8, 2022

devinsba approved these changes Jun 9, 2022

View reviewed changes

devinsba merged commit 5e25caf into DataDog:master Jun 14, 2022

devinsba added this to the 0.103.0 milestone Jun 14, 2022

akleiman mentioned this pull request Apr 26, 2023

Add vert.x 4.x to Java compatibility documentation #5126

Closed

bm1549 mentioned this pull request Oct 24, 2023

Update supported vertx versions DataDog/documentation#20345

Merged

1 task

3143 add support for vertx web 4 #3557

3143 add support for vertx web 4 #3557

Uh oh!

Conversation

LuboViluda commented May 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What Does This Do

Motivation

Additional Notes / open questions

Uh oh!

devinsba commented May 17, 2022

Uh oh!

devinsba left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

devinsba left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

devinsba commented May 22, 2022

Uh oh!

LuboViluda commented May 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LuboViluda commented May 24, 2022

Uh oh!

Uh oh!

bantonsson May 25, 2022

Choose a reason for hiding this comment

Uh oh!

LuboViluda May 31, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LuboViluda commented May 31, 2022

Uh oh!

devinsba commented Jun 1, 2022

Uh oh!

LuboViluda commented Jun 7, 2022

Uh oh!

devinsba commented Jun 8, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LuboViluda commented Jun 8, 2022

Uh oh!

devinsba commented Jun 8, 2022

Uh oh!

devinsba left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

LuboViluda commented May 16, 2022 •

edited

Loading

LuboViluda commented May 22, 2022 •

edited

Loading