Skip to content

Commit ef7877e

Browse files
darcciodarccio
authored
fix(ddtrace/tracer): pre-compute _dd.p.dm numeric value at write-time (#4576)
### What does this PR do? Moves the `strconv.ParseInt` call for `_dd.p.dm` (decision maker) out of the v1 payload flush path and into the write path. A `dm uint32` field is added to the `trace` struct. It is kept in sync with `propagatingTags[keyDecisionMaker]` at all mutation sites (`setPropagatingTagLocked`, `unsetPropagatingTagLocked`, `replacePropagatingTags`). A shared `unsetPropagatingTagLocked` helper is introduced so the delete-and-clear logic is not duplicated between `unsetPropagatingTag` and `setSamplingPriorityLockedWithForce`. `payloadV1.push` now reads the pre-computed value via `trace.decisionMaker()` instead of fetching the string from the propagating tags map and parsing it on every flush. ### Motivation `payloadV1.push` is called once per finished trace chunk. On every call it was acquiring a read lock, doing a map lookup, and then running `strconv.ParseInt` — all to read a value that was set at most once or twice during the lifetime of the trace. Moving the parse to write-time eliminates the per-flush map lookup and parse, replacing them with a single scalar read behind an `RLock`. ### Reviewer's Checklist - [x] Changed code has unit tests for its functionality at or near 100% coverage. - [x] New code is free of linting errors. You can check this by running `make lint` locally. - [x] New code doesn't break existing tests. You can check this by running `make test` locally. - [x] Add an appropriate team label so this PR gets put in the right place for the release notes. - [x] All generated files are up to date. You can check this by running `make generate` locally. Co-authored-by: dario.castane <[email protected]>
1 parent 4e1ef08 commit ef7877e

4 files changed

Lines changed: 80 additions & 14 deletions

File tree

‎ddtrace/tracer/payload_v1.go‎

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import (
1010
"encoding/binary"
1111
"errors"
1212
"fmt"
13-
"strconv"
1413
"sync"
1514
"sync/atomic"
1615

@@ -189,16 +188,7 @@ func (p *payloadV1) push(t spanList) (stats payloadStats, err error) {
189188
// TODO(darccio): are we sure that origin will be shared across all the spans in the chunk?
190189
origin = span.Context().origin // +checklocksignore - Read-only after init.
191190

192-
if dm := span.context.trace.propagatingTag(keyDecisionMaker); dm != "" {
193-
if v, err := strconv.ParseInt(dm, 10, 32); err == nil {
194-
if v < 0 {
195-
v = -v
196-
}
197-
sm = uint32(v)
198-
} else {
199-
log.Error("failed to convert decision maker to uint32: %s", err.Error())
200-
}
201-
}
191+
sm = span.context.trace.decisionMaker()
202192
}
203193
tc := traceChunk{
204194
spans: t,

‎ddtrace/tracer/propagating_tags.go‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66
package tracer
77

88
import (
9+
"strconv"
10+
911
"github.com/DataDog/dd-trace-go/v2/internal"
1012
"github.com/DataDog/dd-trace-go/v2/internal/locking/assert"
1113
"github.com/DataDog/dd-trace-go/v2/internal/log"
@@ -61,13 +63,25 @@ func (t *trace) setPropagatingTagLocked(key, value string) {
6163
t.propagatingTags = make(map[string]string, 1)
6264
}
6365
t.propagatingTags[key] = value
66+
if key == keyDecisionMaker {
67+
t.dm = parseDecisionMaker(value)
68+
}
6469
}
6570

6671
// unsetPropagatingTag deletes the key/value pair from the trace's propagated tags.
6772
func (t *trace) unsetPropagatingTag(key string) {
6873
t.mu.Lock()
6974
defer t.mu.Unlock()
75+
t.unsetPropagatingTagLocked(key)
76+
}
77+
78+
// +checklocks:t.mu
79+
func (t *trace) unsetPropagatingTagLocked(key string) {
80+
assert.RWMutexLocked(&t.mu)
7081
delete(t.propagatingTags, key)
82+
if key == keyDecisionMaker {
83+
t.dm = 0
84+
}
7185
}
7286

7387
// iteratePropagatingTags allows safe iteration through the propagating tags of a trace.
@@ -88,10 +102,35 @@ func (t *trace) replacePropagatingTags(tags map[string]string) {
88102
t.mu.Lock()
89103
defer t.mu.Unlock()
90104
t.propagatingTags = tags
105+
if dm, ok := tags[keyDecisionMaker]; ok {
106+
t.dm = parseDecisionMaker(dm)
107+
} else {
108+
t.dm = 0
109+
}
91110
}
92111

93112
func (t *trace) propagatingTagsLen() int {
94113
t.mu.RLock()
95114
defer t.mu.RUnlock()
96115
return len(t.propagatingTags)
97116
}
117+
118+
// parseDecisionMaker parses the decision maker string (e.g. "-4") into
119+
// its absolute uint32 form for v1 protocol encoding.
120+
func parseDecisionMaker(dm string) uint32 {
121+
v, err := strconv.ParseInt(dm, 10, 32)
122+
if err != nil {
123+
log.Error("failed to convert decision maker to uint32: %s", err.Error())
124+
return 0
125+
}
126+
if v < 0 {
127+
v = -v
128+
}
129+
return uint32(v)
130+
}
131+
132+
func (t *trace) decisionMaker() uint32 {
133+
t.mu.RLock()
134+
defer t.mu.RUnlock()
135+
return t.dm
136+
}

‎ddtrace/tracer/spancontext.go‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -196,8 +196,11 @@ func FromGenericCtx(c ddtrace.SpanContext) *SpanContext {
196196
if sc.trace == nil {
197197
sc.trace = newTrace()
198198
}
199-
sc.trace.tags = ctx.Tags() // +checklocksignore - Initialization time, not shared yet.
200-
sc.trace.propagatingTags = ctx.PropagatingTags() // +checklocksignore - Initialization time, not shared yet.
199+
sc.trace.tags = ctx.Tags() // +checklocksignore - Initialization time, not shared yet.
200+
sc.trace.propagatingTags = ctx.PropagatingTags() // +checklocksignore - Initialization time, not shared yet.
201+
if dm, ok := sc.trace.propagatingTags[keyDecisionMaker]; ok { // +checklocksignore - Initialization time, not shared yet.
202+
sc.trace.dm = parseDecisionMaker(dm) // +checklocksignore - Initialization time, not shared yet.
203+
}
201204
return &sc
202205
}
203206

@@ -464,6 +467,10 @@ type trace struct {
464467
// specifies if the sampling priority can be altered
465468
// +checklocks:mu
466469
locked bool
470+
// dm is the numeric form of _dd.p.dm for v1 protocol encoding.
471+
// It is the absolute value of the parsed integer (e.g., "-4" → 4).
472+
// +checklocks:mu
473+
dm uint32
467474
// samplingDecision indicates whether to send the trace to the agent.
468475
samplingDecision samplingDecision // +checkatomic
469476

@@ -600,7 +607,7 @@ func (t *trace) setSamplingPriorityLockedWithForce(p int, sampler samplernames.S
600607
}
601608
}
602609
if p <= 0 && existed {
603-
delete(t.propagatingTags, keyDecisionMaker)
610+
t.unsetPropagatingTagLocked(keyDecisionMaker)
604611
}
605612

606613
return updatedPriority

‎ddtrace/tracer/spancontext_test.go‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1337,3 +1337,33 @@ func BenchmarkUpdateTracerGitMetadataTags(b *testing.B) {
13371337
}
13381338
})
13391339
}
1340+
1341+
// genericCtxWithDM is a minimal ddtrace.SpanContext + spanContextV1Adapter that
1342+
// carries a _dd.p.dm propagating tag, simulating a context arriving from an
1343+
// external (non-native) integration.
1344+
type genericCtxWithDM struct {
1345+
propagatingTags map[string]string
1346+
}
1347+
1348+
func (g *genericCtxWithDM) SpanID() uint64 { return 1 }
1349+
func (g *genericCtxWithDM) TraceID() string { return "1" }
1350+
func (g *genericCtxWithDM) TraceIDBytes() [16]byte { var b [16]byte; b[15] = 1; return b }
1351+
func (g *genericCtxWithDM) TraceIDLower() uint64 { return 1 }
1352+
func (g *genericCtxWithDM) ForeachBaggageItem(_ func(k, v string) bool) {}
1353+
func (g *genericCtxWithDM) SamplingDecision() uint32 { return uint32(decisionKeep) }
1354+
func (g *genericCtxWithDM) Priority() *float64 { p := 1.0; return &p }
1355+
func (g *genericCtxWithDM) Origin() string { return "" }
1356+
func (g *genericCtxWithDM) PropagatingTags() map[string]string { return g.propagatingTags }
1357+
func (g *genericCtxWithDM) Tags() map[string]string { return nil }
1358+
1359+
// TestFromGenericCtxDecisionMakerCached verifies that FromGenericCtx populates
1360+
// t.dm so that trace.decisionMaker() returns the correct sampling mechanism for
1361+
// traces arriving through generic (non-native) context adapters.
1362+
func TestFromGenericCtxDecisionMakerCached(t *testing.T) {
1363+
ctx := &genericCtxWithDM{
1364+
propagatingTags: map[string]string{keyDecisionMaker: "-4"},
1365+
}
1366+
sc := FromGenericCtx(ctx)
1367+
require.NotNil(t, sc.trace)
1368+
assert.Equal(t, uint32(4), sc.trace.decisionMaker())
1369+
}

0 commit comments

Comments
 (0)