Add Spring WebFlux and WebClient tracing. by csabakos · Pull Request #95 · opentracing-contrib/java-spring-web

csabakos · 2019-02-22T23:19:16Z

This adds Spring WebFlux and WebClient tracing.

I will also add auto-configuration for https://github.com/opentracing-contrib/java-reactor to opentracing-spring-cloud, which will complement these changes with automatic activation of the span in handler functions.

csabakos · 2019-02-22T23:22:39Z

    </dependency>
+    <dependency>
+      <groupId>com.github.tomakehurst</groupId>
+      <artifactId>wiremock-jre8</artifactId>


I had to replace MockRestServiceServer in the tests with something else (WireMock), because MockRestServiceServer does not support WebClient.

csabakos · 2019-02-22T23:23:19Z

  <properties>
-    <maven.compiler.source>1.7</maven.compiler.source>
-    <maven.compiler.target>1.7</maven.compiler.target>
+    <maven.compiler.source>1.8</maven.compiler.source>


Spring Framework 5 requires JRE 8 so I believe this is safe to change.

Note that this is not only SB, the web package can be used with old spring web apps.

@pavolloffay Spring Framework 5+ requires JDK 8+. See here

pavolloffay

@csabakos thanks for the PR! It looks good.

I would like to see a test where a client is used to from multiple threads (assuming it supports concurrent processing) and creates requests to instrumented web endpoint (can be reactive too)

java-spring-web/opentracing-spring-web/src/test/java/io/opentracing/contrib/spring/web/client/TracingAsyncRestTemplateTest.java

Line 65 in 5d7207a

    
           public void testMultipleRequests() throws InterruptedException, ExecutionException {

I have noticed that some files were taken from spring-cloud-sleuth. I think we should add their license (We already doing it for some files in spring-cloud repository)

pavolloffay · 2019-02-25T09:04:29Z

 get automatically traced different set of technologies e.g. `spring-cloud-netflix`, JMS or even more then
 use project [opentracing-spring-cloud](https://github.com/opentracing-contrib/java-spring-cloud) instead.

+For reactive applications, it is especially recommended to use `reactor` tracing from


Is reactor supported in opentracing-spring-cloud?

I'm planning to add Spring Boot auto-configuration for that in a separate pull request. I already have the code ready for it, but opentracing-reactor needs to be published to Maven central first. (See opentracing-contrib/java-reactor#1)

pavolloffay · 2019-02-25T09:08:54Z

+        Assert.assertEquals("GET", span.tags().get(Tags.HTTP_METHOD.getKey()));
+        Assert.assertEquals(TEST_REST_TEMPLATE.getRootUri() + "/", span.tags().get(Tags.HTTP_URL.getKey()));
+        Assert.assertEquals(200, span.tags().get(Tags.HTTP_STATUS.getKey()));
+        Assert.assertEquals("WebFlux/java-spring-web", span.tags().get(Tags.COMPONENT.getKey()));


Change java-spring-webflux to be consistent with other component names in this repo?

pavolloffay · 2019-02-25T09:13:08Z

+    <dependency>
+      <groupId>org.springframework</groupId>
+      <artifactId>spring-webflux</artifactId>
+      <scope>provided</scope>


I think this should be optional? We don't want users to depend on webflux

Thanks, optional makes more sense than provided.

pavolloffay · 2019-02-25T09:14:41Z

+ */
+@Configuration
+@ConditionalOnBean(Tracer.class)
+@ConditionalOnClass(WebClient.class)


Should we also do conditional on reactive?

@ConditionalOnWebApplication(type = ConditionalOnWebApplication.Type.REACTIVE)

@ConditionalOnWebApplication refers to the server technology.

One could use the reactive WebClient in a Servlet-based server application, so I don't think that adding that conditional would be appropriate.

pavolloffay · 2019-02-25T09:25:06Z

+        }
+
+        @Override
+        public void onCancel(final ClientRequest httpRequest, final Span span) {


When does cancel happen?

Excellent question! It looks like cancellation can happen in various circumstances:

If the user of the WebClient "unsubscribes" from the request (for example because they have a timeout that they reached and they no longer wish to wait for the result). I think in this case I think it is correct to log an error on the span, since timeout can be considered an error.

In various Reactor constructs, cancel() is called even when the Mono (successfully) emits an item. An example of this is Flux#next(). Since this is part of the normal (successful) behavior, I don't think we should log an error.

There does not seem to be a good way to differentiate between the above two cases in the tracing code, so I changed this to log a cancelled event instead of an error. It will be up to the user who is looking at the trace logs to determine if the cancellation indicates an error or not. (For example the user can infer this from the HTTP status code, etc.)

If it is a part of normal execution flow and it happes often we can avoid adding meta-data at all.

As an application developer, it is still valuable information that the subscription was cancelled. Cancellation does not happen in simple WebClient flows, only if you're doing something more complex (like retries, or firing off multiple requests and waiting for the fastest one to return).

I think it's worth have these events in the trace logs.

If other people find them bothersome, they can file an issue and we can make it optional, but I think that won't be the case.

csabakos · 2019-02-27T03:01:09Z

@csabakos thanks for the PR! It looks good.

@pavolloffay Thank you for the review!

I would like to see a test where a client is used to from multiple threads (assuming it supports concurrent processing) and creates requests to instrumented web endpoint (can be reactive too)

java-spring-web/opentracing-spring-web/src/test/java/io/opentracing/contrib/spring/web/client/TracingAsyncRestTemplateTest.java

Line 65 in 5d7207a

public void testMultipleRequests() throws InterruptedException, ExecutionException {

I implemented the exact test that you linked to for WebClient: opentracing-spring-web/src/test/java/io/opentracing/contrib/spring/web/client/TracingWebClientTest.java. Is that sufficient or did you have something else in mind?

I have noticed that some files were taken from spring-cloud-sleuth. I think we should add their license (We already doing it for some files in spring-cloud repository)

Conveniently their license is also Apache License 2.0, but I will gladly add it. Would you mind pointing me to an existing example of what exactly needs to be done?

…f the project.

…ince we do not want to signal an error due to cancellation in those cases.

…error, since we do not want to signal an error due to cancellation in those cases." This reverts commit 72a441f.

pavolloffay · 2019-02-27T15:13:30Z

Conveniently their license is also Apache License 2.0, but I will gladly add it. Would you mind pointing me to an existing example of what exactly needs to be done?

I think just add the license to the files and then exclude them in pom https://github.com/opentracing-contrib/java-spring-cloud/blob/master/pom.xml#L380

pavolloffay

@geoand could you please review too?

pavolloffay · 2019-02-27T15:21:05Z

+@AutoConfigureAfter(TracerAutoConfiguration.class)
+@EnableConfigurationProperties(WebTracingProperties.class)
+@ConditionalOnWebApplication(type = ConditionalOnWebApplication.Type.REACTIVE)
+@ConditionalOnProperty(name = "opentracing.spring.web.enabled", havingValue = "true", matchIfMissing = true)


CONFIGURATION_PREFIX can be used

pavolloffay · 2019-02-27T15:24:15Z

+        }
+
+        @Override
+        public void onCancel(final ClientRequest httpRequest, final Span span) {


If it is a part of normal execution flow and it happes often we can avoid adding meta-data at all.

pavolloffay · 2019-02-27T15:27:53Z

+            final String requestUrl = url + i;
+
+            final Scope parentSpan = mockTracer.buildSpan("foo").startActive(false);
+            parentSpan.span().setTag("request-url", requestUrl);


Add tag on spanBuilder

pavolloffay · 2019-02-27T15:29:16Z

+        for (int i = 0; i < numberOfCalls; i++) {
+            final String requestUrl = url + i;
+
+            final Scope parentSpan = mockTracer.buildSpan("foo").startActive(false);


Create a span directly and remove cont. cont does not seems to be needed

geoand · 2019-02-27T15:34:02Z

@geoand could you please review too?

I will do that tomorrow :)

…nfigure license-maven-plugin, and update license headers.

csabakos · 2019-02-27T21:57:35Z

Conveniently their license is also Apache License 2.0, but I will gladly add it. Would you mind pointing me to an existing example of what exactly needs to be done?

I think just add the license to the files and then exclude them in pom https://github.com/opentracing-contrib/java-spring-cloud/blob/master/pom.xml#L380

Done. The pom did not have license-maven-plugin so I went ahead and added that and updated all of the license headers (so that the build passes).

geoand · 2019-02-28T05:57:06Z

@csabakos Can you please resolve the conflict? Thanks

csabakos · 2019-02-28T07:24:42Z

@csabakos Can you please resolve the conflict? Thanks

@geoand resolved by merging master into my feature branch. (Let me know if that's not the preferred way to do this.)

geoand · 2019-02-28T07:46:59Z

That's up to @pavollofay to decide 😎

geoand · 2019-02-28T08:27:36Z

+@ConditionalOnClass(WebClient.class)
+@ConditionalOnProperty(prefix = WebClientTracingProperties.CONFIGURATION_PREFIX, name = "enabled", matchIfMissing = true)
+@AutoConfigureAfter(TracerAutoConfiguration.class)
+@EnableConfigurationProperties(WebClientTracingProperties.class)


Should WebClientTracingProperties be renamed to ClientTracingProperties?
I mean the name is a little misleading since it's configuration for RestTemplate, AsyncRestTemplate and WebClient.
Also, the comments in WebClientTracingProperties should be updated to reflect this fact

I don't have an opinion about renaming - I could see arguments both pro and con. I'll leave the decision up to you and @pavolloffay. (Because the class is public, it is in theory part of the public API, so that'd be one reason for not renaming it.)

I will update the comments though.

It's true that it's public, public configuration classes would rarely be used directly in application code. I would assume that at least a minor version bump would be warranted even without the rename, so some minor breaking could probably be expected.
Let's leave it up to @pavolloffay to decide :)

geoand · 2019-02-28T08:30:52Z

+    <dependency>
+      <groupId>org.springframework</groupId>
+      <artifactId>spring-webflux</artifactId>
+      <scope>compile</scope>


This is default scope, so perhaps best to omit it?

Shouldn't this be optional?

Nice catch! Looks like I only updated the POM in opentracing-spring-web-starter but not in opentracing-spring-web. Fixed now.

geoand · 2019-02-28T09:10:03Z

Looks good from a Spring perspective 👍

pavolloffay · 2019-03-04T16:42:27Z

@geoand can this be merged?

geoand · 2019-03-04T16:46:51Z

@pavolloffay yeah it LGTM. Should I merge and cut a release?

pavolloffay · 2019-03-04T16:48:58Z

👍

geoand · 2019-03-04T16:53:09Z

Thanks for the awesome work @csabakos!

csabakos · 2019-03-04T18:45:21Z

Thanks y'all for the prompt review and good comments!

I'll work on adding the Spring Boot starter for opentracing-reactor to opentracing-spring-cloud after opentracing-contrib/java-reactor#1 is completed.

geoand · 2019-03-04T19:35:55Z

Thanks again @csabakos!

Add Spring WebFlux and WebClient tracing.

e41a79f

csabakos commented Feb 22, 2019

View reviewed changes

csabakos mentioned this pull request Feb 23, 2019

Instrument WebFlux opentracing-contrib/java-spring-cloud#150

Closed

Finish the span on cancel.

911d88a

pavolloffay reviewed Feb 25, 2019

View reviewed changes

csabakos added 5 commits February 26, 2019 21:07

Make WebFlux and WebClient component names consistent with the rest o…

b35ecbd

…f the project.

Make spring-webflux and optional dependency.

5788ed4

Do not call onCancel() if the Mono has emitted a value or an error, s…

72a441f

…ince we do not want to signal an error due to cancellation in those cases.

Revert "Do not call onCancel() if the Mono has emitted a value or an …

8c5ab7a

…error, since we do not want to signal an error due to cancellation in those cases." This reverts commit 72a441f.

Do not flag cancellation as error.

4b05794

pavolloffay reviewed Feb 27, 2019

View reviewed changes

Add the license from spring-cloud-sleuth to the appropriate files, co…

1fffcb8

…nfigure license-maven-plugin, and update license headers.

csabakos added 3 commits February 27, 2019 16:06

Fix one more header.

64ee205

Use CONFIGURATION_PREFIX in WebFluxTracingAutoConfiguration.

6269272

Simplify tests a bit.

3b99d6e

Merge branch 'master' into add-webflux-and-webclient-tracing

baca97e

geoand reviewed Feb 28, 2019

View reviewed changes

csabakos added 3 commits February 28, 2019 10:31

Update comments in WebClientTracingProperties.

5e11028

Omit default "compile" scope from pom.xml.

0920529

Make spring-webflux optional and compile scope.

d6af3d0

csabakos mentioned this pull request Mar 4, 2019

Publish to Maven Central opentracing-contrib/java-reactor#1

Closed

geoand merged commit 0ac36c3 into opentracing-contrib:master Mar 4, 2019

csabakos deleted the add-webflux-and-webclient-tracing branch March 8, 2019 00:07

csabakos mentioned this pull request Apr 8, 2019

Add Spring Boot autoconfiguration for Reactor instrumentation. opentracing-contrib/java-spring-cloud#208

Merged

Uh oh!

Conversation

csabakos commented Feb 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

geoand Feb 25, 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!

pavolloffay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csabakos Feb 27, 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!

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!

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!

csabakos commented Feb 27, 2019

Uh oh!

pavolloffay commented Feb 27, 2019

Uh oh!

pavolloffay 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!

geoand commented Feb 27, 2019

Uh oh!

csabakos commented Feb 27, 2019

Uh oh!

geoand commented Feb 28, 2019

Uh oh!

csabakos commented Feb 28, 2019

Uh oh!

geoand commented Feb 28, 2019

Uh oh!

geoand Feb 28, 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!

Choose a reason for hiding this comment

Uh oh!

geoand Feb 28, 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!

Choose a reason for hiding this comment

csabakos commented Feb 22, 2019 •

edited

Loading

geoand Feb 25, 2019 •

edited

Loading

csabakos Feb 27, 2019 •

edited

Loading

geoand Feb 28, 2019 •

edited

Loading

geoand Feb 28, 2019 •

edited

Loading