From f38fe253228f552676ed1e208386fc418d526acf Mon Sep 17 00:00:00 2001 From: Ivan Babrou Date: Fri, 8 Jan 2021 15:05:05 -0800 Subject: [PATCH 1/2] Copy spans from memory store, fixes #2719 Copying allows spans to be freely modified by adjusters and any other code without accidentally altering what is stored in the in-memory store itself. Signed-off-by: Ivan Babrou --- plugin/storage/memory/memory.go | 23 +++++++++++++++++------ plugin/storage/memory/memory_test.go | 24 +++++++++++++++++++++--- 2 files changed, 38 insertions(+), 9 deletions(-) diff --git a/plugin/storage/memory/memory.go b/plugin/storage/memory/memory.go index ee5e1643cee..8ca8678e2df 100644 --- a/plugin/storage/memory/memory.go +++ b/plugin/storage/memory/memory.go @@ -22,6 +22,8 @@ import ( "sync" "time" + "github.com/golang/protobuf/proto" + "github.com/jaegertracing/jaeger/model" "github.com/jaegertracing/jaeger/model/adjuster" "github.com/jaegertracing/jaeger/pkg/memory/config" @@ -164,15 +166,19 @@ func (m *Store) GetTrace(ctx context.Context, traceID model.TraceID) (*model.Tra if !ok { return nil, spanstore.ErrTraceNotFound } - return m.copyTrace(trace), nil + return m.copyTrace(trace) } // Spans may still be added to traces after they are returned to user code, so make copies. -func (m *Store) copyTrace(trace *model.Trace) *model.Trace { - return &model.Trace{ - Spans: append([]*model.Span(nil), trace.Spans...), - Warnings: append([]string(nil), trace.Warnings...), +func (m *Store) copyTrace(trace *model.Trace) (*model.Trace, error) { + bytes, err := proto.Marshal(trace) + if err != nil { + return nil, err } + + copied := &model.Trace{} + err = proto.Unmarshal(bytes, copied) + return copied, err } // GetServices returns a list of all known services @@ -211,7 +217,12 @@ func (m *Store) FindTraces(ctx context.Context, query *spanstore.TraceQueryParam var retMe []*model.Trace for _, trace := range m.traces { if m.validTrace(trace, query) { - retMe = append(retMe, m.copyTrace(trace)) + copied, err := m.copyTrace(trace) + if err != nil { + return nil, err + } + + retMe = append(retMe, copied) } } diff --git a/plugin/storage/memory/memory_test.go b/plugin/storage/memory/memory_test.go index 104e41a41c4..19ad502016a 100644 --- a/plugin/storage/memory/memory_test.go +++ b/plugin/storage/memory/memory_test.go @@ -34,7 +34,7 @@ var testingSpan = &model.Span{ SpanID: model.NewSpanID(1), Process: &model.Process{ ServiceName: "serviceName", - Tags: model.KeyValues{}, + Tags: []model.KeyValue(nil), }, OperationName: "operationName", Tags: model.KeyValues{ @@ -43,14 +43,14 @@ var testingSpan = &model.Span{ }, Logs: []model.Log{ { - Timestamp: time.Now(), + Timestamp: time.Now().UTC(), Fields: []model.KeyValue{ model.String("logKey", "logValue"), }, }, }, Duration: time.Second * 5, - StartTime: time.Unix(300, 0), + StartTime: time.Unix(300, 0).UTC(), } var childSpan1 = &model.Span{ @@ -210,6 +210,24 @@ func TestStoreGetTraceSuccess(t *testing.T) { }) } +func TestStoreGetAndMutateTrace(t *testing.T) { + withPopulatedMemoryStore(func(store *Store) { + trace, err := store.GetTrace(context.Background(), testingSpan.TraceID) + assert.NoError(t, err) + assert.Len(t, trace.Spans, 1) + assert.Equal(t, testingSpan, trace.Spans[0]) + assert.Len(t, trace.Spans[0].Warnings, 0) + + trace.Spans[0].Warnings = append(trace.Spans[0].Warnings, "the end is near") + + trace, err = store.GetTrace(context.Background(), testingSpan.TraceID) + assert.NoError(t, err) + assert.Len(t, trace.Spans, 1) + assert.Equal(t, testingSpan, trace.Spans[0]) + assert.Len(t, trace.Spans[0].Warnings, 0) + }) +} + func TestStoreGetTraceFailure(t *testing.T) { withPopulatedMemoryStore(func(store *Store) { trace, err := store.GetTrace(context.Background(), model.TraceID{}) From cfb05acc50164a32ccd9589eec4d674acd308186 Mon Sep 17 00:00:00 2001 From: Ivan Babrou Date: Fri, 8 Jan 2021 23:01:00 -0800 Subject: [PATCH 2/2] Add tests to exercise the broken serialization path Signed-off-by: Ivan Babrou --- plugin/storage/memory/memory_test.go | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/plugin/storage/memory/memory_test.go b/plugin/storage/memory/memory_test.go index 19ad502016a..ef5e499a5e6 100644 --- a/plugin/storage/memory/memory_test.go +++ b/plugin/storage/memory/memory_test.go @@ -128,6 +128,14 @@ var childSpan2_1 = &model.Span{ StartTime: time.Unix(300, 0), } +// This kind of trace cannot be serialized +var nonSerializableSpan = &model.Span{ + Process: &model.Process{ + ServiceName: "naughtyService", + }, + StartTime: time.Date(0, 0, 0, 0, 0, 0, 0, time.UTC), +} + func withPopulatedMemoryStore(f func(store *Store)) { memStore := NewStore() memStore.WriteSpan(context.Background(), testingSpan) @@ -228,6 +236,16 @@ func TestStoreGetAndMutateTrace(t *testing.T) { }) } +func TestStoreGetTraceError(t *testing.T) { + withPopulatedMemoryStore(func(store *Store) { + store.traces[testingSpan.TraceID] = &model.Trace{ + Spans: []*model.Span{nonSerializableSpan}, + } + _, err := store.GetTrace(context.Background(), testingSpan.TraceID) + assert.Error(t, err) + }) +} + func TestStoreGetTraceFailure(t *testing.T) { withPopulatedMemoryStore(func(store *Store) { trace, err := store.GetTrace(context.Background(), model.TraceID{}) @@ -300,6 +318,15 @@ func TestStoreGetEmptyTraceSet(t *testing.T) { }) } +func TestStoreFindTracesError(t *testing.T) { + withPopulatedMemoryStore(func(store *Store) { + err := store.WriteSpan(context.Background(), nonSerializableSpan) + assert.NoError(t, err) + _, err = store.FindTraces(context.Background(), &spanstore.TraceQueryParameters{ServiceName: "naughtyService"}) + assert.Error(t, err) + }) +} + func TestStoreFindTracesLimitGetsMostRecent(t *testing.T) { storeSize, querySize := 100, 10