Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Zipkin annotations if the timestamp is zero #1341

Merged
merged 2 commits into from
Feb 13, 2019

Conversation

geobeau
Copy link
Contributor

@geobeau geobeau commented Feb 13, 2019

Which problem is this PR solving?

When the span.Timestamp is 0, jaeger write the span with the timestamp to zero even when we have more informations in the annotations.

Short description of the changes

If the timestamp is zero, we determine if the span is a client or a server and extract timestamp and information from there

Copy link
Contributor

@black-adder black-adder left a comment

Choose a reason for hiding this comment

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

thanks!

if cs != nil {
timestamp = cs.Timestamp
cr := td.findAnnotation(zSpan, zipkincore.CLIENT_RECV)
if cr != nil {
Copy link
Member

Choose a reason for hiding this comment

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

should this perhaps also check for duration == 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@codecov
Copy link

codecov bot commented Feb 13, 2019

Codecov Report

Merging #1341 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1341   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         162     162           
  Lines        7308    7326   +18     
======================================
+ Hits         7308    7326   +18
Impacted Files Coverage Δ
model/converter/thrift/zipkin/to_domain.go 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e96fe91...9e48960. Read the comment docs.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

👏

@yurishkuro yurishkuro merged commit 9953c4d into jaegertracing:master Feb 13, 2019
@yurishkuro
Copy link
Member

@geobeau thanks for the PR? Was there a related ticket, or just the PR?

@geobeau
Copy link
Contributor Author

geobeau commented Feb 13, 2019

No, I encountered this issue so I fixed it directly. Should I have opened one?

Thanks to both of you 👍

@yurishkuro
Copy link
Member

no, PR is fine, thanks.

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

Successfully merging this pull request may close these issues.

3 participants