search

openzipkin / zipkin-reporter-java

Shared library for reporting zipkin spans on transports such as http or kafka
Apache License 2.0
126 stars 69 forks source link

Adds Brave integration #171

Closed codefromthecrypt closed 4 years ago

codefromthecrypt commented 4 years ago

This takes a copy of ZipkinSpanHandler in Brave so that we can eventually decouple Brave from a compile depedency on Zipkin Reporter.

This also allows the commonly requested feature (such as exists in Sleuth) to allow multiple simultaneous reporters.

codefromthecrypt commented 4 years ago

going to add spring-beans support next, so that there's no awkward reverse dependency

codefromthecrypt commented 4 years ago

@anuraaga I tested this with spring 2.5 XML both earliest Brave (5.6) and snapshot (which gets rid of the log messages). PTAL?

codefromthecrypt commented 4 years ago

nap was useful. the generic type of the asyncreporter is wrong! I will fix it in a bit

codefromthecrypt commented 4 years ago

nah can't do what I want right now: AsyncReporter<MutableSpan>. the MutableSpan type doesn't have the fields we need yet to size up the span (traceID, parentId etc). Will revisit once Brave 5.12 is out.

codefromthecrypt commented 4 years ago

I'm currently of the opinion that setting minimum brave version to 5.12 is worth it as it avoids a lot of hacks. Folks can safely move from 5.6 to 5.12 anyway and worst case we have to teach them how to use the bom. By setting minimum to 5.12, we can actually skip the step of converting to zipkin span first, and directly encode into v2 json or proto.

anuraaga commented 4 years ago

This library basically forked off of brave in some sense - so making the minimum the point of fork seems fine to me.

codefromthecrypt commented 4 years ago

we have an interdependency on https://github.com/openzipkin/brave/pull/1167 so I'm merging and cutting zipkin-reporter with a pending version of brave. It will eventually resolve later when brave is cut.

codefromthecrypt commented 4 years ago

thanks for the eyes @anuraaga and @jeqo !