Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spring web tracing #2

Merged

Conversation

pavolloffay
Copy link
Contributor

This pulls auto-configs from ot-spring-web.

Copy link
Contributor

@objectiser objectiser left a comment

Choose a reason for hiding this comment

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

LGTM - might want to get Charles or Ales to also take a look.

Although the tests seem to be failing on Travis.

README.md Outdated
* Spring Web

## Configuration
Just drop the following dependency on classpath:
Copy link
Contributor

Choose a reason for hiding this comment

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

on classpath is misleading - should just say "Just add the following dependency in your maven pom:"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's basically classpath :P

README.md Outdated
, and provide OpenTracing tracer bean:
```java
@Bean
public io.opnetracingTracer tracer() {
Copy link
Contributor

Choose a reason for hiding this comment

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

io.opentracing.Tracer

<dependency>
<groupId>io.opentracing</groupId>
<artifactId>opentracing-mock</artifactId>
</dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be a test dependency? and if so, should then move below the spring-web-autoconfigure dependency.

@malafeev
Copy link

looks like you created multi module project. Maybe it is better to have one project without modules but with different packages for Web, Feign, RxJava etc?

@pavolloffay
Copy link
Contributor Author

Yes, we want one artifact which would instrument everything and currently it is opentracing-spring-cloud. I have created parent to be more flexible (e.g. separate integration tests, possibly other artifacts used by opentracing-spring-cloud)

@malafeev
Copy link

malafeev commented Jul 12, 2017

As I remember I had an issue with @LoadBalanced RestTemplate:

@Bean
@LoadBalanced
public RestTemplate restTemplate() {
 return new RestTemplate();
}

It was not automatically instrumented by java-spring-web.
Therefore I added PostConstruct:

@PostConstruct
public void addRestTemplateInterceptor() {
  // We need manually add tracing interceptor because of @LoadBalanced RestTemplate
  restTemplate.getInterceptors().add(new TracingRestTemplateInterceptor(tracer));
 }

I didn't look how @LoadBalanced annotation is processed.

@pavolloffay
Copy link
Contributor Author

I will have a look at it tomorrow but I believe it can also be handled by Ribbon instrumentation (spring-cloud-starter-ribbon) as @LoadBalanced comes from there.

@pavolloffay
Copy link
Contributor Author

@malafeev I had a look at it and @LoadBalanced for RestTemplate works fine. It just adds another interceptor after the tracing one. What we could do in the future is to trace ribbon ping requests to specified servers. I will create an issue for it.

Example code:
https://github.com/pavolloffay/opentracing-java-examples/tree/ribbon-load-balanced

@pavolloffay
Copy link
Contributor Author

@alesj @cmoulliard feel free to comment later, I will merge to not block @malafeev

@pavolloffay pavolloffay merged commit 774bb4b into opentracing-contrib:master Jul 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants