Transformation to Zipkin

[SergeyKanzhelev]

Replacing #221 s it has no traction

I hope to get this merged before addressing other items that needs to be merged. Discussions for those may be easier to handle in small PRs

Related issue: #220

[tedsuo]

@SergeyKanzhelev this generally looks good. One comment: zipkin has a way of trying to joining the client and server spans together, which is the default behavior described in B3. Perhaps there should be some language here to describe this - something to the effect that it is fine to plug in an SDK which runs in this mode, but by default the OTel SDK follows the alternate B3 implementation which allows server and client spans to have separate IDs.

Related PR: #367

[tsloughter]

Is this encoding the Attributes of an Event as json within a string?

[jmacd]

Let's rephrase these "TODO" items as some kind of undefined to-be-determined future specification? The fact that we have these TODOs is preventing us from merging this work.

[Oberon00]

Why is this blocking the merge? We have TODOs elsewhere too, e.g. in the HTTP semantic conventions. I'd suggest creating issues for them and linking them though.

[jmacd]

Someone in the triage party said we should. Can we replace "TODO" with "TBD"? Otherwise, please explain exactly what we should do to satisfy the TODO in the text, otherwise it feels like unfinished work.

[jmacd]

Let's try to get this merged.

[tsloughter]

Not a blocker, but since I have an implementation of this I hoped to get my question (#380 (comment)) answered (and it would help the spec if it mentioned this).

Is the attribute encoding, like on an event, simply a json string? It would make things clearer if it stated that instead of giving the sample as something that looks like json.

[c24t]

Why is this blocking the merge? We have TODOs elsewhere too, e.g. in the HTTP semantic conventions.

Not all TODOs are TBDs. "TODO: add examples" in particular looks like unfinished work.

I agree that we shouldn't block this over the TODOs, but want to avoid merging in specs that look half-baked.

[vikrambe]

Good to see transformation to zipkin doc here. i think this PR will also have to handle error and error.message span tags
@SergeyKanzhelev @tigrannajaryan @yurishkuro request you to review and comment on PR https://github.com/open-telemetry/oteps/pull/86/files?short_path=5746d35#diff-5746d35006fd9b35a7e7163da3faa935

[Oberon00]

@vikrambe I couldn't find any authoritative information on the error tag in zipkin using a quick search. Could you provide a link to a definition or example?

[vikrambe]

@Oberon00 #380 (comment)

[tigrannajaryan]

For prior art see also how OpenCensus to Zipkin V1 translations are implemented in Collector (which inherited this from OpenCensus): https://github.com/open-telemetry/opentelemetry-collector/blob/master/translator/trace/zipkin/protospan_to_zipkinv1.go
It is preferable to adhere to this prior art where possible unless there is a good reason to diverge, e.g. due to differences in OpenCensus and OpenTelemetry.

[vikrambe]

@vikrambe I couldn't find any authoritative information on the error tag in zipkin using a quick search. Could you provide a link to a definition or example?

https://github.com/jaegertracing/jaeger/blob/master/cmd/collector/app/sanitizer/zipkin/span_sanitizer.go

[Oberon00]

@vikrambe This is from Jaeger. That is not the same as zipkin.

[tigrannajaryan]

@vikrambe I couldn't find any authoritative information on the error tag in zipkin using a quick search. Could you provide a link to a definition or example?

I concur.

https://github.com/jaegertracing/jaeger/blob/master/cmd/collector/app/sanitizer/zipkin/span_sanitizer.go

@vikrambe this shows Jaeger's interpretation of Zipkin's "error" tag. What is the authoritative Zipkin source which describes the meaning of the "error" tag? Is this part of Zipkin specification or just Jaeger's take on it?

[devinsba]

@Oberon00 and @tigrannajaryan

I gave an answer: #380 (comment)

[vikrambe]

@vikrambe I couldn't find any authoritative information on the error tag in zipkin using a quick search. Could you provide a link to a definition or example?

I concur.

https://github.com/jaegertracing/jaeger/blob/master/cmd/collector/app/sanitizer/zipkin/span_sanitizer.go

@vikrambe this shows Jaeger's interpretation of Zipkin's "error" tag. What is the authoritative Zipkin source which describes the meaning of the "error" tag? Is this part of Zipkin specification or just Jaeger's take on it?

@tigrannajaryan openzipkin/zipkin4net#212

[vikrambe]

@vikrambe This is from Jaeger. That is not the same as zipkin.

@Oberon00 openzipkin/zipkin4net#212

[Oberon00]

@vikrambe Answering once is enough 😉 But thanks for the link, it seems that this is indeed a thing.

[CodeBlanch]

@SergeyKanzhelev Any chance of getting Zipkin Remote_endpoint field mapping defined? I have been playing with the .NET ZipkinExporter and without specifying Remote_endpoint Zipkin won't treat spans to non-instrumented services (3rd party) as external/dependencies. It really reduces the usefulness of the tool. See open-telemetry/opentelemetry-dotnet#483

[SergeyKanzhelev]

@CodeBlanch happy to do it in the next iteration

[SergeyKanzhelev]

I addressed all comments. Seems to be all resolved. Merging.