opentracing-contrib / java-spring-web

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

Configurable RestTemplate tracing with support for RestTemplateBuilder #68

Closed mdvorak closed 6 years ago

mdvorak commented 6 years ago
  1. Added WebClientTracingProperties, with enabled and componentName
  2. Moved RestTemplate and AsyncRestTemplate AutoConfig into once umbrella class RestTemplateTracingAutoConfiguration, which allows us to have all @Conditional annotations in one place. Actual configurations are in nested static classes, which is pattern used heavily by Spring Boot itself. Allows us to use additional conditions on each configuration class.
  3. Added TracingRestTemplateCustomizer, which is used by Spring Boot RestTemplateBuilder (not included in Spring Framework itself). With SB, its valid pattern to have one preconfigured builder bean, and consumers create RestTemplate instances locally, private, even on demand - but still share common configuration. RestTemplateCustomizer is pattern to hook into builder config without directly modifying the bean. Side effect is that also TestRestTemplate now creates traces - therefore disabled client tracing for Servlet tests.

This is a big PR including multiple changes, but I found it would be complicated to split it across several PRs. It should however be non-breaking backwards compatible. Only change in behavior should be for RestTemplateBuilder beans, which might influence some applications. But its likely it will fix them instead of breaking them.

Discussion is welcome :)

pavolloffay commented 6 years ago

@mdvorak thanks for the PR.

The auto-configuration part looks fine.

pavolloffay commented 6 years ago

Can be TracingRestTemplateCustomizer moved to the starter module?

mdvorak commented 6 years ago

You mean opentracing-spring-web-autoconfigure? Of course. Moving it to opentracing-spring-cloud would split RestTemplate config into two separate projects, which I don't like. Commit coming up :)

mdvorak commented 6 years ago

Note: While RestTemplateCustomizer should not break anything, its a new feature, and should at least have version bump to 0.3.x.

mdvorak commented 6 years ago

I've tested the change with opentracing-spring-cloud, and indeed there are plenty of failing tests - whenever they expect exact span count in the tracer and TestRestTemplate is used. I can prepare patch for otsc that will work with this version by disabling client tracing for all tests where servlet is tested.

pavolloffay commented 6 years ago

@mdvorak sorry for the delay, I am fine with changes, I will have the last look and merge/comment.

You don't have to create a PR to bump the version, it will be done automatically as part of the release.

mdvorak commented 6 years ago

No problem, thanks for the review, I know its as hard as coding :-)

pavolloffay commented 6 years ago

@mdvorak thanks! I will cut 0.3.0