Skip to content
This repository was archived by the owner on Dec 23, 2023. It is now read-only.

Exporter/Jaeger: Set span status as tags.#1928

Merged
songy23 merged 3 commits intocensus-instrumentation:masterfrom
songy23:jaeger-status-tag
Jun 11, 2019
Merged

Exporter/Jaeger: Set span status as tags.#1928
songy23 merged 3 commits intocensus-instrumentation:masterfrom
songy23:jaeger-status-tag

Conversation

@songy23
Copy link
Copy Markdown
Contributor

@songy23 songy23 commented Jun 10, 2019

@songy23 songy23 requested review from a team, dinooliva and rghetia as code owners June 10, 2019 22:29
@songy23 songy23 requested a review from pjanotti June 10, 2019 22:30
rghetia
rghetia reviewed Jun 10, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@rghetia rghetia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one nit. LGTM otherwise.

Comment thread ...race/jaeger/src/test/java/io/opencensus/exporter/trace/jaeger/JaegerExporterHandlerTest.java Outdated
new Tag("span.kind", TagType.STRING).setVStr("server"),
new Tag("status.code", TagType.LONG).setVLong(4),
new Tag("status.message", TagType.STRING).setVStr(statusMessage),
new Tag("error", TagType.BOOL).setVBool(true));
Copy link
Copy Markdown
Contributor

@rghetia rghetia Jun 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:s/status.code/STATUS_CODE/
same with other strings.

Copy link
Copy Markdown
Contributor Author

@songy23 songy23 Jun 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SG, fixed in c7816a7.

@pjanotti
Copy link
Copy Markdown

pjanotti commented Jun 11, 2019

Thanks for putting out a fix so quickly @songy23!

I like mapping Status to status.code and status.message.

I don't agree with automatically setting error to true as explained in the conversation at census-instrumentation/opencensus-go#1041 (comment) I still think that this is a decision that should be made by the application itself. Automatically setting error to true will tend to mark most spans as of trace as "error" making it redundant. This comment here defends the same position opentracing/specification#96 (comment)

@pjanotti
Copy link
Copy Markdown

pjanotti commented Jun 11, 2019

The Zipkin exporter also should use the same OT tags (status.code and status.message) instead of the custom one (census.statu_code).

@songy23
Copy link
Copy Markdown
Contributor Author

songy23 commented Jun 11, 2019

@pjanotti Thanks for the context. If it's the convention to set the error tag at application level, I'm good with not having it automatically set. Removed in 652f7ff.

Updates to Zipkin will be in another PR.

@pjanotti
Copy link
Copy Markdown

pjanotti commented Jun 11, 2019

Thanks @songy23! LGTM.

@songy23 songy23 mentioned this pull request Jun 11, 2019
Merged
@songy23 songy23 merged commit 48106bc into census-instrumentation:master Jun 11, 2019
@songy23 songy23 deleted the jaeger-status-tag branch June 11, 2019 17:01
@pjanotti pjanotti mentioned this pull request Jun 27, 2019
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@dinooliva dinooliva Awaiting requested review from dinooliva

2 more reviewers

@pjanotti pjanotti pjanotti approved these changes

@rghetia rghetia rghetia approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes exporters tracing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

OC Status info is lost when exporting to Jaeger

4 participants

@songy23 @pjanotti @rghetia @googlebot