OTLP handler #213
OTLP handler #213
Conversation
marcingrzejszczak
force-pushed
the
otlp
branch
from
65e05b9
to
5e68181
Compare
There was a problem hiding this comment.
Thank you for your perseverance through all the iterations on this. I think it landed somewhere nice both on maintenance and usage. I left some minor comments, but overall this looks good to me. I did wonder a bit about the naming of the module. In a theoretical future where we tried to support OTLP in JSON format or OTLP over gRPC, what would we do? The module naming now kind of implies it would all go in the same module. Would that be feasible? I know it's a bikeshed issue, but once we release the module with whatever naming, it will be semi-permanent.
| * @since 2.16 | ||
| */ | ||
| public Builder toBuilder() { | ||
| // For testing, this is easier than making the type abstract: It is package sealed anyway! |
There was a problem hiding this comment.
I think this is copied from ZipkinSpanHandler, but everything involved here is public; not package-private. I thought maybe that was an oversight, but this is what users are supposed to use in the synchronous case, according to the README. So I'm not sure about the comment, but seeing as it is consistent with existing code, I don't think we need to block on figuring it out.
| * | ||
| * @see AsyncOtlpSpanHandler | ||
| * @see brave.Tracing.Builder#addSpanHandler(SpanHandler) | ||
| * @since 2.16 |
There was a problem hiding this comment.
I guess we could do a new minor for this feature, which would make this 2.17. But we can fix all the @since tags on merge.
| * the License. | ||
| */ | ||
| /** | ||
| * This class is internal and is hence not for public use. Its APIs are unstable and can change at |
There was a problem hiding this comment.
This is a package JavaDoc; not a class. At least some of the contents are not internal. Perhaps this is a copy-paste issue?
|
I added a few more reviewers. I'd like to get input from at least one other Zipkin member on this. |
|
Randomly saw the question about JSON / gRPC. Just as a reference, otel sdk has just one artifact, mainly due to reimplementing the gRPC wire protocol on top of okhttp and directly marshaling without protobuf (heavily inspired by brave's proto serialization). I described it in a post once here open-telemetry/opentelemetry-specification#1996 It's quite a bit of work so not something to think about for a first version but maybe that can help if deciding on artifacts in the future. |
|
per various discussions, this belongs in a different repo similar to zipkin-gcp. opened https://github.com/openzipkin-contrib/zipkin-otel to carry any of this forward. I can't personally assist, but perhaps other committers in support of this can review and/or grant karma to let folks make something like zipkin-gcp except for otel stuff! |
|
made a comment here. I'll create scaffolding in zipkin-otel so that it is easy to re-raise this PR and carry things forward. |
|
https://groups.google.com/g/zipkin-dev/c/YwKHhoR-hZQ/m/Fs7UBWMmBAAJ is ready for you to take over and run with |
Fourth pass (and hopefully the last - commit d68defc)
Got feedback from @shakuzen that we can reuse an existing proto artifact that otel produces even though it's alpha (which was th reason why I didn't use it). Also he told me we can send PROTO over HTTP which should make things better from maintainability perspective (no GRPC issues).
Third pass (commit 5e68181)
After looking at the feedback on the issue #209 (comment) I've moved the code to a separate module. The tests are passing so I guess that the PR is ready for review.
Corresponding PR to the Zipkin server that adds a OTLP GRPC endpoint can be found here
Second pass (commit 8bb91a1)
Adds support for using GRPC to send spans over the wire. Uses just the Handler and Reporter abstractions but not the Sender. You have to use the
OtlpReporterand pass in a ManagedChannelBuilder to send traces to an OTLP compliant endpoint.It would be beneficial to add support for receiving of such data and converting them to Zipkin format.
First pass (till commit fbccfce)
I was naive to think you can take a
MutableSpan, convert it to proto version of whatever OTLP accepts and then use Protobuff's JSON conversion. It turns out that OTLP has deviated away from what Protobuf does out of the box - https://github.com/open-telemetry/opentelemetry-proto/blob/main/docs/specification.md#json-protobuf-encoding.Things to look at:
Senderallow to send just bytes not a list of bytes cause OTLP doesn't expect a list. With that change we can reuse all senders we have (with slight changes)Other than that the result looks promising. I don't understand why the annotation has a negative time but I will get to the bottom of that next week.