Handling the ZipkinV2 span error tag conversion to OTLP

[ericmustin]

Describe the bug
This is a continuation of a discussion in the collector contrib repo issues, here open-telemetry/opentelemetry-collector-contrib#1644 (comment). To summarize:

The zipkin translation code used to rely on the error tag, see: https://github.com/open-telemetry/opentelemetry-collector/pull/1002/files#diff-e8c9e4b66e4631dea1b8be1094a17e31ec8037ba033074ce310341bbdf9860a3L495

but now relies on different criteria, depending on whether using zipkinv2 or zipkinv1 format:

• zipkinv1 translation looks to rely on error tag: here

• but v2 does not: here

A user is using the signalfx-nodejs-tracing client, it appears they export zipkin in v2 format according to their documentation:

The SignalFx-Tracing Library for JavaScript is a fork of the DataDog APM JavaScript Tracer that has been modified to provide Zipkin v2 JSON formatting, B3 trace propagation functionality, and properly annotated trace data for handling by SignalFx Microservices APM.

So, it would appear the issue is with either the sfx node tracing client definition of the zipkinv2 format, or the collector's zipkinv2 => internal translation logic.

Steps to reproduce
Simply send errors generated by the sfx node client to an otel collector. Here's an example client and otel config
https://gist.github.com/ericmustin/3a66bf36840aadd4ccd58ebf8e7278c7

What did you expect to see?
These should be translated to have a Span.Status() representing an error

What did you see instead?
No span status representing an error

What version did you use?
v0.15 of collector

What config did you use?
See reproduction gist

Additional context

I woud like to determine next steps here and narrow down whether:

1. The OpenTelemetry translation of zipkinv2 spans to OTLP is semantically correct and no changes need to be made within the OpenTelemetry-Collector

2. The OpenTelemetry translation of zipkinv2 spans to OTLP is Incorrect and needs to be changed to account for the error tag.

3. The SignalFx-Nodejs-Tracing client is not correctly exporting traces in Zipkinv2 format

As @owais is a maintainer of both this library and the SignalFx Library, his input is greatly appreciated here, along with @rmfitzpatrick who is quite active on both libraries as well. If there are no plans to make any changes to either the collector translation logic or the vendor client as well, that is OK too but it would be good to know so that myself and @MrAlias can make any determinations around our own vendor exporter implementations in opentelemetry-collector-contrib, for how we want to handle this special case.

Hope that helps. Please ping if any Qs

[tigrannajaryan]

The OpenTelemetry translation of zipkinv2 spans to OTLP is Incorrect and needs to be changed to account for the error tag.

@ericmustin I believe this ^^^^ is the right approach.

@owais please confirm that the "error" tag is indeed what SignalFx client libraries set when exporting in Zipkin format.

[owais]

I can confirm. SFX node library may set the error tag to any string value to represent a failed operation. If the error tag is present, it should translate to failed status in Otel IMO.

[tigrannajaryan]

@ericmustin we have the confirmation. I believe the next step is to fix zipkin to internal pdata translation to account for error tag. Will you be able to submit a fix?

[ericmustin]

@tigrannajaryan sure i can give it a shot, i don't claim to be the cosmos on zipkin but i'll try to push something up in the next day or so

[ericmustin]

One question...It looks like semantic conventions to reflect this are being added in this PR, which is still open open-telemetry/opentelemetry-specification#1257 ... @tigrannajaryan am i ok to make a PR to the collector accounting for the error tag before the semantic convention PR gets merged?

[tigrannajaryan]

@ericmustin you are right, let's wait for open-telemetry/opentelemetry-specification#1257 to be finalized.

[andrewhsu]

from the sig mtg, assigning to eric

[tigrannajaryan]

Fixed by #2253