Adds XRay specific brave codes #535
Conversation
|
@adriancole I am in the process of starting to support xray but instead of opening a new module I decided to open packages to support it. What do you think? |
|
As a user of |
|
The problem is that there is already propagation codes in Brave related to aws. Adding |
|
The problem is that there is already propagation codes in Brave related to
aws. Adding brave dependency to zipkin-aws may cause circular
dependencies. see #openzipkin/zipkin-aws#58
<openzipkin/zipkin-aws#58> please
I know I made the comment, but in a pinch, there's no problem. For example,
X-Ray is in fact different than AWS propagation. There are nodes that don't
use X-Ray but use AWS propagation. I'm not sure exactly where we would end
up with a circular dep unless you know a place where it is required to
access AWS propagation in order to derive out-of-band data..
In other words, I don't think there will be a circular dependency unless we
somehow depend on brave-propagation-aws. One reason I'd consider reverting
opinion (keeping mapping-related here), is that @devinsba has a history of
working with aws. Having engagement from others can help sway an opinion
from mildly in favor one way towards another. Indeed there would be less
thrash if mapping-related concerns are in the same place, at least
initially.
If we decided to do this here, we'd need to set the version of brave to
provided scope.
|
|
one thing lets try to do is be flexible until things work.. practice
will steer things, too
|
|
Just to put more weight on this. I'm watching things related to X-Ray very
closely. In analyzing our deployment the cost benefit to elasticsearch is
not there for us, so I feel moving to X-Ray is a change that I may make in
the short term to cut some costs until someone commits to a solution or
vendor much further up the ladder from myself.
|
|
So I will move these classes to |
| } | ||
|
|
||
| @Override | ||
| protected void doPreProcessTag(Connection connection, String sql, Span span) { |
There was a problem hiding this comment.
you've probably seen the http client parser stuff in brave-instrumentation-http. This hints at a similar one brave-instrumentation-sql. If you are up to it, probably a good thing to make a module as most of these properties will be re-used across p6spy and the two mysql variants. This will reduce the amount of copy/pasting and also decouple dependencies when you go to make brave-parser-xray
| import brave.http.HttpServerParser; | ||
| import zipkin.TraceKeys; | ||
|
|
||
| public class XRayHttpServerParser extends HttpServerParser { |
There was a problem hiding this comment.
I think we'll end up with something like XRayParser.httpServer() httpClient() sqlClient() etc. If we do our dependencies well, then we can have brave-parser-xray live in zipkin-aws and only depend on brave-instrumentation-http and a future brave-instrumentation-sql
There was a problem hiding this comment.
@adriancole Can you expand this a little bid more? Since TracingStatementInterceptor are appended to sql connection strings, it made me confused.
There was a problem hiding this comment.
I mean to create a generic parser for SQL databases, similar to what we have in brave-instrumentation-http. ex a new support module which would allow you to plug-in SqlClientParser like HttpClientParser. ex brave-instrumentation-sql
There was a problem hiding this comment.
if you did this, it would decouple xray from each individual sql instrumentation library
|
closing because it is really old and we do have interest in the zipkin-aws repo |
Don't merge please yet