Skip to content

Commit

Permalink
fix: remove some possible panics (#3405)
Browse files Browse the repository at this point in the history 
* fix: remove panic warnings from traces package

* fix: possible panics in tracedb package

* empty commit to trigger pipeline
  • Loading branch information
@mathnogueira
mathnogueira committed Nov 29, 2023
1 parent aa7e597 commit 567d0be
Show file tree
Hide file tree
Showing 7 changed files with 90 additions and 31 deletions.
4 changes: 4 additions & 0 deletions server/tracedb/azureappinsights.go
View file
Expand Up @@ -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
Expand Down
5 changes: 5 additions & 0 deletions server/tracedb/elasticsearchdb.go
View file
Expand Up @@ -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
Expand Down
8 changes: 8 additions & 0 deletions server/tracedb/signalfxdb.go
View file
Expand Up @@ -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))
}
Expand Down Expand Up @@ -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
}
Expand Down
5 changes: 4 additions & 1 deletion server/tracedb/tempodb.go
View file
Expand Up @@ -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
}
Expand Down
20 changes: 20 additions & 0 deletions server/traces/otel_converter.go
View file
Expand Up @@ -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 {
Expand All @@ -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)
}

Expand Down Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion server/traces/span_entitiess.go
View file
Expand Up @@ -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
}
Expand Down
77 changes: 48 additions & 29 deletions server/traces/traces_test.go
View file
Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -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)
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}

0 comments on commit 567d0be

Please sign in to comment.