Fix ordering of indexScanKeys after TraceID parsing, closes #1808

Signed-off-by: Michael Burman <[email protected]>

Author: burmanm

Diff for: plugin/storage/badger/spanstore/read_write_test.go

@@ -25,7 +25,7 @@ import (
 	"testing"
 	"time"
 
-	"github.com/stretchr/testify/assert"
+	assert "github.com/stretchr/testify/require"
 	"github.com/uber/jaeger-lib/metrics"
 	"go.uber.org/zap"
 
@@ -587,14 +587,14 @@ func BenchmarkServiceIndexLimitFetch(b *testing.B) {
 
 // Opens a badger db and runs a test on it.
 func runLargeFactoryTest(tb testing.TB, test func(tb testing.TB, sw spanstore.Writer, sr spanstore.Reader)) {
+	assert := assert.New(tb)
 	f := badger.NewFactory()
 	opts := badger.NewOptions("badger")
 	v, command := config.Viperize(opts.AddFlags)
 
 	dir := "/mnt/ssd/badger/testRun"
 	err := os.MkdirAll(dir, 0700)
-	defer os.RemoveAll(dir)
-	assert.NoError(tb, err)
+	assert.NoError(err)
 	keyParam := fmt.Sprintf("--badger.directory-key=%s", dir)
 	valueParam := fmt.Sprintf("--badger.directory-value=%s", dir)
 
@@ -608,22 +608,89 @@ func runLargeFactoryTest(tb testing.TB, test func(tb testing.TB, sw spanstore.Wr
 	f.InitFromViper(v)
 
 	err = f.Initialize(metrics.NullFactory, zap.NewNop())
-	assert.NoError(tb, err)
+	assert.NoError(err)
 
 	sw, err := f.CreateSpanWriter()
-	assert.NoError(tb, err)
+	assert.NoError(err)
 
 	sr, err := f.CreateSpanReader()
-	assert.NoError(tb, err)
+	assert.NoError(err)
 
 	defer func() {
 		if closer, ok := sw.(io.Closer); ok {
 			err := closer.Close()
-			assert.NoError(tb, err)
+			os.RemoveAll(dir)
+			assert.NoError(err)
 		} else {
-			tb.FailNow()
+			assert.FailNow("io.Closer not implemented by SpanWriter")
 		}
-
 	}()
 	test(tb, sw, sr)
 }
+
+// TestRandomTraceID from issue #1808
+func TestRandomTraceID(t *testing.T) {
+	runFactoryTest(t, func(tb testing.TB, sw spanstore.Writer, sr spanstore.Reader) {
+		s1 := model.Span{
+			TraceID: model.TraceID{
+				Low:  uint64(14767110704788176287),
+				High: 0,
+			},
+			SpanID:        model.SpanID(14976775253976086374),
+			OperationName: "/",
+			Process: &model.Process{
+				ServiceName: "nginx",
+			},
+			Tags: model.KeyValues{
+				model.KeyValue{
+					Key:   "http.request_id",
+					VStr:  "first",
+					VType: model.StringType,
+				},
+			},
+			StartTime: time.Now(),
+			Duration:  1 * time.Second,
+		}
+		err := sw.WriteSpan(&s1)
+		assert.NoError(t, err)
+
+		s2 := model.Span{
+			TraceID: model.TraceID{
+				Low:  uint64(4775132888371984950),
+				High: 0,
+			},
+			SpanID:        model.SpanID(13576481569227028654),
+			OperationName: "/",
+			Process: &model.Process{
+				ServiceName: "nginx",
+			},
+			Tags: model.KeyValues{
+				model.KeyValue{
+					Key:   "http.request_id",
+					VStr:  "second",
+					VType: model.StringType,
+				},
+			},
+			StartTime: time.Now(),
+			Duration:  1 * time.Second,
+		}
+		err = sw.WriteSpan(&s2)
+		assert.NoError(t, err)
+
+		params := &spanstore.TraceQueryParameters{
+			StartTimeMin: time.Now().Add(-1 * time.Minute),
+			StartTimeMax: time.Now(),
+			ServiceName:  "nginx",
+			Tags: map[string]string{
+				"http.request_id": "second",
+			},
+		}
+		traces, err := sr.FindTraces(context.Background(), params)
+		assert.NoError(t, err)
+
+		// failed with `second` tag query, but success with `first`
+		assert.Equal(t, 1, len(traces))
+	})
+
+	assert.True(t, false)
+}

Diff for: plugin/storage/badger/spanstore/reader.go

@@ -22,6 +22,7 @@ import (
 	"errors"
 	"fmt"
 	"math"
+	"sort"
 	"time"
 
 	"github.com/dgraph-io/badger"
@@ -291,6 +292,9 @@ func (r *TraceReader) indexSeeksToTraceIDs(query *spanstore.TraceQueryParameters
 				prevTraceID = traceID
 			}
 		}
+		sort.Slice(ids[i], func(k, h int) bool {
+			return bytes.Compare(ids[i][k], ids[i][h]) < 0
+		})
 	}
 	return ids, nil
 }
@@ -522,6 +526,7 @@ func (r *TraceReader) scanIndexKeys(indexKeyValue []byte, startTimeMin time.Time
 		}
 		return nil
 	})
+
 	return indexResults, err
 }
 

Diff for: plugin/storage/badger/spanstore/rw_internal_test.go

@@ -17,6 +17,8 @@ import (
 	"bytes"
 	"context"
 	"encoding/binary"
+	"math/rand"
+	"sort"
 	"testing"
 	"time"
 
@@ -212,3 +214,39 @@ func TestMergeJoin(t *testing.T) {
 	assert.Equal(2, len(merged))
 	assert.Equal(uint32(2), binary.BigEndian.Uint32(merged[1]))
 }
+
+func TestIndexScanReturnOrder(t *testing.T) {
+	runWithBadger(t, func(store *badger.DB, t *testing.T) {
+		testSpan := createDummySpan()
+		cache := NewCacheStore(store, time.Duration(1*time.Hour), true)
+		sw := NewSpanWriter(store, cache, time.Duration(1*time.Hour), nil)
+		rw := NewTraceReader(store, cache)
+
+		for i := 0; i < 1000; i++ {
+			testSpan.TraceID = model.TraceID{
+				High: rand.Uint64(),
+				Low:  uint64(rand.Int63()),
+			}
+			testSpan.SpanID = model.SpanID(rand.Uint64())
+			testSpan.StartTime = testSpan.StartTime.Add(time.Duration(i) * time.Millisecond)
+			err := sw.WriteSpan(&testSpan)
+			assert.NoError(t, err)
+		}
+
+		tqp := &spanstore.TraceQueryParameters{
+			ServiceName:  "service",
+			StartTimeMax: testSpan.StartTime.Add(time.Hour),
+			StartTimeMin: testSpan.StartTime.Add(-1 * time.Hour),
+		}
+
+		indexSeeks := make([][]byte, 0, 1)
+		indexSeeks = serviceQueries(tqp, indexSeeks)
+
+		ids := make([][][]byte, 0, len(indexSeeks)+1)
+
+		indexResults, _ := rw.indexSeeksToTraceIDs(tqp, indexSeeks, ids)
+		assert.True(t, sort.SliceIsSorted(indexResults[0], func(i, j int) bool {
+			return bytes.Compare(indexResults[0][i], indexResults[0][j]) < 0
+		}))
+	})
+}