Upgrade the dependencies to their latest stable versions #195
Conversation
trustin
commented
Jul 4, 2016
•
trustin
commented
- Brave 3.9.0
- Jackson 2.7.5
- Jetty 9.3.10
- Jetty ALPN agent 2.0.3
- Netty 4.1.2.Final
- Netty TCNative (BoringSSL-static) 1.1.33.Fork18
- Tomcat 8.0.36
- Related changes:
- Remove the references to the deprecated/removed APIs
| @@ -59,7 +60,8 @@ protected TracingRemoteInvoker(RemoteInvoker remoteInvoker, Brave brave) { | |||
| Method method, Object[] args) throws Exception { | |||
|
|
|||
| // create new request adapter to catch generated spanId | |||
| final InternalClientRequestAdapter requestAdapter = new InternalClientRequestAdapter(method.getName()); | |||
| final InternalClientRequestAdapter requestAdapter = new InternalClientRequestAdapter( | |||
| Endpoint.create(uri.toString(), 0, 0), method.getName()); | |||
There was a problem hiding this comment.
@synk It seems like ClientRequestAdapter now allows specifying the information about the endpoint. Because we do not know about the IP address and port number of the endpoint at this point, I just specified 0. As you see, I specified the full URI as a service name. Would this be OK?
By the way, I guess Endpoint API may change in the near future because it doesn't seem like a good idea to have no support for IPv6 addresses.
There was a problem hiding this comment.
/cc @adriancole FYI about lacking IPv6 support
There was a problem hiding this comment.
service names should be low cardinality, as they end up in the UI as a drop-down. URI's usually aren't low cardinality.
There's a number of options people use, best is some sort of a platform id for the instance serving the request.
There was a problem hiding this comment.
Thanks for an advice. Let me change it to URI.getAuthority() then.
There was a problem hiding this comment.
ps on lacking ipv6 openzipkin/zipkin#306 (comment)
There was a problem hiding this comment.
Thanks a lot, @adriancole !
| return TraceData.builder().build(); | ||
| } | ||
|
|
||
| final HttpHeaders headers = ((HttpRequest) request).headers(); | ||
| final HttpHeaders headers = ((HttpMessage) request).headers(); |
There was a problem hiding this comment.
Should the above instanceof check be updated to match this cast?
There was a problem hiding this comment.
Yes and no. Casting to HttpRequest is overly narrow downcast. I'm fine with either but I just wanted to make the IDEA inspector happy.
- Brave 3.9.0 - Jackson 2.7.5 - Jetty 9.3.10 - Jetty ALPN agent 2.0.3 - Netty 4.1.2.Final - Netty TCNative (BoringSSL-static) 1.1.33.Fork18 - Tomcat 8.0.36 - Related changes: - Remove the references to the deprecated/removed APIs
|
@trustin The changes related with tracing looks good to me |
|
I also checked changes. LGTM! |
