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

Do not remove leading zeros in trace ID hex string #1956

Merged
merged 12 commits into from
Dec 4, 2019

Conversation

yurishkuro
Copy link
Member

@yurishkuro yurishkuro commented Nov 30, 2019

Resolves #1578 and probably #1951

Motivation

Among randomly generated and uniformly distributed trace IDs, only 1/16th of them start with 0 followed by a significant digit, 1/256th start with two 0s, and so on in decreasing geometric progression. Therefore, trimming the leading 0s is a very modest optimization on the size of the data being transmitted or stored.

However, trimming 0s leads to ambiguities when the IDs are used as correlations with other monitoring systems, such as logging, that treat the IDs as opaque strings and cannot establish the equivalence between padded and unpadded IDs. It is also incompatible with W3C Trace Context and Zipkin B3 format, both of which include all leading 0s, so an application instrumented with OpenTelemetry SDKs may be logging different trace ID strings than application instrumented with Jaeger SDKs (related issue #1657).

Open question: this change may prevent ES backend from reading the old traces if their IDs had leading zeroes, because previously the zeroes were not added to string representation, and now they are. Roughly one in 16 traces will be affected.

@codecov
Copy link

codecov bot commented Nov 30, 2019

Codecov Report

Merging #1956 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1956      +/-   ##
==========================================
+ Coverage   98.46%   98.46%   +<.01%     
==========================================
  Files         199      199              
  Lines        9875     9890      +15     
==========================================
+ Hits         9723     9738      +15     
- Misses        115      116       +1     
+ Partials       37       36       -1
Impacted Files Coverage Δ
plugin/storage/es/spanstore/reader.go 100% <100%> (ø) ⬆️
model/ids.go 100% <100%> (ø) ⬆️
plugin/storage/badger/spanstore/reader.go 96.79% <0%> (ø) ⬆️

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 85b2d2a...bcaeb58. Read the comment docs.

@yurishkuro
Copy link
Member Author

@pavolloffay please review, and any thoughts on the ES question? One option is to add a backwards compatibility mode by doing a fallback search with unpadded trace ID strings if full-length strings are not found. It would be an option people can configure.

@yurishkuro
Copy link
Member Author

cc @jpkrohling (I think Pavol is on PTO)

@pavolloffay
Copy link
Member

I will have a look at this one, hod for the moment

@pavolloffay
Copy link
Member

Open question: this change may prevent ES backend from reading the old traces if their IDs had leading zeroes, because previously the zeroes were not added to string representation, and now they are. Roughly one in 16 traces will be affected.

Indeed, spans previously stored with with id like 1 are not searchable after this change. We definitely need a way to be backward compatible.

@pavolloffay
Copy link
Member

Something like this should work https://github.com/yurishkuro/jaeger/pull/1/files it needs some tests though.

@yurishkuro could you please summarize why adding leading zeros is necessary?

@yurishkuro
Copy link
Member Author

@pavolloffay I added motivation to description.

@pavolloffay
Copy link
Member

thanks @yurishkuro. It makes sense. I have updated my PR to your branch.

func convertTraceIDsStringsToModels(traceIDs []string) ([]model.TraceID, error) {
traceIDsModels := make([]model.TraceID, len(traceIDs))
for i, ID := range traceIDs {
traceIDsMap := map[model.TraceID]bool{}
Copy link
Member

Choose a reason for hiding this comment

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

I forgot to add a TODO comment here similar to one in buildTraceByIDQuery

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add it. Please approve if you're otherwise ok with the change.

@pavolloffay pavolloffay changed the title Do not remote leading zeros in trace ID hex string Do not remove leading zeros in trace ID hex string Dec 4, 2019
@pavolloffay pavolloffay added this to the Release 1.16 milestone Dec 4, 2019
@pavolloffay
Copy link
Member

We should list this in breaking changes as it might impact integrations

Yuri Shkuro and others added 9 commits December 4, 2019 13:53
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
pavolloffay and others added 3 commits December 4, 2019 13:53
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
@yurishkuro yurishkuro merged commit 95331ad into jaegertracing:master Dec 4, 2019
@yurishkuro yurishkuro deleted the fix-1578 branch December 4, 2019 21:13
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.

[jaeger-query] Leading zeros stripped from trace IDs
2 participants