Do not remove leading zeros in trace ID hex string #1956
Conversation
yurishkuro
requested review from
black-adder,
jpkrohling,
objectiser,
pavolloffay,
tiffon and
vprithvi
as code owners
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
@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. |
|
cc @jpkrohling (I think Pavol is on PTO) |
|
I will have a look at this one, hod for the moment |
Indeed, spans previously stored with with id like |
|
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? |
|
@pavolloffay I added motivation to description. |
|
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{} |
There was a problem hiding this comment.
I forgot to add a TODO comment here similar to one in buildTraceByIDQuery
There was a problem hiding this comment.
I'll add it. Please approve if you're otherwise ok with the change.
pavolloffay
changed the title
|
We should list this in breaking changes as it might impact integrations |
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>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
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.