Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/trace/agent/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ dd_agent_go_test(
"@com_github_golang_mock//gomock",
"@com_github_stretchr_testify//assert",
"@com_github_stretchr_testify//require",
"@com_github_tinylib_msgp//msgp",
"@org_golang_google_protobuf//proto",
"@org_uber_go_atomic//:atomic",
] + select({
Expand Down
5 changes: 5 additions & 0 deletions pkg/trace/agent/normalizer.go
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,11 @@ func setChunkAttributes(chunk *pb.TraceChunk, root *pb.Span) {
for _, span := range chunk.Spans {
// First span wins
if dm, ok := span.Meta[tagDecisionMaker]; ok {
// A v0.7 payload that omits the "tags" key decodes with a nil
// map; allocate before writing to avoid panicking on it.
if chunk.Tags == nil {
chunk.Tags = make(map[string]string, 1)
}
chunk.Tags[tagDecisionMaker] = dm
break
}
Expand Down
31 changes: 31 additions & 0 deletions pkg/trace/agent/normalizer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/DataDog/datadog-go/v5/statsd"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/tinylib/msgp/msgp"
"go.uber.org/atomic"

gzip "github.com/DataDog/datadog-agent/comp/trace/compression/impl-gzip"
Expand Down Expand Up @@ -700,6 +701,36 @@ func TestTagDecisionMaker(t *testing.T) {
assert.Equal("right", chunk.Spans[1].Meta[tagDecisionMaker])
}

// TestTagDecisionMakerNilChunkTags covers a v0.7 payload that omits the chunk
// "tags" key entirely: the decoder leaves Tags nil, and promoting the span-level
// decision maker into it must not panic.
func TestTagDecisionMakerNilChunkTags(t *testing.T) {
assert := assert.New(t)
var chunk pb.TraceChunk
// Encode a chunk map without a "tags" field, so UnmarshalMsg never
// allocates chunk.Tags.
var b []byte
b = msgp.AppendMapHeader(b, 2)
b = msgp.AppendString(b, "priority")
b = msgp.AppendInt32(b, int32(sampler.PriorityAutoKeep))
b = msgp.AppendString(b, "spans")
b = msgp.AppendArrayHeader(b, 1)
b = msgp.AppendMapHeader(b, 1)
b = msgp.AppendString(b, "meta")
b = msgp.AppendMapHeader(b, 1)
b = msgp.AppendString(b, tagDecisionMaker)
b = msgp.AppendString(b, "-4")

left, err := chunk.UnmarshalMsg(b)
require.NoError(t, err)
require.Empty(t, left)
require.Nil(t, chunk.Tags)
require.Len(t, chunk.Spans, 1)

setChunkAttributes(&chunk, chunk.Spans[0])
assert.Equal("-4", chunk.Tags[tagDecisionMaker])
}

func BenchmarkNormalization(b *testing.B) {
a := &Agent{conf: config.New()}
b.ReportAllocs()
Expand Down
28 changes: 20 additions & 8 deletions pkg/trace/api/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -548,12 +548,13 @@ func (r *HTTPReceiver) tagStats(v Version, req *http.Request, service string) *i
// - tp is the decoded payload
// - err is the first error encountered
func (r *HTTPReceiver) decodeTracerPayload(v Version, req *http.Request, cIDProvider IDProvider, lang, langVersion, tracerVersion string) (tp *pb.TracerPayload, err error) {
// Legacy decoders use pointer slices and may preserve nil wire entries.
// Establish the payload invariant here, before receiver metadata extraction
// Decoders vary in what they leave behind: legacy ones use pointer slices
// and may preserve nil wire entries, and v0.7 may leave chunk Tags nil.
// Establish the payload invariants here, before receiver metadata extraction
// or the payload is handed to the processing pipeline.
defer func() {
if err == nil && tp != nil {
removeNilEntries(tp)
normalizeDecodedPayload(tp)
}
}()

Expand Down Expand Up @@ -1310,18 +1311,29 @@ func traceChunksFromTraces(traces pb.Traces) []*pb.TraceChunk {
return traceChunks
}

// removeNilEntries removes nil entries produced by legacy payload
// decoders. After this function returns, every retained chunk, span, link,
// event, event attribute, and attribute-array element is non-nil. Chunks with
// no remaining spans are dropped because they carry no processable trace.
func removeNilEntries(tp *pb.TracerPayload) {
// normalizeDecodedPayload establishes the invariants the processing pipeline
// relies on for a decoded payload.
//
// It removes nil entries produced by legacy payload decoders: after this
// function returns, every retained chunk, span, link, event, event attribute,
// and attribute-array element is non-nil. Chunks with no remaining spans are
// dropped because they carry no processable trace.
//
// It also guarantees a non-nil Tags map on every retained chunk. The v0.7
// decoder allocates Tags only when the wire payload carries a "tags" key, while
// the v0.1/v0.4/v0.5 chunk builders always allocate one; normalizing here lets
// downstream code write chunk tags without a nil check.
func normalizeDecodedPayload(tp *pb.TracerPayload) {
chunks := compactNonNil(tp.Chunks)
keptChunks := chunks[:0]
for _, chunk := range chunks {
chunk.Spans = compactNonNil(chunk.Spans)
if len(chunk.Spans) == 0 {
continue
}
if chunk.Tags == nil {
chunk.Tags = make(map[string]string)
}
for _, span := range chunk.Spans {
span.SpanLinks = compactNonNil(span.SpanLinks)
span.SpanEvents = compactNonNil(span.SpanEvents)
Expand Down
19 changes: 18 additions & 1 deletion pkg/trace/api/api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -541,18 +541,35 @@ func TestSanitizeTracerPayload(t *testing.T) {
{Spans: []*pb.Span{nil}},
}}

removeNilEntries(payload)
normalizeDecodedPayload(payload)

require.Len(t, payload.Chunks, 1)
require.Len(t, payload.Chunks[0].Spans, 1)
assert.Same(t, span, payload.Chunks[0].Spans[0])
assert.NotNil(t, payload.Chunks[0].Tags, "retained chunks must have a non-nil Tags map")
require.Len(t, span.SpanLinks, 1)
require.Len(t, span.SpanEvents, 1)
assert.NotContains(t, span.SpanEvents[0].Attributes, "drop")
require.Len(t, arrayValue.ArrayValue.Values, 1)
assert.Equal(t, "keep", arrayValue.ArrayValue.Values[0].StringValue)
}

func TestNormalizeDecodedPayloadChunkTags(t *testing.T) {
span := &pb.Span{Service: "svc"}
payload := &pb.TracerPayload{Chunks: []*pb.TraceChunk{
{Spans: []*pb.Span{span}}, // v0.7 chunk that omitted "tags"
{Spans: []*pb.Span{span}, Tags: map[string]string{"_dd.p.dm": "-4"}}, // tags already present
}}

normalizeDecodedPayload(payload)

require.Len(t, payload.Chunks, 2)
assert.NotNil(t, payload.Chunks[0].Tags)
assert.Empty(t, payload.Chunks[0].Tags)
// An existing map must be left untouched.
assert.Equal(t, map[string]string{"_dd.p.dm": "-4"}, payload.Chunks[1].Tags)
}

func TestReceiverJSONDecoder(t *testing.T) {
// testing traces without content-type in agent endpoints, it should use JSON decoding
assert := assert.New(t)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
fixes:
- |
Fixed a crash in the trace-agent when processing a v0.7 payload whose trace
chunk omits the ``tags`` field and whose spans carry a ``_dd.p.dm`` tag.
Promoting the decision maker to the chunk level no longer writes to an
uninitialized map.
Loading