Keep leading zeros in trace ID hex string (jaegertracing#1956)

* Do not remote leading zeros in trace ID hex string Signed-off-by: Yuri Shkuro <[email protected]> * Fix test in ./cmd/query/app Signed-off-by: Yuri Shkuro <[email protected]> * Fix tests in ./model Signed-off-by: Yuri Shkuro <[email protected]> * Fix tests in ./model/converter/json Signed-off-by: Yuri Shkuro <[email protected]> * Fix tests in ./plugin/storage/cassandra/spanstore Signed-off-by: Yuri Shkuro <[email protected]> * Fix tests in ./plugin/storage/cassandra/spanstore/dbmodel Signed-off-by: Yuri Shkuro <[email protected]> * Fix tests in ./plugin/storage/es/spanstore Signed-off-by: Yuri Shkuro <[email protected]> * Zero-pad SpanID as well Signed-off-by: Yuri Shkuro <[email protected]> * Read trace ID from ES without leading zeros Signed-off-by: Pavol Loffay <[email protected]> * Review comments Signed-off-by: Pavol Loffay <[email protected]> * Cleanup tests Signed-off-by: Yuri Shkuro <[email protected]> * Add to changelog Signed-off-by: Yuri Shkuro <[email protected]>
ilyamor · Dec 4, 2019 · 95331ad · 95331ad
1 parent 85b2d2a
commit 95331ad
Show file tree

Hide file tree

Showing 13 changed files with 172 additions and 73 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,11 +4,14 @@ Changes by Version
 1.16.0 (unreleased)
 ------------------
 
-#### Backend Changes
+### Breaking Changes
 
-##### Breaking Changes
+#### List of service operations can be classified by span kinds
+
+([PR #1943](https://github.com/jaegertracing/jaeger/pull/1943), [@guo0693](https://github.com/guo0693))
+
+TODO: fix formatting/indentation below
 
-* Changes to the endpoints that returns a list of operations ([#1943](https://github.com/jaegertracing/jaeger/pull/1943), [@guo0693](https://github.com/guo0693))
     * Endpoint changes:
         * Both Http & gRPC servers now take new optional parameter `spanKind` in addition to `service`. When spanKind
          is absent or empty, operations from all kinds of spans will be returned.
@@ -54,11 +57,39 @@ Changes by Version
                 `spanKind` column will be empty for those data.  
                 At the end, it will ask you whether you want to drop the old table or not.
             2. Restart ingester & query services so that they begin to use the new table
-##### New Features
 
-##### Bug fixes, Minor Improvements
+#### Trace and Span IDs are always padded to 32 or 16 hex characters with leading zeros
 
-#### UI Changes
+([PR #1956](https://github.com/jaegertracing/jaeger/pull/1956), [@yurishkuro](https://github.com/yurishkuro))
+
+Previously, Jaeger backend always rendered trace and span IDs as  the shortest possible hex string, e.g. an ID
+with numeric value 255 would be rendered as a string `ff`. This change makes the IDs to always render as 16 or 32
+characters long hex string, e.g. the same id=255 would render as `00000000000000ff`. It mostly affects how UI
+displays the IDs, the URLs, and the JSON returned from `jaeger-query` service.
+
+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 formats, 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).
+
+Overall, the change is backward compatible:
+  * links with non-padded IDs in the UI will still work
+  * data stored in Elasticsearch (where IDs are represented as strings) is still readable
+
+However, some custom integration that rely on exact string matches of trace IDs may be broken.
+
+### Backend Changes
+
+#### New Features
+
+#### Bug fixes, Minor Improvements
+
+### UI Changes
 
 
 1.15.1 (2019-11-07)

diff --git a/cmd/query/app/http_handler_test.go b/cmd/query/app/http_handler_test.go
@@ -380,7 +380,7 @@ func TestSearchByTraceIDNotFound(t *testing.T) {
 	err := getJSON(server.URL+`/api/traces?traceID=1`, &response)
 	assert.NoError(t, err)
 	assert.Len(t, response.Errors, 1)
-	assert.Equal(t, structuredError{Msg: "trace not found", TraceID: ui.TraceID("1")}, response.Errors[0])
+	assert.Equal(t, structuredError{Msg: "trace not found", TraceID: ui.TraceID("0000000000000001")}, response.Errors[0])
 }
 
 func TestSearchByTraceIDFailure(t *testing.T) {

diff --git a/model/adjuster/clockskew_test.go b/model/adjuster/clockskew_test.go
@@ -84,7 +84,7 @@ func TestClockSkewAdjuster(t *testing.T) {
 			trace: []spanProto{
 				{id: 1, parent: 99, startTime: 0, duration: 100, host: "a", adjusted: 0},
 			},
-			err: "invalid parent span IDs=63; skipping clock skew adjustment", // 99 == 0x63
+			err: "invalid parent span IDs=0000000000000063; skipping clock skew adjustment", // 99 == 0x63
 		},
 		{
 			description: "single span with empty host key",

diff --git a/model/converter/json/fixtures/es_01.json b/model/converter/json/fixtures/es_01.json
@@ -1,23 +1,23 @@
 {
-  "traceID": "1",
-  "spanID": "2",
+  "traceID": "0000000000000001",
+  "spanID": "0000000000000002",
   "flags": 1,
   "operationName": "test-general-conversion",
   "references": [
     {
       "refType": "CHILD_OF",
-      "traceID": "1",
-      "spanID": "3"
+      "traceID": "0000000000000001",
+      "spanID": "0000000000000003"
     },
     {
       "refType": "FOLLOWS_FROM",
-      "traceID": "1",
-      "spanID": "4"
+      "traceID": "0000000000000001",
+      "spanID": "0000000000000004"
     },
     {
       "refType": "CHILD_OF",
-      "traceID": "ff",
-      "spanID": "ff"
+      "traceID": "00000000000000ff",
+      "spanID": "00000000000000ff"
     }
   ],
   "startTime": 1485467191639875,

diff --git a/model/converter/json/fixtures/ui_01.json b/model/converter/json/fixtures/ui_01.json
@@ -1,9 +1,9 @@
 {
-  "traceID": "1",
+  "traceID": "0000000000000001",
   "spans": [
     {
-      "traceID": "1",
-      "spanID": "2",
+      "traceID": "0000000000000001",
+      "spanID": "0000000000000002",
       "operationName": "test-general-conversion",
       "references": [],
       "startTime": 1485467191639875,
@@ -35,8 +35,8 @@
       "warnings": null
     },
     {
-      "traceID": "1",
-      "spanID": "2",
+      "traceID": "0000000000000001",
+      "spanID": "0000000000000002",
       "operationName": "some-operation",
       "references": [],
       "startTime": 1485467191639875,
@@ -73,14 +73,14 @@
       "warnings": null
     },
     {
-      "traceID": "1",
-      "spanID": "3",
+      "traceID": "0000000000000001",
+      "spanID": "0000000000000003",
       "operationName": "some-operation",
       "references": [
         {
           "refType": "CHILD_OF",
-          "traceID": "1",
-          "spanID": "2"
+          "traceID": "0000000000000001",
+          "spanID": "0000000000000002"
         }
       ],
       "startTime": 1485467191639875,
@@ -91,24 +91,24 @@
       "warnings": null
     },
     {
-      "traceID": "1",
-      "spanID": "4",
+      "traceID": "0000000000000001",
+      "spanID": "0000000000000004",
       "operationName": "reference-test",
       "references": [
         {
           "refType": "CHILD_OF",
-          "traceID": "ff",
-          "spanID": "ff"
+          "traceID": "00000000000000ff",
+          "spanID": "00000000000000ff"
         },
         {
           "refType": "CHILD_OF",
-          "traceID": "1",
-          "spanID": "2"
+          "traceID": "0000000000000001",
+          "spanID": "0000000000000002"
         },
         {
           "refType": "FOLLOWS_FROM",
-          "traceID": "1",
-          "spanID": "2"
+          "traceID": "0000000000000001",
+          "spanID": "0000000000000002"
         }
       ],
       "startTime": 1485467191639875,
@@ -121,14 +121,14 @@
       ]
     },
     {
-      "traceID": "1",
-      "spanID": "5",
+      "traceID": "0000000000000001",
+      "spanID": "0000000000000005",
       "operationName": "preserveParentID-test",
       "references": [
         {
           "refType": "CHILD_OF",
-          "traceID": "1",
-          "spanID": "4"
+          "traceID": "0000000000000001",
+          "spanID": "0000000000000004"
         }
       ],
       "startTime": 1485467191639875,

diff --git a/model/ids.go b/model/ids.go
@@ -49,9 +49,9 @@ func NewTraceID(high, low uint64) TraceID {
 
 func (t TraceID) String() string {
 	if t.High == 0 {
-		return fmt.Sprintf("%x", t.Low)
+		return fmt.Sprintf("%016x", t.Low)
 	}
-	return fmt.Sprintf("%x%016x", t.High, t.Low)
+	return fmt.Sprintf("%016x%016x", t.High, t.Low)
 }
 
 // TraceIDFromString creates a TraceID from a hexadecimal string
@@ -163,7 +163,7 @@ func NewSpanID(v uint64) SpanID {
 }
 
 func (s SpanID) String() string {
-	return fmt.Sprintf("%x", uint64(s))
+	return fmt.Sprintf("%016x", uint64(s))
 }
 
 // SpanIDFromString creates a SpanID from a hexadecimal string

diff --git a/model/span_test.go b/model/span_test.go
@@ -35,12 +35,12 @@ var testCasesTraceID = []struct {
 	hex    string
 	b64    string
 }{
-	{lo: 1, hex: "1", b64: "AAAAAAAAAAAAAAAAAAAAAQ=="},
-	{lo: 15, hex: "f", b64: "AAAAAAAAAAAAAAAAAAAADw=="},
-	{lo: 31, hex: "1f", b64: "AAAAAAAAAAAAAAAAAAAAHw=="},
-	{lo: 257, hex: "101", b64: "AAAAAAAAAAAAAAAAAAABAQ=="},
-	{hi: 1, lo: 1, hex: "10000000000000001", b64: "AAAAAAAAAAEAAAAAAAAAAQ=="},
-	{hi: 257, lo: 1, hex: "1010000000000000001", b64: "AAAAAAAAAQEAAAAAAAAAAQ=="},
+	{lo: 1, hex: "0000000000000001", b64: "AAAAAAAAAAAAAAAAAAAAAQ=="},
+	{lo: 15, hex: "000000000000000f", b64: "AAAAAAAAAAAAAAAAAAAADw=="},
+	{lo: 31, hex: "000000000000001f", b64: "AAAAAAAAAAAAAAAAAAAAHw=="},
+	{lo: 257, hex: "0000000000000101", b64: "AAAAAAAAAAAAAAAAAAABAQ=="},
+	{hi: 1, lo: 1, hex: "00000000000000010000000000000001", b64: "AAAAAAAAAAEAAAAAAAAAAQ=="},
+	{hi: 257, lo: 1, hex: "00000000000001010000000000000001", b64: "AAAAAAAAAQEAAAAAAAAAAQ=="},
 }
 
 func TestTraceIDMarshalJSONPB(t *testing.T) {
@@ -111,10 +111,10 @@ var testCasesSpanID = []struct {
 	hex string
 	b64 string
 }{
-	{id: 1, hex: "1", b64: "AAAAAAAAAAE="},
-	{id: 15, hex: "f", b64: "AAAAAAAAAA8="},
-	{id: 31, hex: "1f", b64: "AAAAAAAAAB8="},
-	{id: 257, hex: "101", b64: "AAAAAAAAAQE="},
+	{id: 1, hex: "0000000000000001", b64: "AAAAAAAAAAE="},
+	{id: 15, hex: "000000000000000f", b64: "AAAAAAAAAA8="},
+	{id: 31, hex: "000000000000001f", b64: "AAAAAAAAAB8="},
+	{id: 257, hex: "0000000000000101", b64: "AAAAAAAAAQE="},
 	{id: uint64(maxSpanID), hex: "ffffffffffffffff", b64: "//////////8="},
 }
 

diff --git a/plugin/storage/cassandra/spanstore/dbmodel/model_test.go b/plugin/storage/cassandra/spanstore/dbmodel/model_test.go
@@ -30,5 +30,5 @@ func TestTagInsertionString(t *testing.T) {
 
 func TestTraceIDString(t *testing.T) {
 	id := TraceIDFromDomain(model.NewTraceID(1, 1))
-	assert.Equal(t, "10000000000000001", id.String())
+	assert.Equal(t, "00000000000000010000000000000001", id.String())
 }
diff --git a/plugin/storage/cassandra/spanstore/reader_test.go b/plugin/storage/cassandra/spanstore/reader_test.go
@@ -279,8 +279,8 @@ func TestSpanReaderFindTraces(t *testing.T) {
 			expectedLogs: []string{
 				"Failure to read trace",
 				"Error reading traces from storage: load query error",
-				`"trace_id":"1"`,
-				`"trace_id":"2"`,
+				`"trace_id":"0000000000000001"`,
+				`"trace_id":"0000000000000002"`,
 			},
 		},
 	}

diff --git a/plugin/storage/cassandra/spanstore/writer_test.go b/plugin/storage/cassandra/spanstore/writer_test.go
@@ -94,7 +94,7 @@ func TestSpanWriter(t *testing.T) {
 				`"query":"select from traces"`,
 				`"error":"main query error"`,
 				"Failed to insert span",
-				`"trace_id":"1"`,
+				`"trace_id":"0000000000000001"`,
 				`"span_id":0`,
 			},
 		},

diff --git a/plugin/storage/es/spanstore/dbmodel/fixtures/es_01.json b/plugin/storage/es/spanstore/dbmodel/fixtures/es_01.json
@@ -1,23 +1,23 @@
 {
-  "traceID": "1",
-  "spanID": "2",
+  "traceID": "0000000000000001",
+  "spanID": "0000000000000002",
   "flags": 1,
   "operationName": "test-general-conversion",
   "references": [
     {
       "refType": "CHILD_OF",
-      "traceID": "1",
-      "spanID": "3"
+      "traceID": "0000000000000001",
+      "spanID": "0000000000000003"
     },
     {
       "refType": "FOLLOWS_FROM",
-      "traceID": "1",
-      "spanID": "4"
+      "traceID": "0000000000000001",
+      "spanID": "0000000000000004"
     },
     {
       "refType": "CHILD_OF",
-      "traceID": "ff",
-      "spanID": "ff"
+      "traceID": "00000000000000ff",
+      "spanID": "00000000000000ff"
     }
   ],
   "startTime": 1485467191639875,

diff --git a/plugin/storage/es/spanstore/reader.go b/plugin/storage/es/spanstore/reader.go
@@ -337,7 +337,7 @@ func (s *SpanReader) multiRead(ctx context.Context, traceIDs []model.TraceID, st
 		}
 		searchRequests := make([]*elastic.SearchRequest, len(traceIDs))
 		for i, traceID := range traceIDs {
-			query := elastic.NewTermQuery("traceID", traceID.String())
+			query := buildTraceByIDQuery(traceID)
 			if val, ok := searchAfterTime[traceID]; ok {
 				nextTime = val
 			}
@@ -393,15 +393,42 @@ func (s *SpanReader) multiRead(ctx context.Context, traceIDs []model.TraceID, st
 	return traces, nil
 }
 
+func buildTraceByIDQuery(traceID model.TraceID) elastic.Query {
+	traceIDStr := traceID.String()
+	if traceIDStr[0] != '0' {
+		return elastic.NewTermQuery(traceIDField, traceIDStr)
+	}
+	// https://github.com/jaegertracing/jaeger/pull/1956 added leading zeros to IDs
+	// So we need to also read IDs without leading zeros for compatibility with previously saved data.
+	// TODO remove in newer versions, added in Jaeger 1.16
+	var legacyTraceID string
+	if traceID.High == 0 {
+		legacyTraceID = fmt.Sprintf("%x", traceID.Low)
+	} else {
+		legacyTraceID = fmt.Sprintf("%x%016x", traceID.High, traceID.Low)
+	}
+	return elastic.NewBoolQuery().Should(
+		elastic.NewTermQuery(traceIDField, traceIDStr).Boost(2),
+		elastic.NewTermQuery(traceIDField, legacyTraceID))
+}
+
 func convertTraceIDsStringsToModels(traceIDs []string) ([]model.TraceID, error) {
-	traceIDsModels := make([]model.TraceID, len(traceIDs))
-	for i, ID := range traceIDs {
+	traceIDsMap := map[model.TraceID]bool{}
+	// https://github.com/jaegertracing/jaeger/pull/1956 added leading zeros to IDs
+	// So we need to also read IDs without leading zeros for compatibility with previously saved data.
+	// That means the input to this function may contain logically identical trace IDs but formatted
+	// with or without padding, and we need to dedupe them.
+	// TODO remove deduping in newer versions, added in Jaeger 1.16
+	traceIDsModels := make([]model.TraceID, 0, len(traceIDs))
+	for _, ID := range traceIDs {
 		traceID, err := model.TraceIDFromString(ID)
 		if err != nil {
 			return nil, errors.Wrap(err, fmt.Sprintf("Making traceID from string '%s' failed", ID))
 		}
-
-		traceIDsModels[i] = traceID
+		if _, ok := traceIDsMap[traceID]; !ok {
+			traceIDsMap[traceID] = true
+			traceIDsModels = append(traceIDsModels, traceID)
+		}
 	}
 
 	return traceIDsModels, nil