WIP - Upgrades Ratpack 1.6 and Brave 5.5.1 #67
Conversation
llinder
commented
Dec 10, 2018
llinder
commented
- replaces client instrumentation wrapper with new interceptor api.
- updated spock tests for ratpack 1.6
| <braveVersion>5.1.5</braveVersion> | ||
| <ratpackVersion>1.4.6</ratpackVersion> | ||
| <braveVersion>5.5.1</braveVersion> | ||
| <ratpackVersion>1.6.0-rc-2</ratpackVersion> |
There was a problem hiding this comment.
1.6 hasn't been released yet so we should wait for that.
|
|
||
| bind(ZipkinHttpClientImpl.class); | ||
| bind(HttpClient.class) |
There was a problem hiding this comment.
This changes behavior in that all bindings on HttpClient will now use one that is configured for tracing. This may be a confusing change if folks are use to the required @Zipkin annotation. In practice however, I have found it more likely that folks forget the @Zipkin annotation and wonder why there is no client tracing.
There was a problem hiding this comment.
I think this is inline with how tracing should work.
|
|
||
| private final HttpClientHandler<RequestSpec, Integer> handler; | ||
| private final TraceContext.Injector<MutableHeaders> injector; | ||
| private final Execution execution; |
There was a problem hiding this comment.
According to feedback I received from the Ratpack Slack channel, we should be able to rely on the Ratpack Execution to maintain trace context state for us. While the tests do pass with this configuration, we should still try it on a real application just to be sure.
| )) | ||
| ).getValue(); | ||
| // small delay to ensure the response interceptor is invoked before this method exits. | ||
| Thread.sleep(1); |
There was a problem hiding this comment.
I'm not sure why but the response interceptor ran after the blocking get call. While not a thrilling solution, adding a sleep here fixed the issue. Since the tests are written in a blocking fashion, and Ratpack code is written in a non blocking fashion, I suspect that actual production code will work as expected. Again we should investigate testing this more... maybe adding some Ratpack specific tests is warranted here before merging.
|
Given the major changes I think it would make sense to rev the major version on this |
- replaces client instrumentation wrapper with new interceptor api. - updated spock tests for ratpack 1.6
