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

Do not strip leading zeros from trace IDs and span IDs #259

Merged
merged 3 commits into from
Jan 29, 2021
Merged

Do not strip leading zeros from trace IDs and span IDs #259

merged 3 commits into from
Jan 29, 2021

Conversation

tobiasstadler
Copy link
Contributor

Which problem is this PR solving?

@AppVeyorBot
Copy link

Signed-off-by: Tobias Stadler <ts.stadler@gmx.de>
Signed-off-by: Tobias Stadler <ts.stadler@gmx.de>
@AppVeyorBot
Copy link

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.

Q: since you're looking into this, I just want too make sure there is a unit test that ensures that this library still CAN consume non-padded IDs from the incoming request headers

out << std::hex << _high << std::setw(16) << std::setfill('0')
<< std::hex << _low;
}
out << std::setw(16) << std::setfill('0') << std::hex << _high
Copy link
Member

Choose a reason for hiding this comment

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

this does not seem backwards compatible, what's the reason for removing if (_high == 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I though the trace id is now always 32 hex chars long. But I was wrong.

Copy link
Member

Choose a reason for hiding this comment

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

No, all Jaeger SDKs still use 64bit by default, you have to explicitly enable 128bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, noticed that after your comment. I misread the prs for the other sdks.

@tobiasstadler
Copy link
Contributor Author

Q: since you're looking into this, I just want too make sure there is a unit test that ensures that this library still CAN consume non-padded IDs from the incoming request headers

TEST(SpanContext, testFromStream) tests both padded and unpadded trace ids

Signed-off-by: Tobias Stadler <ts.stadler@gmx.de>
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.

Thanks!

@AppVeyorBot
Copy link

@yurishkuro yurishkuro merged commit fd2989a into jaegertracing:master Jan 29, 2021
lrouquette pushed a commit to lrouquette/jaeger-client-cpp that referenced this pull request Nov 2, 2021
…#259)

* Do not strip leading zeros from trace IDs

Signed-off-by: Tobias Stadler <ts.stadler@gmx.de>

* Do not strip leading zeros from span IDs

Signed-off-by: Tobias Stadler <ts.stadler@gmx.de>

* high should only be used if it is non zero

Signed-off-by: Tobias Stadler <ts.stadler@gmx.de>
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