Skip to content

Vert.x WebClient instrumentation#1824

Merged
eyalkoren merged 15 commits intoelastic:masterfrom
AlexanderWert:vertx-http-client
Jun 10, 2021
Merged

Vert.x WebClient instrumentation#1824
eyalkoren merged 15 commits intoelastic:masterfrom
AlexanderWert:vertx-http-client

Conversation

@AlexanderWert
Copy link
Member

@AlexanderWert AlexanderWert commented May 22, 2021

Extended Vert.x instrumentation with instrumentation for Vert.x Web Client.

Notes:

  • Didn't use the tracer approach for version 4 as this does not allow to handle instrumentation of redirects properly.

  • Instrumentation approach is similar for all versions

  • had to fix Vert.x core instrumentation for setTimer context propagation: avoid instrumentation of directly recursive setTimer invocations (as this is a common pattern for endless checks)

  • Added to CHANGELOG

  • Added to supported technologies page

@AlexanderWert AlexanderWert added this to the 7.14 milestone May 22, 2021
@ghost
Copy link

ghost commented May 22, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #1824 updated

  • Start Time: 2021-06-09T13:36:14.935+0000

  • Duration: 54 min 14 sec

  • Commit: 8afd326

Test stats 🧪

Test Results
Failed 0
Passed 2245
Skipped 18
Total 2263

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 2245
Skipped 18
Total 2263

Copy link
Contributor

@eyalkoren eyalkoren left a comment

Choose a reason for hiding this comment

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

I will take over from here, thanks @AlexanderWert!

Copy link
Contributor

@eyalkoren eyalkoren left a comment

Choose a reason for hiding this comment

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

@AlexanderWert it is ready as far as I am concerned.
Please go over the changes I've made to make sure they make sense.

I fixed the fail() API instrumentation (decrementing references for the parent span so that it can be reported) and added a test. In addition, I made sure that we at least capture an error for this scenario. Currently, a span is reported with all the details for failed requests only in Vert.x 3. We can add this capability to Vert.x 4 as well, but it will be messy - we will need to rely on internal fields for that (see details) so I decided an error will do for now 🙂 .

@AlexanderWert
Copy link
Member Author

@eyalkoren: LGTM, I'm fine with merging!

@eyalkoren eyalkoren merged commit 72ffb62 into elastic:master Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

2 participants