perf: improve process tags insertion by raphaelgavache · Pull Request #4010 · DataDog/dd-trace-go

raphaelgavache · 2025-10-01T03:07:04Z

What does this PR do?

Process tags were set on first span of each chunk instead of first span of first chunk which is what the agent expects. This results in a significant addition of bytes (+25%) for single span chunks with no tags!
The cost was caught by this regression test on the PR that enables process tags by default

Change of behaviour in this PR is to set process tags at the encoding level on the first span of the first chunk

NB Revisited tests to use getMeta/setMeta which are safe reads. They would trigger race conditions on map accesses when process tags are enabled without these changes

Motivation

Reviewer's Checklist

Changed code has unit tests for its functionality at or near 100% coverage.
System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
There is a benchmark for any new code, or changes to existing code.
If this interacts with the agent in a new way, a system test has been added.
New code is free of linting errors. You can check this by running ./scripts/lint.sh locally.
Add an appropriate team label so this PR gets put in the right place for the release notes.
Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.

Unsure? Have a question? Request a review!

pr-commenter · 2025-10-01T03:09:37Z

Benchmarks

Benchmark execution time: 2025-10-06 21:32:54

Comparing candidate commit a5d6611 in PR branch raphael/fix_process_tags_spans with baseline commit c7783df in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 24 metrics, 0 unstable metrics.

datadog-official · 2025-10-01T03:26:51Z

✅ Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

_{This comment will be updated automatically if new data arrives.

🔗 Commit SHA: a5d6611 | Docs | Was this helpful? Give us feedback!}

Co-authored-by: Rodrigo Argüello <[email protected]>

darccio · 2025-10-01T13:16:29Z

+		shallowCopySpan := *s //nolint:govet // to avoid the copylocks warning, the locks were not used further down
+		// favoring a copy of all fields to avoid missing one
+		shallowCopySpan.meta = metaWithProcessTags(s.meta, pTags)
+		t[i] = &shallowCopySpan


I think this is not required. Each unsafePayload function assumes that is under a lock from the same function at safePayload. Did you find race conditions during development?

I did find a race condition on map accesses of span.meta, that should be possible too on the field span.meta when overwriting it, that's why I did the copy.

I'll try to trigger that race on span.meta field

ok I did trigger the race on TestSpanProcessTags with -race otpion, you can try it out by removing the shallow copy and running

$ go test -run TestSpanProcessTags -race

================== WARNING: DATA RACE Write at 0x00c0000003c8 by goroutine 8263: github.com/DataDog/dd-trace-go/v2/ddtrace/tracer.(*unsafePayload).setTracerTags() /Users/raphael.gavache/dd/dd-trace-go/ddtrace/tracer/payload.go:141 +0x2bc github.com/DataDog/dd-trace-go/v2/ddtrace/tracer.(*unsafePayload).push() /Users/raphael.gavache/dd/dd-trace-go/ddtrace/tracer/payload.go:116 +0x40 github.com/DataDog/dd-trace-go/v2/ddtrace/tracer.(*safePayload).push() /Users/raphael.gavache/dd/dd-trace-go/ddtrace/tracer/payload.go:277 +0xa8 github.com/DataDog/dd-trace-go/v2/ddtrace/tracer.(*agentTraceWriter).add() /Users/raphael.gavache/dd/dd-trace-go/ddtrace/tracer/writer.go:74 +0x74 github.com/DataDog/dd-trace-go/v2/ddtrace/tracer.(*tracer).worker() /Users/raphael.gavache/dd/dd-trace-go/ddtrace/tracer/tracer.go:518 +0x22c github.com/DataDog/dd-trace-go/v2/ddtrace/tracer.newTracer.func2() /Users/raphael.gavache/dd/dd-trace-go/ddtrace/tracer/tracer.go:473 +0x10c Previous read at 0x00c0000003c8 by goroutine 8248: github.com/DataDog/dd-trace-go/v2/ddtrace/tracer.(*concentrator).newTracerStatSpan() ... ================== --- FAIL: TestSpanProcessTags (0.02s) --- FAIL: TestSpanProcessTags/enabled (0.01s) testing.go:1490: race detected during execution of test

what about just using the span mutex?

s.mu.Lock() s.meta[keyProcessTags] = pTags s.mu.Unlock()

Given this is only done for the first span in the first chunk, it shouldn't add much lock contention and I think it should be slightly better performance-wise. WDYT? @darccio @raphaelgavache

looks good to me, nice call out
I don't know the implications on your side though, as it forces the span.finishes and especially stat computation to be completed before merging. From afar that doesn't seem a problem

updated with the lock, all tests pass

@kakkoyun Could you check this, please? Specially, if you can check this lock with the checklocks tooling, that'd be interesting.

kakkoyun

This is looking good.

I have only want comment.

mtoffl01

Overall approach LGTM, just some nits.

Also, some feedback on the setTracerTags:

setProcessTags may be a better name as its more descriptive/accurate
Does it really need to be a method on unsafePayload? It could just take in a []*Span and set tags on the first one in the slice. Then you can move the if atomic.LoadUint32(&p.count) != 0 check to the unsafePayload.push method; if true, you call the set tags method.

I'm holding off on approving until I get confirmation about the Github Actions checks that are failing about the use of atomics 🤔 (reaching out now)

mtoffl01 · 2025-10-06T16:22:14Z

+	if len(t) == 0 {
+		return
+	}


Why not move this above line 131? In this case, we can avoid processing lines 131-134 entirely.

mtoffl01 · 2025-10-06T16:27:22Z

 // TestPayloadDecode ensures that whatever we push into the payload can
 // be decoded by the codec.
 func TestPayloadDecode(t *testing.T) {
+	t.Setenv("DD_EXPERIMENTAL_PROPAGATE_PROCESS_TAGS_ENABLED", "true")


Code quality preference: I'd probably prefer making envProcessTagsEnabled public, and using that here instead of the raw string version.

this env var will stop being called in the PR that follows where process tags are enabled by default:
#4008
I won't expose it as it will become public and not useful

mtoffl01 · 2025-10-06T16:30:23Z

+	processtags.Reload()
+	defer func() {
+		t.Setenv("DD_EXPERIMENTAL_PROPAGATE_PROCESS_TAGS_ENABLED", "false")
+		processtags.Reload()
+	}()


No need to reset this when using t.SetEnv (https://pkg.go.dev/testing#T.Setenv)

Regarding the duplicate content in lines 69 and 72 -- judging by the name of the function, I'd expect Reload to be called at the start of a test, not at the end. Do we need line 72?

the reload call changes a global var/state of the process, so if we want this test not to impact all other tests run in the same process (tests run after the execution of this one), we have to reset

mtoffl01

I still think setTracerTags should be called setProcessTags, but other than that - LGTM!

raphaelgavache · 2025-10-07T14:28:33Z

/merge

dd-devflow-routing-codex · 2025-10-07T14:28:37Z

View all feedbacks in Devflow UI.

2025-10-07 14:28:37 UTC ℹ️ Start processing command /merge

2025-10-07 14:28:43 UTC ℹ️ MergeQueue: pull request added to the queue

The expected merge time in main is approximately 18m (p90).

2025-10-07 14:44:51 UTC ℹ️ MergeQueue: This merge request was merged

raphaelgavache changed the title ~~improve process tags insertion~~ perf: improve process tags insertion Oct 1, 2025

raphaelgavache force-pushed the raphael/fix_process_tags_spans branch from 7dc6b29 to 8a5339e Compare October 1, 2025 03:09

improve process tags insertion

17e7345

raphaelgavache force-pushed the raphael/fix_process_tags_spans branch from 8a5339e to 17e7345 Compare October 1, 2025 03:12

raphaelgavache force-pushed the raphael/fix_process_tags_spans branch from 1976f52 to 17e7345 Compare October 1, 2025 03:29

raphaelgavache commented Oct 1, 2025

View reviewed changes

Comment thread ddtrace/tracer/payload.go Outdated

raphaelgavache mentioned this pull request Oct 1, 2025

feat: enable process tags by default #4008

Merged

7 tasks

a bit safer

7769535

raphaelgavache force-pushed the raphael/fix_process_tags_spans branch from 290b71b to 7769535 Compare October 1, 2025 04:20

cleanup after test

ad9f2e6

raphaelgavache added the core label Oct 1, 2025

raphaelgavache marked this pull request as ready for review October 1, 2025 04:43

raphaelgavache requested review from a team as code owners October 1, 2025 04:43

raphaelgavache requested review from genesor and rarguelloF October 1, 2025 04:43

skip lint check

28f42b0

raphaelgavache force-pushed the raphael/fix_process_tags_spans branch from 328f168 to 28f42b0 Compare October 1, 2025 04:47

rarguelloF reviewed Oct 1, 2025

View reviewed changes

Comment thread ddtrace/tracer/payload_test.go Outdated

Comment thread ddtrace/tracer/payload_test.go Outdated

Comment thread ddtrace/tracer/payload_test.go Outdated

Comment thread ddtrace/tracer/payload.go Outdated

Apply suggestions from code review

5795de5

Co-authored-by: Rodrigo Argüello <[email protected]>

darccio reviewed Oct 1, 2025

View reviewed changes

raphaelgavache added 3 commits October 1, 2025 11:47

Merge branch 'main' into raphael/fix_process_tags_spans

c71ad09

Merge branch 'main' into raphael/fix_process_tags_spans

53b9cbf

use span lock instead

e8252f6

kakkoyun reviewed Oct 6, 2025

View reviewed changes

Comment thread ddtrace/tracer/payload.go Outdated

update after review

e225c89

raphaelgavache commented Oct 6, 2025

View reviewed changes

Comment thread ddtrace/tracer/payload.go Outdated

first span is not nil, called in sampleChunk

f3c8258

darccio approved these changes Oct 6, 2025

View reviewed changes

kakkoyun approved these changes Oct 6, 2025

View reviewed changes

mtoffl01 reviewed Oct 6, 2025

View reviewed changes

raphaelgavache added 7 commits October 6, 2025 19:23

update after review

e3e7d02

first batch of unsafe test meta accesses

9401e31

other batch of test

468fd5d

update sampler tests

088c9ba

disable process tags in async lock test

2ca6d5b

fix flaky test

74ca96d

Merge branch 'main' into raphael/fix_process_tags_spans

a5d6611

mtoffl01 approved these changes Oct 7, 2025

View reviewed changes

dd-devflow Bot added mergequeue-status: queued mergequeue-status: in_progress and removed mergequeue-status: queued labels Oct 7, 2025

dd-mergequeue Bot merged commit 23545d4 into main Oct 7, 2025
239 checks passed

dd-mergequeue Bot deleted the raphael/fix_process_tags_spans branch October 7, 2025 14:44

dd-devflow Bot added mergequeue-status: done and removed mergequeue-status: in_progress labels Oct 7, 2025

raphaelgavache mentioned this pull request Oct 15, 2025

[Tracer] Add new process tags DataDog/dd-trace-dotnet#7651

Merged

darccio added a commit that referenced this pull request Oct 16, 2025

chore: apply changes from #4010

bbb62d2

Uh oh!

Conversation

raphaelgavache commented Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Reviewer's Checklist

Uh oh!

pr-commenter Bot commented Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks

Uh oh!

datadog-official Bot commented Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

darccio Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rarguelloF Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kakkoyun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mtoffl01 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raphaelgavache Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtoffl01 left a comment

Choose a reason for hiding this comment

Uh oh!

raphaelgavache commented Oct 7, 2025

Uh oh!

dd-devflow-routing-codex Bot commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

raphaelgavache commented Oct 1, 2025 •

edited

Loading

pr-commenter Bot commented Oct 1, 2025 •

edited

Loading

datadog-official Bot commented Oct 1, 2025 •

edited

Loading

darccio Oct 1, 2025 •

edited

Loading

rarguelloF Oct 2, 2025 •

edited

Loading

raphaelgavache Oct 6, 2025 •

edited

Loading

dd-devflow-routing-codex Bot commented Oct 7, 2025 •

edited

Loading