Skip to content
This repository has been archived by the owner on Nov 7, 2022. It is now read-only.

Status is dropped when translating spans to Jaeger TChannel spans #586

Open
asutoshpalai opened this issue Jun 19, 2019 · 8 comments
Open
Assignees

Comments

@asutoshpalai
Copy link
Contributor

The status field and its message is not added to the spans as tags while translating the spans from the OpenCensus span format to TChannel span format at translator/trace/jaeger/protospan_to_jaegerthrift.go#L148.

This is done correctly in protospan_to_jaegerproto.go and Jaeger exporter for OpenCensus.

@pjanotti
Copy link

Thanks for reporting @asutoshpalai. /cc @owais

@owais
Copy link
Contributor

owais commented Jun 19, 2019

We should have a fix for this in a few days @asutoshpalai

@flands flands added this to the 0.1.9 milestone Jun 21, 2019
@asutoshpalai
Copy link
Contributor Author

@owais Have you started working on this? If not, can I please pick this up?

@owais
Copy link
Contributor

owais commented Jun 26, 2019

@asutoshpalai Yes, I'm almost done with it. Will submit a patch in a couple of days.

@asutoshpalai
Copy link
Contributor Author

I was going through the code of jaegerproto, it looks like they don't set error tag when the status is not OK. This is not consistent with the behavior of Jaeger exporter for OpenCensus which does that, census-ecosystem/opencensus-go-exporter-jaeger/blob/master/jaeger.go#L217.

Ref: census-instrumentation/opencensus-go#1041

I would really appreciate it if @owais can include that in his fix for thrift protocol too.

@pjanotti
Copy link

pjanotti commented Jun 27, 2019

One important observation here: the behavior on opencensus-go is not the recommended one for OpenTracing (which Jaeger follows). The error tag should be explictly set by the caller code. I had mentioned this on the opencensus-go repo and this was recently changed on java client library. Relevant link:

census-instrumentation/opencensus-java#1928 (comment)

@owais
Copy link
Contributor

owais commented Jun 30, 2019

We have a PR for OT here: open-telemetry/opentelemetry-collector#70

Once it is accepted there, I'll back-port the fix to OC.

@owais
Copy link
Contributor

owais commented Jul 2, 2019

We've merged it to OT. Will try to backport here later this week.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants