From 567d0be5bb40b132e3b2e67d65bf1c3e25d20b1a Mon Sep 17 00:00:00 2001 From: Matheus Nogueira Date: Wed, 29 Nov 2023 13:02:59 -0300 Subject: [PATCH] fix: remove some possible panics (#3405) * fix: remove panic warnings from traces package * fix: possible panics in tracedb package * empty commit to trigger pipeline --- server/tracedb/azureappinsights.go | 4 ++ server/tracedb/elasticsearchdb.go | 5 ++ server/tracedb/signalfxdb.go | 8 ++++ server/tracedb/tempodb.go | 5 +- server/traces/otel_converter.go | 20 ++++++++ server/traces/span_entitiess.go | 2 +- server/traces/traces_test.go | 77 +++++++++++++++++++----------- 7 files changed, 90 insertions(+), 31 deletions(-) diff --git a/server/tracedb/azureappinsights.go b/server/tracedb/azureappinsights.go index 6276ebe3fe..590be7aa41 100644 --- a/server/tracedb/azureappinsights.go +++ b/server/tracedb/azureappinsights.go @@ -214,6 +214,10 @@ func parseSpans(table *azquery.Table) ([]traces.Span, error) { for _, eventRow := range eventRows { parentSpan := spanMap[eventRow.ParentID()] + if parentSpan == nil { + continue + } + event, err := parseEvent(eventRow) if err != nil { return []traces.Span{}, err diff --git a/server/tracedb/elasticsearchdb.go b/server/tracedb/elasticsearchdb.go index f10e138e35..6f2d35f9e3 100644 --- a/server/tracedb/elasticsearchdb.go +++ b/server/tracedb/elasticsearchdb.go @@ -137,6 +137,11 @@ func getClusterInfo(client *elasticsearch.Client) (string, error) { if err != nil { return "", fmt.Errorf("error getting cluster info response: %s", err) } + + if res == nil { + return "", fmt.Errorf("could not get response") + } + defer res.Body.Close() // Check response status diff --git a/server/tracedb/signalfxdb.go b/server/tracedb/signalfxdb.go index caa60271c5..8e9c64ad76 100644 --- a/server/tracedb/signalfxdb.go +++ b/server/tracedb/signalfxdb.go @@ -114,6 +114,10 @@ func (db signalfxDB) getSegmentsTimestamps(ctx context.Context, traceID string) return []int64{}, fmt.Errorf("could not execute request: %w", err) } + if response == nil { + return []int64{}, fmt.Errorf("could not get response") + } + if response.StatusCode != http.StatusOK { return []int64{}, fmt.Errorf("service responded with a non ok status code: %s", strconv.Itoa(response.StatusCode)) } @@ -148,6 +152,10 @@ func (db signalfxDB) getSegmentSpans(ctx context.Context, traceID string, timest return []signalFXSpan{}, fmt.Errorf("could not execute request: %w", err) } + if response == nil { + return []signalFXSpan{}, fmt.Errorf("could not get response") + } + if response.StatusCode != 200 { return []signalFXSpan{}, nil } diff --git a/server/tracedb/tempodb.go b/server/tracedb/tempodb.go index 5f2f08cb7d..f017536f89 100644 --- a/server/tracedb/tempodb.go +++ b/server/tracedb/tempodb.go @@ -121,11 +121,14 @@ func httpGetTraceByID(ctx context.Context, traceID string, client *datasource.Ht return traces.Trace{}, err } resp, err := client.Request(ctx, fmt.Sprintf("/api/traces/%s", trID), http.MethodGet, "") - if err != nil { return traces.Trace{}, handleError(err) } + if resp == nil { + return traces.Trace{}, fmt.Errorf("could not get response") + } + if resp.StatusCode == 404 { return traces.Trace{}, connection.ErrTraceNotFound } diff --git a/server/traces/otel_converter.go b/server/traces/otel_converter.go index 5c2ba81a44..09bd002402 100644 --- a/server/traces/otel_converter.go +++ b/server/traces/otel_converter.go @@ -121,12 +121,24 @@ func spanKind(span *v1.Span) SpanKind { func getAttributeValue(value *v11.AnyValue) string { switch v := value.GetValue().(type) { case *v11.AnyValue_StringValue: + if v == nil { + return "" + } + return v.StringValue case *v11.AnyValue_IntValue: + if v == nil { + return "0" + } + return fmt.Sprintf("%d", v.IntValue) case *v11.AnyValue_DoubleValue: + if v == nil { + return "0.0" + } + if v.DoubleValue != 0.0 { isFloatingPoint := math.Abs(v.DoubleValue-math.Abs(v.DoubleValue)) > 0.0 if isFloatingPoint { @@ -137,6 +149,10 @@ func getAttributeValue(value *v11.AnyValue) string { } case *v11.AnyValue_BoolValue: + if v == nil { + return "false" + } + return fmt.Sprintf("%t", v.BoolValue) } @@ -166,6 +182,10 @@ func CreateTraceID(id []byte) trace.TraceID { func DecodeTraceID(id string) trace.TraceID { bytes, _ := hex.DecodeString(id) + if len(bytes) < 16 { + return trace.TraceID{} + } + var tid [16]byte copy(tid[:], bytes[:16]) return trace.TraceID(tid) diff --git a/server/traces/span_entitiess.go b/server/traces/span_entitiess.go index 6ea53e1c7a..81ea27c160 100644 --- a/server/traces/span_entitiess.go +++ b/server/traces/span_entitiess.go @@ -302,7 +302,7 @@ func decodeChildren(parent *Span, children []encodedSpan, cache spanCache) ([]*S } res := make([]*Span, len(children)) for i, c := range children { - if span, ok := cache.Get(c.ID); ok { + if span, ok := cache.Get(c.ID); ok && span != nil { res[i] = span continue } diff --git a/server/traces/traces_test.go b/server/traces/traces_test.go index 67df259e45..555400e5e4 100644 --- a/server/traces/traces_test.go +++ b/server/traces/traces_test.go @@ -24,11 +24,12 @@ func TestTraces(t *testing.T) { spans := []traces.Span{rootSpan, childSpan1, childSpan2, grandchildSpan} trace := traces.NewTrace("trace", spans) - assert.Len(t, trace.Flat, 4) + require.Len(t, trace.Flat, 4) assert.Equal(t, "Root", trace.RootSpan.Name) - assert.Equal(t, "child 1", trace.RootSpan.Children[0].Name) - assert.Equal(t, "child 2", trace.RootSpan.Children[1].Name) - assert.Equal(t, "grandchild", trace.RootSpan.Children[1].Children[0].Name) + + assert.Equal(t, "child 1", child(t, &trace.RootSpan, 0).Name) + assert.Equal(t, "child 2", child(t, &trace.RootSpan, 1).Name) + assert.Equal(t, "grandchild", grandchild(t, &trace.RootSpan, 1, 0).Name) } func TestTraceWithMultipleRoots(t *testing.T) { @@ -45,12 +46,12 @@ func TestTraceWithMultipleRoots(t *testing.T) { // agreggate root + 3 roots + 3 child assert.Len(t, trace.Flat, 7) assert.Equal(t, traces.TemporaryRootSpanName, trace.RootSpan.Name) - assert.Equal(t, "Root 1", trace.RootSpan.Children[0].Name) - assert.Equal(t, "Root 2", trace.RootSpan.Children[1].Name) - assert.Equal(t, "Root 3", trace.RootSpan.Children[2].Name) - assert.Equal(t, "Child from root 1", trace.RootSpan.Children[0].Children[0].Name) - assert.Equal(t, "Child from root 2", trace.RootSpan.Children[1].Children[0].Name) - assert.Equal(t, "Child from root 3", trace.RootSpan.Children[2].Children[0].Name) + assert.Equal(t, "Root 1", child(t, &trace.RootSpan, 0).Name) + assert.Equal(t, "Root 2", child(t, &trace.RootSpan, 1).Name) + assert.Equal(t, "Root 3", child(t, &trace.RootSpan, 2).Name) + assert.Equal(t, "Child from root 1", grandchild(t, &trace.RootSpan, 0, 0).Name) + assert.Equal(t, "Child from root 2", grandchild(t, &trace.RootSpan, 1, 0).Name) + assert.Equal(t, "Child from root 3", grandchild(t, &trace.RootSpan, 2, 0).Name) } func TestTraceWithMultipleTemporaryRoots(t *testing.T) { @@ -66,9 +67,9 @@ func TestTraceWithMultipleTemporaryRoots(t *testing.T) { require.Len(t, trace.Flat, 4) assert.Equal(t, traces.TemporaryRootSpanName, trace.RootSpan.Name) - assert.Equal(t, "Child from root 1", trace.RootSpan.Children[0].Name) - assert.Equal(t, "Child from root 2", trace.RootSpan.Children[1].Name) - assert.Equal(t, "Child from root 3", trace.RootSpan.Children[2].Name) + assert.Equal(t, "Child from root 1", child(t, &trace.RootSpan, 0).Name) + assert.Equal(t, "Child from root 2", child(t, &trace.RootSpan, 1).Name) + assert.Equal(t, "Child from root 3", child(t, &trace.RootSpan, 2).Name) } func TestTraceAssemble(t *testing.T) { @@ -82,16 +83,16 @@ func TestTraceAssemble(t *testing.T) { assert.Len(t, trace.Flat, 4) assert.Equal(t, "Temporary Tracetest root span", trace.RootSpan.Name) - assert.Equal(t, "Root", trace.RootSpan.Children[0].Name) - assert.Equal(t, "child 1", trace.RootSpan.Children[0].Children[0].Name) - assert.Equal(t, "grandchild", trace.RootSpan.Children[1].Name) + assert.Equal(t, "Root", child(t, &trace.RootSpan, 0).Name) + assert.Equal(t, "child 1", grandchild(t, &trace.RootSpan, 0, 0).Name) + assert.Equal(t, "grandchild", child(t, &trace.RootSpan, 1).Name) trace = traces.NewTrace(trace.ID.String(), append(trace.Spans(), childSpan2)) assert.Len(t, trace.Flat, 4) assert.Equal(t, "Root", trace.RootSpan.Name) - assert.Equal(t, "child 1", trace.RootSpan.Children[0].Name) - assert.Equal(t, "child 2", trace.RootSpan.Children[1].Name) - assert.Equal(t, "grandchild", trace.RootSpan.Children[1].Children[0].Name) + assert.Equal(t, "child 1", child(t, &trace.RootSpan, 0).Name) + assert.Equal(t, "child 2", child(t, &trace.RootSpan, 1).Name) + assert.Equal(t, "grandchild", grandchild(t, &trace.RootSpan, 1, 0).Name) } func TestTraceWithMultipleRootsFromOtel(t *testing.T) { @@ -119,12 +120,12 @@ func TestTraceWithMultipleRootsFromOtel(t *testing.T) { // agreggate root + 3 roots + 3 child assert.Len(t, trace.Flat, 7) assert.Equal(t, traces.TemporaryRootSpanName, trace.RootSpan.Name) - assert.Equal(t, "Root 1", trace.RootSpan.Children[0].Name) - assert.Equal(t, "Root 2", trace.RootSpan.Children[1].Name) - assert.Equal(t, "Root 3", trace.RootSpan.Children[2].Name) - assert.Equal(t, "Child from root 1", trace.RootSpan.Children[0].Children[0].Name) - assert.Equal(t, "Child from root 2", trace.RootSpan.Children[1].Children[0].Name) - assert.Equal(t, "Child from root 3", trace.RootSpan.Children[2].Children[0].Name) + assert.Equal(t, "Root 1", child(t, &trace.RootSpan, 0).Name) + assert.Equal(t, "Root 2", child(t, &trace.RootSpan, 1).Name) + assert.Equal(t, "Root 3", child(t, &trace.RootSpan, 2).Name) + assert.Equal(t, "Child from root 1", grandchild(t, &trace.RootSpan, 0, 0).Name) + assert.Equal(t, "Child from root 2", grandchild(t, &trace.RootSpan, 1, 0).Name) + assert.Equal(t, "Child from root 3", grandchild(t, &trace.RootSpan, 2, 0).Name) } func TestInjectingNewRootWhenSingleRoot(t *testing.T) { @@ -142,7 +143,7 @@ func TestInjectingNewRootWhenSingleRoot(t *testing.T) { assert.Len(t, newTrace.Flat, 5) assert.Equal(t, "new Root", newTrace.RootSpan.Name) assert.Len(t, newTrace.RootSpan.Children, 1) - assert.Equal(t, "Root", newTrace.RootSpan.Children[0].Name) + assert.Equal(t, "Root", child(t, &newTrace.RootSpan, 0).Name) } func TestInjectingNewRootWhenMultipleRoots(t *testing.T) { @@ -166,9 +167,9 @@ func TestInjectingNewRootWhenMultipleRoots(t *testing.T) { assert.Len(t, newTrace.Flat, 7) assert.Equal(t, "new Root", newTrace.RootSpan.Name) assert.Len(t, newTrace.RootSpan.Children, 3) - assert.Equal(t, "Root 1", newTrace.RootSpan.Children[0].Name) - assert.Equal(t, "Root 2", newTrace.RootSpan.Children[1].Name) - assert.Equal(t, "Root 3", newTrace.RootSpan.Children[2].Name) + assert.Equal(t, "Root 1", child(t, &newTrace.RootSpan, 0).Name) + assert.Equal(t, "Root 2", child(t, &newTrace.RootSpan, 1).Name) + assert.Equal(t, "Root 3", child(t, &newTrace.RootSpan, 2).Name) for _, oldRoot := range trace.RootSpan.Children { require.NotNil(t, oldRoot.Parent) @@ -260,6 +261,7 @@ func TestMergingFragmentsFromSameTrace(t *testing.T) { secondFragment := traces.NewTrace(traceID, []traces.Span{rootSpan, childSpan1}) trace := traces.MergeTraces(&firstFragment, &secondFragment) + require.NotNil(t, trace) assert.NotEmpty(t, trace.ID) assert.Len(t, trace.Flat, 3) } @@ -554,3 +556,20 @@ func attributesFromMap(input map[string]string) traces.Attributes { return attributes } + +func child(t *testing.T, span *traces.Span, index int) *traces.Span { + if len(span.Children) < index+1 { + t.FailNow() + } + + child := span.Children[index] + if child == nil { + t.FailNow() + } + + return child +} + +func grandchild(t *testing.T, span *traces.Span, parentIndex int, grandChildIndex int) *traces.Span { + return child(t, child(t, span, parentIndex), grandChildIndex) +}