search

opentracing-contrib / java-spring-web

OpenTracing Spring Web instrumentation
Apache License 2.0
107 stars 59 forks source link

Add Spring WebFlux and WebClient tracing. #95

Closed csabakos closed 5 years ago

csabakos commented 5 years ago

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 commented 5 years ago

@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?

pavolloffay commented 5 years ago

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

geoand commented 5 years ago

@geoand could you please review too?

I will do that tomorrow :)

csabakos commented 5 years ago

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 commented 5 years ago

@csabakos Can you please resolve the conflict? Thanks

csabakos commented 5 years ago

@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 commented 5 years ago

That's up to @pavollofay to decide 😎

geoand commented 5 years ago

Looks good from a Spring perspective :+1:

pavolloffay commented 5 years ago

@geoand can this be merged?

geoand commented 5 years ago

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

pavolloffay commented 5 years ago

:+1:

geoand commented 5 years ago

Thanks for the awesome work @csabakos!

csabakos commented 5 years ago

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 https://github.com/opentracing-contrib/java-reactor/issues/1 is completed.

geoand commented 5 years ago

Thanks again @csabakos!