From 95331addaa0cccb628b363542c5064958e9a2ccc Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Wed, 4 Dec 2019 16:13:50 -0500 Subject: [PATCH] Keep leading zeros in trace ID hex string (#1956) * Do not remote leading zeros in trace ID hex string Signed-off-by: Yuri Shkuro * Fix test in ./cmd/query/app Signed-off-by: Yuri Shkuro * Fix tests in ./model Signed-off-by: Yuri Shkuro * Fix tests in ./model/converter/json Signed-off-by: Yuri Shkuro * Fix tests in ./plugin/storage/cassandra/spanstore Signed-off-by: Yuri Shkuro * Fix tests in ./plugin/storage/cassandra/spanstore/dbmodel Signed-off-by: Yuri Shkuro * Fix tests in ./plugin/storage/es/spanstore Signed-off-by: Yuri Shkuro * Zero-pad SpanID as well Signed-off-by: Yuri Shkuro * Read trace ID from ES without leading zeros Signed-off-by: Pavol Loffay * Review comments Signed-off-by: Pavol Loffay * Cleanup tests Signed-off-by: Yuri Shkuro * Add to changelog Signed-off-by: Yuri Shkuro --- CHANGELOG.md | 43 ++++++++++++--- cmd/query/app/http_handler_test.go | 2 +- model/adjuster/clockskew_test.go | 2 +- model/converter/json/fixtures/es_01.json | 16 +++--- model/converter/json/fixtures/ui_01.json | 42 +++++++-------- model/ids.go | 6 +-- model/span_test.go | 20 +++---- .../cassandra/spanstore/dbmodel/model_test.go | 2 +- .../cassandra/spanstore/reader_test.go | 4 +- .../cassandra/spanstore/writer_test.go | 2 +- .../es/spanstore/dbmodel/fixtures/es_01.json | 16 +++--- plugin/storage/es/spanstore/reader.go | 37 +++++++++++-- plugin/storage/es/spanstore/reader_test.go | 53 ++++++++++++++++--- 13 files changed, 172 insertions(+), 73 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 572c6ac0c61..fec8126d6c0 100644 --- 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 index 2f6fdad52e8..a38c8324eb8 100644 --- 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 index 969f1a3fa20..b1e449d745f 100644 --- 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 index d57d0db562a..1e4b3dcd337 100644 --- 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 index fa8bebb85ee..06a61ddb6c7 100644 --- 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 index 8b34f7c9a33..be7da4d546c 100644 --- 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 index 4e601f71349..2fafe3ca9a3 100644 --- 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 index 93fb80a1ebd..e48c18eed01 100644 --- 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 index 66ca4450831..abb757acc4f 100644 --- 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 index 414050d8f69..5966b7d9427 100644 --- 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 index d1cfbabcf41..8b30c120d9c 100644 --- 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 index a2163ed9afb..29124eda749 100644 --- 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 diff --git a/plugin/storage/es/spanstore/reader_test.go b/plugin/storage/es/spanstore/reader_test.go index d95f99f34b1..62f7e63eb58 100644 --- a/plugin/storage/es/spanstore/reader_test.go +++ b/plugin/storage/es/spanstore/reader_test.go @@ -216,11 +216,15 @@ func TestSpanReader_multiRead_followUp_query(t *testing.T) { spanBytesID2, err := json.Marshal(spanID2) require.NoError(t, err) - id1Query := elastic.NewTermQuery("traceID", model.TraceID{High: 0, Low: 1}.String()) + id1Query := elastic.NewBoolQuery().Should( + elastic.NewTermQuery(traceIDField, model.TraceID{High: 0, Low: 1}.String()).Boost(2), + elastic.NewTermQuery(traceIDField, fmt.Sprintf("%x", 1))) id1Search := elastic.NewSearchRequest(). IgnoreUnavailable(true). Source(r.reader.sourceFn(id1Query, model.TimeAsEpochMicroseconds(date.Add(-time.Hour)))) - id2Query := elastic.NewTermQuery("traceID", model.TraceID{High: 0, Low: 2}.String()) + id2Query := elastic.NewBoolQuery().Should( + elastic.NewTermQuery(traceIDField, model.TraceID{High: 0, Low: 2}.String()).Boost(2), + elastic.NewTermQuery(traceIDField, fmt.Sprintf("%x", 2))) id2Search := elastic.NewSearchRequest(). IgnoreUnavailable(true). Source(r.reader.sourceFn(id2Query, model.TimeAsEpochMicroseconds(date.Add(-time.Hour)))) @@ -797,7 +801,7 @@ func TestTraceIDsStringsToModelsConversion(t *testing.T) { traceIDs, err := convertTraceIDsStringsToModels([]string{"1", "2", "3"}) assert.NoError(t, err) assert.Equal(t, 3, len(traceIDs)) - assert.Equal(t, "1", traceIDs[0].String()) + assert.Equal(t, model.NewTraceID(0, 1), traceIDs[0]) traceIDs, err = convertTraceIDsStringsToModels([]string{"dsfjsdklfjdsofdfsdbfkgbgoaemlrksdfbsdofgerjl"}) assert.EqualError(t, err, "Making traceID from string 'dsfjsdklfjdsofdfsdbfkgbgoaemlrksdfbsdofgerjl' failed: TraceID cannot be longer than 32 hex characters: dsfjsdklfjdsofdfsdbfkgbgoaemlrksdfbsdofgerjl") @@ -1080,7 +1084,44 @@ func TestSpanReader_ArchiveTraces_ReadAlias(t *testing.T) { }) } -func TestNewSpanReader2(t *testing.T) { - spanDate := time.Now().UTC().Format("2006") - fmt.Println(spanDate) +func TestConvertTraceIDsStringsToModels(t *testing.T) { + ids, err := convertTraceIDsStringsToModels([]string{"1", "2", "01", "02", "001", "002"}) + require.NoError(t, err) + assert.Equal(t, []model.TraceID{model.NewTraceID(0, 1), model.NewTraceID(0, 2)}, ids) + _, err = convertTraceIDsStringsToModels([]string{"1", "2", "01", "02", "001", "002", "blah"}) + assert.Error(t, err) +} + +func TestBuildTraceByIDQuery(t *testing.T) { + uintMax := ^uint64(0) + traceIDNoHigh := model.NewTraceID(0, 1) + traceIDHigh := model.NewTraceID(1, 1) + traceID := model.NewTraceID(uintMax, uintMax) + tests := []struct { + traceID model.TraceID + query elastic.Query + }{ + { + traceID: traceIDNoHigh, + query: elastic.NewBoolQuery().Should( + elastic.NewTermQuery(traceIDField, "0000000000000001").Boost(2), + elastic.NewTermQuery(traceIDField, "1"), + ), + }, + { + traceID: traceIDHigh, + query: elastic.NewBoolQuery().Should( + elastic.NewTermQuery(traceIDField, "00000000000000010000000000000001").Boost(2), + elastic.NewTermQuery(traceIDField, "10000000000000001"), + ), + }, + { + traceID: traceID, + query: elastic.NewTermQuery(traceIDField, "ffffffffffffffffffffffffffffffff"), + }, + } + for _, test := range tests { + q := buildTraceByIDQuery(test.traceID) + assert.Equal(t, test.query, q) + } }