Skip to content

Commit 25a2304

Browse files
fix(serverless-init): eliminate GlobalTags data race in MicroVM trace tags
Codex flagged a data race on PR #53036: MicroVM's /run lifecycle hook calls serverlessTraceAgent.SetTags from an async goroutine, which mutates Agent.GlobalTags via SetGlobalTagsUnsafe concurrently with the trace agent's span-processing loop reading that same field unsynchronized. GlobalTags is computed once at config-build time and every reader in pkg/trace/agent and pkg/trace/api assumes it is frozen thereafter; MicroVM's dynamic lambda_microvm_id update breaks that invariant. Route the async update through the span modifier instead, which already runs at the identical point in the span-processing loop and is exclusively serverless-owned: - pkg/serverless/trace/span_modifier.go: tags field is now atomic.Pointer[map[string]string]; ModifySpan reads it lock-free instead of reading a field written by a separate mutator with no synchronization. - pkg/serverless/trace/trace.go: new UpdateRuntimeTags method that only updates the span modifier, never GlobalTags. Existing SetTags (used once synchronously at startup, before the trace agent runs) is untouched. - cmd/serverless-init/main.go: the two async TraceTagSetterFunc closures now call UpdateRuntimeTags instead of SetTags. This is scoped entirely to MicroVM: every other cloud service ignores LifecycleCtx in Init, so they never exercise the async path and keep calling the original SetTags at startup unchanged. Also fixes an issue flagged by Codex review: ModifySpan's new tag-apply loop unconditionally overwrote _dd.origin whenever the tags map contained it, undoing the "only fill _dd.origin if absent" guard immediately above it. Every CloudService.GetTags() sets _dd.origin (not just MicroVM's), and that value flows into the tags applied here via SetTags/UpdateRuntimeTags at startup for every cloud service — so this would have silently overwritten a tracer-supplied span origin (e.g. _dd.origin:rum) for all of them, not just MicroVM. Fixed by skipping _dd.origin in the loop. Covered by TestSpanModifierModifySpanPreservesExistingOrigin. A second Codex finding on this PR — UpdateRuntimeTags not reaching spans processed via the V1 payload path (ProcessV1 skips SpanModifier) — is left as a known follow-up; not fixed here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 1bf0f6c commit 25a2304

6 files changed

Lines changed: 182 additions & 5 deletions

File tree

cmd/serverless-init/lifecycle/server.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,7 @@ type LogsTagSetterFunc func([]string)
143143
func (f LogsTagSetterFunc) SetLogsTags(tags []string) { f(tags) }
144144

145145
// TraceTagSetter can replace the full tag map on the live trace pipeline.
146-
// Satisfied by trace.ServerlessTraceAgent.SetTags (wrapped via TraceTagSetterFunc).
146+
// Satisfied by trace.ServerlessTraceAgent.UpdateRuntimeTags (wrapped via TraceTagSetterFunc).
147147
type TraceTagSetter interface {
148148
SetTraceTags(tags map[string]string)
149149
}

cmd/serverless-init/main.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -496,7 +496,7 @@ func setup(
496496
}),
497497
BaseTags: logTagsBase,
498498
TraceTagSetter: lifecycle.TraceTagSetterFunc(func(tags map[string]string) {
499-
traceAgent.SetTags(tags)
499+
traceAgent.UpdateRuntimeTags(tags)
500500
}),
501501
BaseTraceTags: serverlessInitTag.MakeTraceAgentTags(tagConfig.Tags),
502502
},
@@ -532,7 +532,7 @@ func setup(
532532
}),
533533
BaseTags: logTagsBase,
534534
TraceTagSetter: lifecycle.TraceTagSetterFunc(func(tags map[string]string) {
535-
traceAgent.SetTags(tags)
535+
traceAgent.UpdateRuntimeTags(tags)
536536
}),
537537
BaseTraceTags: serverlessInitTag.MakeTraceAgentTags(tagConfig.Tags),
538538
},

pkg/serverless/trace/span_modifer_test.go

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import (
1717

1818
"github.com/DataDog/datadog-agent/cmd/serverless-init/cloudservice"
1919
gzip "github.com/DataDog/datadog-agent/comp/trace/compression/impl-gzip"
20+
pb "github.com/DataDog/datadog-agent/pkg/proto/pbgo/trace"
2021
"github.com/DataDog/datadog-agent/pkg/trace/agent"
2122
"github.com/DataDog/datadog-agent/pkg/trace/api"
2223
"github.com/DataDog/datadog-agent/pkg/trace/config"
@@ -109,3 +110,89 @@ func TestSpanModifierDetectsCloudService(t *testing.T) {
109110
os.Unsetenv(cloudServiceEnvVar)
110111
}
111112
}
113+
114+
// TestSpanModifierModifySpanBeforeSetTags verifies that ModifySpan only
115+
// applies the _dd.origin tag when SetTags has never been called, since
116+
// spanModifier.tags starts as an unset atomic.Pointer.
117+
func TestSpanModifierModifySpanBeforeSetTags(t *testing.T) {
118+
sm := &spanModifier{ddOrigin: "lambda"}
119+
span := &pb.Span{Meta: map[string]string{}}
120+
121+
sm.ModifySpan(&pb.TraceChunk{}, span)
122+
123+
assert.Equal(t, "lambda", span.Meta[ddOriginTagName])
124+
assert.Len(t, span.Meta, 1)
125+
}
126+
127+
// TestSpanModifierModifySpanAppliesTagsSetDynamically verifies that tags
128+
// applied via SetTags after construction are picked up by ModifySpan, the
129+
// mechanism MicroVM uses to deliver the lambda_microvm_id tag once it becomes
130+
// known at /run time.
131+
func TestSpanModifierModifySpanAppliesTagsSetDynamically(t *testing.T) {
132+
sm := &spanModifier{ddOrigin: "lambda"}
133+
sm.SetTags(map[string]string{"lambda_microvm_id": "vm-123"})
134+
135+
span := &pb.Span{Meta: map[string]string{}}
136+
sm.ModifySpan(&pb.TraceChunk{}, span)
137+
138+
assert.Equal(t, "lambda", span.Meta[ddOriginTagName])
139+
assert.Equal(t, "vm-123", span.Meta["lambda_microvm_id"])
140+
}
141+
142+
// TestSpanModifierModifySpanPreservesExistingOrigin verifies that ModifySpan
143+
// does not overwrite a span's existing _dd.origin when the dynamically-set
144+
// tags also contain _dd.origin. Every CloudService.GetTags() sets _dd.origin
145+
// (e.g. MicroVM sets "lambdamicrovm"), and that value flows into the tags
146+
// applied here via SetTags/UpdateRuntimeTags — so without this guard, every
147+
// span would have a tracer-supplied origin (e.g. "rum") silently replaced.
148+
func TestSpanModifierModifySpanPreservesExistingOrigin(t *testing.T) {
149+
sm := &spanModifier{ddOrigin: "lambda"}
150+
sm.SetTags(map[string]string{ddOriginTagName: "lambdamicrovm", "lambda_microvm_id": "vm-123"})
151+
152+
span := &pb.Span{Meta: map[string]string{ddOriginTagName: "rum"}}
153+
sm.ModifySpan(&pb.TraceChunk{}, span)
154+
155+
assert.Equal(t, "rum", span.Meta[ddOriginTagName], "must not overwrite a tracer-supplied origin")
156+
assert.Equal(t, "vm-123", span.Meta["lambda_microvm_id"])
157+
}
158+
159+
// TestSpanModifierModifySpanReflectsLatestSetTags verifies that a later
160+
// SetTags call replaces the tag set used by subsequent ModifySpan calls.
161+
func TestSpanModifierModifySpanReflectsLatestSetTags(t *testing.T) {
162+
sm := &spanModifier{ddOrigin: "lambda"}
163+
sm.SetTags(map[string]string{"lambda_microvm_id": "vm-1"})
164+
sm.SetTags(map[string]string{"lambda_microvm_id": "vm-2"})
165+
166+
span := &pb.Span{Meta: map[string]string{}}
167+
sm.ModifySpan(&pb.TraceChunk{}, span)
168+
169+
assert.Equal(t, "vm-2", span.Meta["lambda_microvm_id"])
170+
}
171+
172+
// TestSpanModifierSetTagsConcurrentWithModifySpan exercises SetTags and
173+
// ModifySpan concurrently under the race detector. This is a regression test
174+
// for the data race Codex flagged on PR #53036: MicroVM's /run hook calls
175+
// SetTags from a goroutine that runs concurrently with the trace agent's
176+
// span-processing loop, which calls ModifySpan on every span.
177+
func TestSpanModifierSetTagsConcurrentWithModifySpan(t *testing.T) {
178+
sm := &spanModifier{ddOrigin: "lambda"}
179+
180+
var wg sync.WaitGroup
181+
wg.Add(2)
182+
go func() {
183+
defer wg.Done()
184+
for i := 0; i < 1000; i++ {
185+
sm.SetTags(map[string]string{"lambda_microvm_id": "vm-1"})
186+
}
187+
}()
188+
go func() {
189+
defer wg.Done()
190+
for i := 0; i < 1000; i++ {
191+
span := &pb.Span{Meta: map[string]string{}}
192+
sm.ModifySpan(&pb.TraceChunk{}, span)
193+
}
194+
}()
195+
wg.Wait()
196+
197+
assert.Equal(t, map[string]string{"lambda_microvm_id": "vm-1"}, *sm.tags.Load())
198+
}

pkg/serverless/trace/span_modifier.go

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@
77
package trace
88

99
import (
10+
"go.uber.org/atomic"
11+
1012
pb "github.com/DataDog/datadog-agent/pkg/proto/pbgo/trace"
1113
"github.com/DataDog/datadog-agent/pkg/trace/traceutil"
1214
)
@@ -16,7 +18,7 @@ const (
1618
)
1719

1820
type spanModifier struct {
19-
tags map[string]string
21+
tags atomic.Pointer[map[string]string]
2022
ddOrigin string
2123
}
2224

@@ -26,9 +28,17 @@ func (s *spanModifier) ModifySpan(_ *pb.TraceChunk, span *pb.Span) {
2628
if origin := span.Meta[ddOriginTagName]; origin == "" {
2729
traceutil.SetMeta(span, ddOriginTagName, s.ddOrigin)
2830
}
31+
if tags := s.tags.Load(); tags != nil {
32+
for k, v := range *tags {
33+
if k == ddOriginTagName {
34+
continue
35+
}
36+
traceutil.SetMeta(span, k, v)
37+
}
38+
}
2939
}
3040

3141
// SetTags sets the tags to be used by the span modifier.
3242
func (s *spanModifier) SetTags(tags map[string]string) {
33-
s.tags = tags
43+
s.tags.Store(&tags)
3444
}

pkg/serverless/trace/trace.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ type ServerlessTraceAgent interface {
4343
Flush()
4444
Process(p *api.Payload)
4545
SetTags(map[string]string)
46+
UpdateRuntimeTags(map[string]string)
4647
SetTargetTPS(float64)
4748
SetSpanModifier(agent.SpanModifier)
4849
GetSpanModifier() agent.SpanModifier
@@ -204,6 +205,15 @@ func (t *serverlessTraceAgent) SetTags(tags map[string]string) {
204205
}
205206
}
206207

208+
// UpdateRuntimeTags updates the tags applied by the span modifier only. Unlike
209+
// SetTags, it does not touch the trace agent's GlobalTags, so it is safe to
210+
// call concurrently with span processing.
211+
func (t *serverlessTraceAgent) UpdateRuntimeTags(tags map[string]string) {
212+
if tagger, ok := t.ta.SpanModifier.(taggable); ok {
213+
tagger.SetTags(tags)
214+
}
215+
}
216+
207217
// Stop cancels the trace agent's context and waits for its Run loop to finish.
208218
// The Run loop handles the full shutdown sequence: draining in-flight traces,
209219
// flushing stats producers, sending buffered data to the network, and stopping
@@ -300,6 +310,7 @@ func (t noopTraceAgent) Stop() {}
300310
func (t noopTraceAgent) Flush() {}
301311
func (t noopTraceAgent) Process(*api.Payload) {}
302312
func (t noopTraceAgent) SetTags(map[string]string) {}
313+
func (t noopTraceAgent) UpdateRuntimeTags(map[string]string) {}
303314
func (t noopTraceAgent) SetTargetTPS(float64) {}
304315
func (t noopTraceAgent) SetSpanModifier(agent.SpanModifier) {}
305316
func (t noopTraceAgent) GetSpanModifier() agent.SpanModifier { return nil }

pkg/serverless/trace/trace_test.go

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
package trace
99

1010
import (
11+
"context"
1112
"errors"
1213
"os"
1314
"path/filepath"
@@ -19,13 +20,31 @@ import (
1920
"github.com/stretchr/testify/require"
2021

2122
"github.com/DataDog/datadog-agent/cmd/serverless-init/cloudservice"
23+
gzip "github.com/DataDog/datadog-agent/comp/trace/compression/impl-gzip"
2224
configmock "github.com/DataDog/datadog-agent/pkg/config/mock"
2325
pkgconfigsetup "github.com/DataDog/datadog-agent/pkg/config/setup"
2426
pb "github.com/DataDog/datadog-agent/pkg/proto/pbgo/trace"
27+
"github.com/DataDog/datadog-agent/pkg/trace/agent"
2528
"github.com/DataDog/datadog-agent/pkg/trace/config"
29+
"github.com/DataDog/datadog-agent/pkg/trace/telemetry"
2630
"github.com/DataDog/datadog-agent/pkg/trace/testutil"
31+
32+
"github.com/DataDog/datadog-go/v5/statsd"
2733
)
2834

35+
// newTestServerlessTraceAgent builds a minimal serverlessTraceAgent backed by
36+
// a real *agent.Agent and spanModifier, without starting the agent's Run loop.
37+
func newTestServerlessTraceAgent(t *testing.T) (*serverlessTraceAgent, *config.AgentConfig) {
38+
t.Helper()
39+
cfg := config.New()
40+
cfg.Endpoints[0].APIKey = "test"
41+
ctx, cancel := context.WithCancel(context.Background())
42+
t.Cleanup(cancel)
43+
ta := agent.NewAgent(ctx, cfg, telemetry.NewNoopCollector(), &statsd.NoOpClient{}, gzip.NewComponent())
44+
ta.SpanModifier = &spanModifier{ddOrigin: "lambda"}
45+
return &serverlessTraceAgent{ta: ta, cancel: cancel}, cfg
46+
}
47+
2948
func setupTraceAgentTest(t *testing.T) {
3049
// ensure a free port is used for starting the trace agent
3150
port, err := testutil.FindTCPPort()
@@ -247,3 +266,53 @@ func TestServerlessTraceAgentDisableTraceStats(t *testing.T) {
247266
})
248267
}
249268
}
269+
270+
// TestServerlessTraceAgentSetTagsUpdatesGlobalTagsAndSpanModifier verifies
271+
// that the synchronous SetTags path (used once at startup, before the trace
272+
// agent starts processing spans) still updates both GlobalTags and the span
273+
// modifier, as it did before this fix.
274+
func TestServerlessTraceAgentSetTagsUpdatesGlobalTagsAndSpanModifier(t *testing.T) {
275+
sta, cfg := newTestServerlessTraceAgent(t)
276+
277+
sta.SetTags(map[string]string{"lambda_microvm_id": "vm-1"})
278+
279+
assert.Equal(t, map[string]string{"lambda_microvm_id": "vm-1"}, cfg.GlobalTags)
280+
281+
sm, ok := sta.ta.SpanModifier.(*spanModifier)
282+
require.True(t, ok)
283+
got := sm.tags.Load()
284+
require.NotNil(t, got)
285+
assert.Equal(t, map[string]string{"lambda_microvm_id": "vm-1"}, *got)
286+
}
287+
288+
// TestServerlessTraceAgentUpdateRuntimeTagsDoesNotTouchGlobalTags is the core
289+
// regression test for the fix: UpdateRuntimeTags (used by MicroVM's async
290+
// /run hook) must only update the span modifier and must never write to
291+
// GlobalTags, since GlobalTags is read unsynchronized by the trace agent's
292+
// span-processing hot path.
293+
func TestServerlessTraceAgentUpdateRuntimeTagsDoesNotTouchGlobalTags(t *testing.T) {
294+
sta, cfg := newTestServerlessTraceAgent(t)
295+
cfg.GlobalTags = map[string]string{"env": "prod"}
296+
297+
sta.UpdateRuntimeTags(map[string]string{"lambda_microvm_id": "vm-2"})
298+
299+
assert.Equal(t, map[string]string{"env": "prod"}, cfg.GlobalTags)
300+
301+
sm, ok := sta.ta.SpanModifier.(*spanModifier)
302+
require.True(t, ok)
303+
got := sm.tags.Load()
304+
require.NotNil(t, got)
305+
assert.Equal(t, map[string]string{"lambda_microvm_id": "vm-2"}, *got)
306+
}
307+
308+
// TestServerlessTraceAgentUpdateRuntimeTagsNoSpanModifier verifies
309+
// UpdateRuntimeTags is safe to call when SpanModifier doesn't implement
310+
// taggable (e.g. nil, or some other SpanModifier set via SetSpanModifier).
311+
func TestServerlessTraceAgentUpdateRuntimeTagsNoSpanModifier(t *testing.T) {
312+
sta, _ := newTestServerlessTraceAgent(t)
313+
sta.ta.SpanModifier = nil
314+
315+
assert.NotPanics(t, func() {
316+
sta.UpdateRuntimeTags(map[string]string{"lambda_microvm_id": "vm-3"})
317+
})
318+
}

0 commit comments

Comments
 (0)