Skip to content

internal/datastreams: add process tags to dsm payloads#3585

Merged
kakkoyun merged 4 commits into
mainfrom
rarguelloF/process-tags-dsm
Jun 6, 2025
Merged

internal/datastreams: add process tags to dsm payloads#3585
kakkoyun merged 4 commits into
mainfrom
rarguelloF/process-tags-dsm

Conversation

@rarguelloF

@rarguelloF rarguelloF commented Jun 2, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

What does this PR do?

Followup of #3566

Adds ProcessTags to DSM payloads.

(needed to update the hashing logic to include process tags as well).

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 golangci-lint run 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

pr-commenter Bot commented Jun 2, 2025 •
edited
Loading

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2025-06-06 15:58:32

Comparing candidate commit 03e5301 in PR branch rarguelloF/process-tags-dsm with baseline commit 5ea1efb in branch main.

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

piochelepiotr
piochelepiotr reviewed Jun 3, 2025
View reviewed changes

@piochelepiotr piochelepiotr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main question about performance. Is there a way to add a RW lock on the value of process tags, and then copy on write if the value is changed?
I want to make sure the SetCheckpoint method is not impacted performance wise.

Comment thread internal/datastreams/processor.go
Comment thread internal/datastreams/processor.go Outdated
@rarguelloF

rarguelloF commented Jun 4, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@piochelepiotr Leaving here the benchmarks for posterity:

Ran the existing benchmark on my machine:

// 1st run

goos: darwin
goarch: arm64
pkg: github.com/DataDog/dd-trace-go/v2/internal/datastreams
cpu: Apple M1 Max
BenchmarkSetCheckpoint
BenchmarkSetCheckpoint-10    	 3214754	       374.0 ns/op
PASS

Process finished with the exit code 0

// 2nd run

goos: darwin
goarch: arm64
pkg: github.com/DataDog/dd-trace-go/v2/internal/datastreams
cpu: Apple M1 Max
BenchmarkSetCheckpoint
BenchmarkSetCheckpoint-10    	 3276264	       365.8 ns/op
PASS

Process finished with the exit code 0

New benchmark enabling process tags:

// 1st run

goos: darwin
goarch: arm64
pkg: github.com/DataDog/dd-trace-go/v2/internal/datastreams
cpu: Apple M1 Max
BenchmarkSetCheckpointProcessTags
BenchmarkSetCheckpointProcessTags-10    	 3170246	       376.6 ns/op
PASS

Process finished with the exit code 0

// 2nd run

goos: darwin
goarch: arm64
pkg: github.com/DataDog/dd-trace-go/v2/internal/datastreams
cpu: Apple M1 Max
BenchmarkSetCheckpointProcessTags
BenchmarkSetCheckpointProcessTags-10    	 3138550	       372.7 ns/op
PASS

Process finished with the exit code 0

Tested a version of processtags using atomic instead of RWMutex:

goos: darwin
goarch: arm64
pkg: github.com/DataDog/dd-trace-go/v2/internal/datastreams
cpu: Apple M1 Max
BenchmarkSetCheckpointProcessTags
BenchmarkSetCheckpointProcessTags-10    	 3209713	       368.0 ns/op
PASS

Process finished with the exit code 0

In the end I left it the same as it is because there are no notable differences.

@rarguelloF
rarguelloF marked this pull request as ready for review June 4, 2025 14:22
@rarguelloF
rarguelloF requested review from a team as code owners June 4, 2025 14:22
@rarguelloF
rarguelloF requested a review from amarziali June 4, 2025 14:24
amarziali
amarziali reviewed Jun 5, 2025
View reviewed changes
Comment thread internal/datastreams/hash_cache.go
kakkoyun
kakkoyun reviewed Jun 5, 2025
View reviewed changes
Comment thread internal/datastreams/pathway.go Outdated
@rarguelloF
rarguelloF requested a review from kakkoyun June 5, 2025 11:01
@kakkoyun
kakkoyun enabled auto-merge (squash) June 6, 2025 10:14
@kakkoyun
kakkoyun merged commit bac87e1 into main Jun 6, 2025
192 checks passed
@kakkoyun
kakkoyun deleted the rarguelloF/process-tags-dsm branch June 6, 2025 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kakkoyun kakkoyun kakkoyun approved these changes
@amarziali amarziali amarziali approved these changes
@piochelepiotr piochelepiotr piochelepiotr approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rarguelloF @kakkoyun @amarziali @piochelepiotr