Issue 614 spring webclient tracing · Pull Request #831 · DataDog/dd-trace-java

ghost · 2019-05-14T13:00:49Z

Enabled tracing for Spring WebClient.

Heavily inspired on Opentracing Spring Web.

TODO:
(-) Rework check to only apply the advice once in DefaultWebClientAdvice.java
(-) Licensing

tylerbenson

This is awesome. Thanks for taking this on. Please see feedback below.
Also, could you add some tests? You can use HttpClientTest as a base (for example), but feel free to add additional tests that might be unique to webflux.

Also, what was your concern about licensing?
Good work!

tylerbenson · 2019-05-14T16:26:54Z

...rc/main/java8/datadog/trace/instrumentation/springwebflux/client/DefaultWebClientAdvice.java

This check is not really needed... we added it to okhttp client because we use that internally to report traces to the agent.

The check for the header is needed because otherwise the advice over and over again and results in a stackoverflow.
It's not very clean so any suggestions for a more subtle solution are more than welcome.

Perhaps you could explain why it's called over and over again? (Maybe in a comment would be good if we can't figure out a better alternative.) I'm afraid I don't really understand the issue.

This advice returns a TracingClientResponseMono which is a decorator around the Mono that is returned from the original ExchangeFunction.exchange().
The TracingClientResponseMono class itself also calls ExchangeFunction.exchange() which triggers this advice again.
If the advice is applied the first time, the x-datadog-trace-id header is added to the client request.
If the advice is applied a second time (checked by looking at the x-datadog-trace-id header on the ClientRequest), it won't decorate the result again otherwise this would lead to a StackOverflowError.

Hope this clarifies the issue. If you have a better solution, please share.

...java/datadog/trace/instrumentation/springwebflux/client/DefaultWebClientInstrumentation.java

...src/main/java8/datadog/trace/instrumentation/springwebflux/client/ClientResponseWrapper.java

...ava8/datadog/trace/instrumentation/springwebflux/client/TracingClientResponseSubscriber.java

tylerbenson · 2019-05-14T16:50:56Z

...ava8/datadog/trace/instrumentation/springwebflux/client/TracingClientResponseSubscriber.java

just out of curiosity, under what scenarios would this be called more than once?

onNext had to be implemented because it's part of the interface of CoreSubscriber.
Could this be called more than once in case of an TEXT_EVENT_STREAM?

...ava8/datadog/trace/instrumentation/springwebflux/client/TracingClientResponseSubscriber.java

...main/java8/datadog/trace/instrumentation/springwebflux/client/TracingClientResponseMono.java

johanvandeweerd · 2019-05-15T12:26:37Z

@tylerbenson I've added SpringWebfluxHttpClientTest based on ApacheHttpClientTest but the test is failing locally. The first test (basic #method request) is expecting 2 traces but got 3 traces and the span the webclient created doesn't seem to have the same trace id. When I used the webclient instrumentation on one of my projects, it did work.
Any suggestions?

About the licensing: I've copied some code from Opentracing Spring Web but I don't know if all conditions are met (License and copyright notice, State changes) and I'm not that familiar with licensing.

...java/datadog/trace/instrumentation/springwebflux/client/DefaultWebClientInstrumentation.java

...src/main/java8/datadog/trace/instrumentation/springwebflux/client/ClientResponseWrapper.java

tylerbenson

A few minor requests, and some suggestions on how to work with the tests.

Nice work. I think we're almost done here.

tylerbenson · 2019-05-15T15:26:52Z

...ava8/datadog/trace/instrumentation/springwebflux/client/TracingClientResponseSubscriber.java

This method is no longer used.

tylerbenson · 2019-05-16T15:16:37Z

...src/main/java8/datadog/trace/instrumentation/springwebflux/client/ClientResponseWrapper.java

typo: rewStatusCode

tylerbenson · 2019-05-16T15:20:08Z

...rc/main/java8/datadog/trace/instrumentation/springwebflux/client/DefaultWebClientAdvice.java

Perhaps you could explain why it's called over and over again? (Maybe in a comment would be good if we can't figure out a better alternative.) I'm afraid I don't really understand the issue.

tylerbenson · 2019-05-16T17:19:25Z

...test/groovy/dd/trace/instrumentation/springwebflux/client/SpringWebfluxHttpClientTest.groovy

Ok, this might be a bit messy, but try adding a setupSpec that disables the netty instrumentation via system property (and maybe retransforms the relevant classes?) and an cleanupSpec that does the opposite (so as to not impact the other tests).

Another idea is to possibly override the clientSpan method, but I'm not positive that would work since it's not abstract.

Other ideas?

Last resort, I guess you could just copy the whole test and I can clean it up later.

tylerbenson · 2019-05-22T00:17:34Z

Do you think you'll be able to make these last few changes, or should I try implementing them?

johanvandeweerd · 2019-05-23T08:50:34Z

I've been rather busy the past week. Will pick this up tomorrow.

tylerbenson · 2019-05-25T03:30:51Z

@johanvandeweerd @labbati I decided to try my suggestion (about overriding the clientSpan method) on a different test with a similar problem: OkHttp. I made progress and was able to demonstrate my ideas for how to solve it, but was not able to finish. You can see my work here: a2537b5

tylerbenson · 2019-05-31T17:39:14Z

That work has been merged into master (#860). You can follow what I did for the OkHttpClientTest.
Thanks for working on this!

tylerbenson · 2019-06-14T18:20:43Z

I looked into this a bit and there seems to be an issue with the netty client span not being linked to the parent span correctly. I'm not exactly sure where it's breaking, but my guess is in the async reactor call.

I don't know if it will help, but here's an example of how I instrumented rxjava in hystrix: https://github.com/DataDog/dd-trace-java/blob/master/dd-java-agent/instrumentation/hystrix-1.4/src/main/java/datadog/trace/instrumentation/hystrix/HystrixInstrumentation.java#L99

Let us know if there's anything else we can help with.

tylerbenson · 2019-07-16T02:34:02Z

@johanvandeweerd I think I have a fix for this, but it will depend on #917 and #886. Can you give me push access to your repo and I can finish updating this?

johanvandeweerd · 2019-07-16T08:02:52Z

@johanvandeweerd I think I have a fix for this, but it will depend on #917 and #886. Can you give me push access to your repo and I can finish updating this?

Done.

And sorry for the lack of effort lately.

… and SpringWebfluxHttpClientDecorator

…unctions$DefaultExchangeFunction

…e in spring-webflux-5.0

…plied

tylerbenson

@johanvandeweerd want to take a look at my changes to make sure I didn't miss anything?

I'd also like a second review from @labbati @mar-kolya or @randomanderson, but looks good to me now.

randomanderson

Looks good. The only slightly weird thing is that the advice needs to prevent an infinite loop and I see that was already discussed.

I wasn't sure stylistically about saving Tracer to a field vs using GlobalTracer.get() everywhere but I checked and it's not uncommon.

johanvandeweerd · 2019-07-23T15:07:48Z

@tylerbenson I've locally build the dd-java-agent and tested in a small demo application. Everything looks good!
Any idea when this will be GA?
Tnx for the effort and work you put into this.

tylerbenson · 2019-07-23T16:34:38Z

@johanvandeweerd Thanks to you too for getting it started. You did most of the work.

It will be included in the next release, likely this week or next.

ghost mentioned this pull request May 14, 2019

Spring WebClient tracing issue #614

Closed

tylerbenson suggested changes May 14, 2019

View reviewed changes

tylerbenson reviewed May 15, 2019

View reviewed changes

...java/datadog/trace/instrumentation/springwebflux/client/DefaultWebClientInstrumentation.java Outdated Show resolved Hide resolved

...src/main/java8/datadog/trace/instrumentation/springwebflux/client/ClientResponseWrapper.java Outdated Show resolved Hide resolved

tylerbenson reviewed May 16, 2019

View reviewed changes

tylerbenson added this to the 0.28.0 milestone May 16, 2019

labbati modified the milestones: 0.28.0, 0.29.0 May 17, 2019

tylerbenson removed this from the 0.29.0 milestone May 29, 2019

ghost self-requested a review as a code owner June 7, 2019 10:51

johanvandeweerd force-pushed the issue-614-spring-webclient-tracing branch from 3275ba7 to 27a0749 Compare June 7, 2019 11:05

johanvandeweerd force-pushed the issue-614-spring-webclient-tracing branch from 27a0749 to b348f51 Compare July 16, 2019 08:01

Johan Vandeweerd added 11 commits July 22, 2019 11:01

Moved Spring Webflux instrumentation code to server subpackage

797003b

Enable tracing for Spring Webflux WebClient

091648d

Aligned instrumentation names between DefaultWebClientInstrumentation…

0e9edb1

… and SpringWebfluxHttpClientDecorator

Instrument all subtypes of ExchangeFunction instead of only ExchangeF…

e4560ba

…unctions$DefaultExchangeFunction

Add javadoc to clarify usage of this class

f5f8fe1

Replace span.log with span.tag

35b924b

Replace operation name with static value

ac11bba

Add decorator hooks during lifecycle of Publisher

23b8caa

Remove onPeerConnection decorator call

872a099

Add test for spring webflux webclient

7059cca

Add Javadoc to rawStatusCode method that is not part of ClientRespons…

8f96205

…e in spring-webflux-5.0

Johan Vandeweerd and others added 5 commits July 22, 2019 11:01

Replace hasSuperType with safeHasSuperType

bc51f04

Fix typo rawStatusCode in javadoc

633db43

Add documentation on conditions when DefaultWebClientAdvice is not ap…

0043efa

…plied

Remove unused private method

fff8006

Fix trace propagation

aaba3fc

tylerbenson force-pushed the issue-614-spring-webclient-tracing branch from b348f51 to aaba3fc Compare July 22, 2019 18:20

tylerbenson added this to the 0.31.0 milestone Jul 22, 2019

tylerbenson approved these changes Jul 22, 2019

View reviewed changes

randomanderson approved these changes Jul 22, 2019

View reviewed changes

tylerbenson merged commit 0a79235 into DataDog:master Jul 23, 2019

tylerbenson deleted the issue-614-spring-webclient-tracing branch July 23, 2019 14:50

Conversation

ghost commented May 14, 2019

Uh oh!

tylerbenson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johanvandeweerd Jun 7, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

johanvandeweerd commented May 15, 2019

Uh oh!

Uh oh!

Uh oh!

tylerbenson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tylerbenson commented May 22, 2019

Uh oh!

johanvandeweerd commented May 23, 2019

Uh oh!

tylerbenson commented May 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tylerbenson commented May 31, 2019

Uh oh!

tylerbenson commented Jun 14, 2019

Uh oh!

tylerbenson commented Jul 16, 2019

Uh oh!

johanvandeweerd commented Jul 16, 2019

Uh oh!

tylerbenson left a comment

Choose a reason for hiding this comment

Uh oh!

randomanderson left a comment

Choose a reason for hiding this comment

Uh oh!

johanvandeweerd commented Jul 23, 2019

Uh oh!

tylerbenson commented Jul 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

johanvandeweerd Jun 7, 2019 •

edited

Loading

tylerbenson commented May 25, 2019 •

edited

Loading