add instrumentation for spring AsyncRestTemplate.#1919
add instrumentation for spring AsyncRestTemplate.#1919rghetia merged 6 commits intocensus-instrumentation:masterfrom
Conversation
| signalfxVersion = '0.0.48' | ||
| springBootVersion = '1.5.15.RELEASE' | ||
| springBootTestVersion = '2.1.1.RELEASE' | ||
| springBootVersion = '2.1.5.RELEASE' |
There was a problem hiding this comment.
IIRC Spring boot 1.x and 2.x are incompatible and we're still depending on 1.x in other artifacts. So maybe we need to keep the 1.5.15 version? /cc @dinooliva
There was a problem hiding this comment.
Yes - springBootVersion is used to build libraries.spring_boot_starter_web which is used to build spring_sleuth_v1x and that artifact is explicitly meant to be built with Spring 1.x (surprised the unit tests don't fail).
We may need to create a legacy version of the Spring boot library for use with existing artifacts and migrate them as possible.
There was a problem hiding this comment.
I have left the version as is for spring_sleuth_v1x. Changed the version for opencensus-contrib-spring.
| @SuppressWarnings("MustBeClosedChecker") | ||
| public void should_close_span_upon_success_callback() | ||
| throws ExecutionException, InterruptedException { | ||
| this.tracer = Tracing.getTracer(); |
There was a problem hiding this comment.
Curious - why 'this.tracer' rather than 'tracer'?
There was a problem hiding this comment.
copy/paste. Removed.
|
|
||
| @Test(timeout = 10000) | ||
| @Order(1) | ||
| // Need to suppress warnings for MustBeClosed because Java-6 does not support try-with-resources. |
There was a problem hiding this comment.
I thought tests were no longer constrained to Java 6.
There was a problem hiding this comment.
change the compatibility to 1.8 and removed the suppression.
| .start(); | ||
|
|
||
| future.get(500, TimeUnit.MILLISECONDS); | ||
| assertThat(true).named("should throw an exception from the controller"); |
There was a problem hiding this comment.
actually I wanted equivalent of assertThrows(). I have re-implemented this. Let me know if that works.
| } | ||
|
|
||
| TracingAsyncClientHttpRequestInterceptor() { | ||
| System.out.println("constructing interceptor"); |
There was a problem hiding this comment.
Why the println? Here and below.
There was a problem hiding this comment.
leftover from debugging. Removed it.
|
@dinooliva PTAL. |
dinooliva
left a comment
dinooliva
left a comment
There was a problem hiding this comment.
Overall looks good - a couple minor questions.
|
|
||
| @PostConstruct | ||
| public void init() { | ||
| if (this.restTemplates != null) { |
There was a problem hiding this comment.
Why 'this' here and elsewhere?
| new ArrayList<org.springframework.http.client.AsyncClientHttpRequestInterceptor>( | ||
| restTemplate.getInterceptors()); | ||
| interceptors.add(this.clientInterceptor); | ||
| restTemplate.setInterceptors(interceptors); |
There was a problem hiding this comment.
Is there any concern that you may overwrite existing interceptors in the restTemplate?
There was a problem hiding this comment.
I am adding it to existing interceptor.
|
@dinooliva PTAL. |
|
@songy23 PTAL. |
songy23
left a comment
songy23
left a comment
There was a problem hiding this comment.
A few minor nits, overall LGTM.
| log4j2Version = '2.11.1' | ||
| signalfxVersion = '0.0.48' | ||
| springBoot2Version = '2.1.5.RELEASE' | ||
| springBootTest2Version = '2.1.5.RELEASE' |
There was a problem hiding this comment.
Nit: this isn't needed since it's the same as springBoot2Version
| it.targetCompatibility = 1.8 | ||
| } | ||
|
|
||
| springBootVersion = '2.1.5.RELEASE' |
| final Tracer tracer; | ||
| final HttpClientHandler<HttpRequest, ClientHttpResponse, HttpRequest> handler; | ||
|
|
||
| // final Injector<HttpHeaders> injector; |
4 participants
No description provided.