Skip to content
This repository has been archived by the owner on May 23, 2024. It is now read-only.

Add support for 128bit trace ids to zipkin thrift spans. #378

Merged
merged 5 commits into from
Feb 28, 2019

Conversation

douglas-reid
Copy link
Contributor

@douglas-reid douglas-reid commented Feb 23, 2019

Includes:

  • Updating IDL to 3471ffb
  • Running make thrift
  • Add TraceIDHigh to zipkin_thrift_span

Note: I manually verified this by running a binary with these changes compiled in, as illustrated in istio/istio#11954 (comment).

Signed-off-by: Douglas Reid [email protected]

Which problem is this PR solving?

  • Zipkin trace export drops the high order 64bits from trace ids

Short description of the changes

  • updated the idl submodule, ran the thrift-gen, added a few lines to include the new field.

Includes:
- Updating IDL to master
- Running make thrift
- Add TraceIDHigh to zipkin_thrift_span

Signed-off-by: Douglas Reid <[email protected]>
@codecov
Copy link

codecov bot commented Feb 23, 2019

Codecov Report

Merging #378 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #378      +/-   ##
==========================================
+ Coverage   87.29%   87.31%   +0.02%     
==========================================
  Files          54       54              
  Lines        3006     3011       +5     
==========================================
+ Hits         2624     2629       +5     
  Misses        271      271              
  Partials      111      111
Impacted Files Coverage Δ
zipkin_thrift_span.go 77.48% <100%> (+0.6%) ⬆️

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 64f5786...41b3e3a. Read the comment docs.

@douglas-reid
Copy link
Contributor Author

@yurishkuro is there any more that needs to be done here?

@duderino
Copy link

@yurishkuro we'd love to get this fix into Istio 1.1 release. anything else you need here?

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.

Sorry, been swamped with final reviews for my book.

I am ready to merge this, but I would like to roll back the version of the IDL to something before the incomplete flag was added to jaeger.thrift (I am about to revert that in jaegertracing/jaeger-idl#53).

For example, pin the idl subrepo to commit 3471ffb.

Signed-off-by: Douglas Reid <[email protected]>
Signed-off-by: Douglas Reid <[email protected]>
@douglas-reid
Copy link
Contributor Author

@yurishkuro no worries. I've pinned the IDL to the requested commit, rerun make thrift and pushed the changes. please let me know if there is more I should do.

@yurishkuro yurishkuro merged commit ecf2d03 into jaegertracing:master Feb 28, 2019
@douglas-reid
Copy link
Contributor Author

@yurishkuro many thanks!

@duderino
Copy link

@yurishkuro thank you

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

Successfully merging this pull request may close these issues.

3 participants