Verify and update jaeger and zipkin exporters for error code handling.

[jkwatson]

This PR in the specs updated how span errors should be handled in jaeger & zipkin: open-telemetry/opentelemetry-specification#1257

We should verify that our code is doing what it should and update it if it isn't.

[ChrisJBurns]

I can get stuck into this one for a good second issue 👍🏼

[ChrisJBurns]

I am assuming for this, I am more specifically looking at the exporters/jaeger exporters/jaeger-thrift and exporters/zipkin modules?

[ChrisJBurns]

So, for brevity, clarity and future reference I'll condense all of the details on this issue with regards to my findings.

Jaeger

The error handling has been detailed in the jaeger exporter specification.

When Span Status is set to ERROR an error tag MUST be added with the Boolean value of true. The added error tag MAY override any previous value.

Jaeger-gRPC

This has been consistently implemented in the following code:

if (span.getStatus().getStatusCode() == StatusCode.ERROR) {
    target.addTags(toKeyValue(KEY_ERROR, true));
}

Jaeger-Thrift

The same has been done in the jaeger-thrift Adapter.java class:

if (span.getStatus().getStatusCode() == StatusCode.ERROR) {
    tags.add(toTag(KEY_ERROR, true));
}

Zipkin

The Error handling has been detailed in the zipkin exporter specification. However, being pedantic, unlike the jaeger specification, there is not a sub-heading of error-flag within the Status section. Instead it's just a note Not sure if this should be updated to be kept consistent across both exporter README specifications. Is very trivial.

Note: The error tag should only be set if Status is Error. If a boolean version ({"error":false} or {"error":"false"}) is present, it SHOULD be removed. Zipkin will treat any span with error sent as failed.

Zipkin exporter

It seems that the code doesn't exactly do what the specification mandates. The specification says that the description field is only set IF there is an error. Additionally, it doesn't say to use the otel.status_description tag, instead it says to set the error tag. Which means the following code isn't exactly correct as it sets the description if it is not null and doesn't do the error check:

if (!status.isUnset()) {
    spanBuilder.putTag(OTEL_STATUS_CODE, status.getStatusCode().toString());
    if (status.getDescription() != null) {
    spanBuilder.putTag(OTEL_STATUS_DESCRIPTION, status.getDescription());
    }
}

I have got extra questions around this as I believe there is a slight inconsistency between the jaeger and zipkin specification details. I have added a comment to the original PR that updated the exporter error handling specifications and am awaiting a reply with regarding to the confirmation of my query.

Lastly, depending on if there are any updates in the specifications on the back of my queries, the code I've referenced above may need to change slightly with regards to the description condition being moved inside an error status check. I will check back once I've got a reply. 👍

[ChrisJBurns]

Have had a response back to my query, it seems that there are some variances within the jaeger and zipkin implementations which is why there are differences in the specifications.

So on the back of the above, it seems that the jaeger exporter error handling implementation works as expected. But the zipkin implementation will need a slight tweak.

The Zipkin specification says that the error tag:

Description of the Status. MUST be set if the code is ERROR, use an empty string if Description has no value. MUST NOT be set for OK and UNSET codes.

The ZipkinSpanExporter.java code currently does the following:

// include status code & description.
if (!status.isUnset()) {
    spanBuilder.putTag(OTEL_STATUS_CODE, status.getStatusCode().toString());
    if (status.getDescription() != null) {
    spanBuilder.putTag(OTEL_STATUS_DESCRIPTION, status.getDescription());
    }
}
// add the error tag, if it isn't already in the source span.
if (!status.isOk() && spanAttributes.get(STATUS_ERROR) == null) {
    spanBuilder.putTag(STATUS_ERROR.getKey(), status.getStatusCode().toString());
}

So it essentially sets both the otel.status_code and otel.status_description if the status is NOT unset. Below this, it then checks if the status is NOT OK and if there is an error status in the span - to which if true, it will set the error tag to the statusCode.

I believe the changes we have to make to make sure this adheres to the zipkin specification is to move the error tag setting into the condition above and to tweak the value to be the description instead of the statusCode - whilst also removing any setting of the otel.status_description tag. Like so:

// include status code & error.
if (!status.isUnset()) {
    spanBuilder.putTag(OTEL_STATUS_CODE, status.getStatusCode().toString());

    // add the error tag, if it isn't already in the source span.
    if (!status.isOk() && spanAttributes.get(STATUS_ERROR) == null) {
        spanBuilder.putTag(STATUS_ERROR.getKey(), getValueOrReturnEmpty(status.getDescription()));
    }
}

...

private static String getValueOrReturnEmpty(String value) {
    return value != null ? value : "";
}

Welcome your thoughts 👍

[anuraaga]

@ChrisJBurns Thanks for the summary, that proposed change seems good to me