search

jaegertracing / legacy-client-java

Legacy com.uber.jaeger java client
Apache License 2.0
5 stars 6 forks source link

Port main libraries to use new client implementation #20

Closed isaachier closed 6 years ago

isaachier commented 6 years ago

@pavolloffay @yurishkuro this is my bridge to new client discussed in jaegertracing/jaeger-client-java#508. I changed the core library to "typedef" the new client's core classes. Then, I made as few changes as I felt necessary to keep the instrumentation working as is. That is the biggest "test case" seeing as they relied on parts of the internal library that most clients would not. Regarding instrumentation, I did delete a number of the libraries that are not used at Uber and/or implemented in the new client. These include jaeger-b3, jaeger-zipkin, jaeger-crossdock, jaeger-thrift, jaeger-traceresolver, and jaeger-micrometer.

pavolloffay commented 6 years ago

I haven't probably read the https://github.com/jaegertracing/jaeger-client-java/issues/508 properly...

We didn't do this exact thing when migrating because it requires breaking changes anyway. Only the most simple scenarios work without breaking change (new Configuration("service").getTracer()).

isaachier commented 6 years ago

Correct but this actually solves issues for 90% of internal code at Uber.

isaachier commented 6 years ago

@pavolloffay out of curiosity, what did you think my plan was before you saw this?

yurishkuro commented 6 years ago

as a proof of concept, I think you should have kept the tests for the classes that are extending io.jaegertracing, i.e. tests for configuration, tracer, span, span context.

isaachier commented 6 years ago

@yurishkuro I could fix those things. Is it worth pursuing this here or just closing this and doing internally?

yurishkuro commented 6 years ago

I am fine with doing it internally only, as we will have fewer restrictions on back-compat

isaachier commented 6 years ago

Cool thanks.

pavolloffay commented 6 years ago

@isaachier I didn't think that you are fine with this and I was curious to see if some breaking changes can be solved. I tried to do the same when migrating to io.jaegertracing but I stopped as there are always breaking changes - so rather let people fully upgrade.