Cleanup tests

Signed-off-by: Yuri Shkuro <ys@uber.com>
jaegertracing · Dec 3, 2019 · 6ec0f93 · 6ec0f93
1 parent 7e561d2
commit 6ec0f93
Show file tree

Hide file tree

Showing 2 changed files with 38 additions and 25 deletions.
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 := traceIDQuery(traceID)
+			query := buildTraceByIDQuery(traceID)
 			if val, ok := searchAfterTime[traceID]; ok {
 				nextTime = val
 			}
@@ -393,35 +393,36 @@ func (s *SpanReader) multiRead(ctx context.Context, traceIDs []model.TraceID, st
 	return traces, nil
 }
 
-func traceIDQuery(traceID model.TraceID) elastic.Query {
+func buildTraceByIDQuery(traceID model.TraceID) elastic.Query {
 	traceIDStr := traceID.String()
+	if traceIDStr[0] != '0' {
+		return elastic.NewTermQuery(traceIDField, traceIDStr)
+	}
 	// TODO remove in newer versions, added in Jaeger 1.16
-	// read ID without leading zeros
 	// https://github.com/jaegertracing/jaeger/pull/1956 added leading zeros to IDs
-	if traceIDStr[0] == '0' {
-		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))
-	}
-	return elastic.NewTermQuery(traceIDField, traceIDStr)
+	// So we need to also read IDs without leading zeros for compatibility with previously saved data.
+	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) {
 	traceIDsMap := map[model.TraceID]bool{}
-	traceIDsModels := make([]model.TraceID, len(traceIDs))
-	for i, ID := range traceIDs {
+	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))
 		}
 		if _, ok := traceIDsMap[traceID]; !ok {
 			traceIDsMap[traceID] = true
-			traceIDsModels[i] = traceID
+			traceIDsModels = append(traceIDsModels, traceID)
 		}
 	}
 

diff --git a/plugin/storage/es/spanstore/reader_test.go b/plugin/storage/es/spanstore/reader_test.go
@@ -21,7 +21,6 @@ import (
 	"errors"
 	"fmt"
 	"io/ioutil"
-	"math/bits"
 	"reflect"
 	"testing"
 	"time"
@@ -223,7 +222,6 @@ func TestSpanReader_multiRead_followUp_query(t *testing.T) {
 		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)))
@@ -1086,8 +1084,16 @@ func TestSpanReader_ArchiveTraces_ReadAlias(t *testing.T) {
 	})
 }
 
-func TestTraceIDQuery(t *testing.T) {
-	uintMax := uint64((1 << bits.UintSize) - 1)
+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)
@@ -1097,19 +1103,25 @@ func TestTraceIDQuery(t *testing.T) {
 	}{
 		{
 			traceID: traceIDNoHigh,
-			query:   elastic.NewBoolQuery().Should(elastic.NewTermQuery(traceIDField, traceIDNoHigh.String()).Boost(2), elastic.NewTermQuery(traceIDField, fmt.Sprintf("%x", traceIDNoHigh.Low))),
+			query: elastic.NewBoolQuery().Should(
+				elastic.NewTermQuery(traceIDField, "0000000000000001").Boost(2),
+				elastic.NewTermQuery(traceIDField, "1"),
+			),
 		},
 		{
 			traceID: traceIDHigh,
-			query:   elastic.NewBoolQuery().Should(elastic.NewTermQuery(traceIDField, traceIDHigh.String()).Boost(2), elastic.NewTermQuery(traceIDField, fmt.Sprintf("%x%016x", traceIDHigh.High, traceIDHigh.Low))),
+			query: elastic.NewBoolQuery().Should(
+				elastic.NewTermQuery(traceIDField, "00000000000000010000000000000001").Boost(2),
+				elastic.NewTermQuery(traceIDField, "10000000000000001"),
+			),
 		},
 		{
 			traceID: traceID,
-			query:   elastic.NewTermQuery(traceIDField, traceID.String()),
+			query:   elastic.NewTermQuery(traceIDField, "ffffffffffffffffffffffffffffffff"),
 		},
 	}
 	for _, test := range tests {
-		q := traceIDQuery(test.traceID)
+		q := buildTraceByIDQuery(test.traceID)
 		assert.Equal(t, test.query, q)
 	}
 }