Skip to content

Performance investigation#1905

Closed
pawelchcki wants to merge 15 commits into
mainfrom
pawel/test_env_mutex_theory
Closed

Performance investigation#1905
pawelchcki wants to merge 15 commits into
mainfrom
pawel/test_env_mutex_theory

Conversation

@pawelchcki

Copy link
Copy Markdown

What does this PR do?

Motivation

Describe how to test/QA your changes

Reviewer's Checklist

  • Changed code has unit tests for its functionality.
  • If this interacts with the agent in a new way, a system test has been added.

@pr-commenter

pr-commenter Bot commented Apr 17, 2023

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2023-11-23 16:08:26

Comparing candidate commit f6a1336 in PR branch pawel/test_env_mutex_theory with baseline commit 8beb30d in branch main.

Found 13 performance improvements and 4 performance regressions! Performance is the same for 22 metrics, 2 unstable metrics.

scenario:BenchmarkExtractW3C-24

  • 🟩 allocations [-1; -1] or [-5.556%; -5.556%]
  • 🟩 execution_time [-196.531ns; -166.269ns] or [-7.114%; -6.019%]

scenario:BenchmarkOTelApiWithCustomTags/datadog_otel_api-24

  • 🟩 allocated_mem [-490 bytes; -480 bytes] or [-13.547%; -13.262%]
  • 🟩 allocations [-6; -6] or [-20.000%; -20.000%]
  • 🟩 execution_time [-992.465ns; -938.135ns] or [-19.765%; -18.683%]

scenario:BenchmarkOTelApiWithCustomTags/otel_api-24

  • 🟩 allocated_mem [-494 bytes; -481 bytes] or [-10.215%; -9.940%]
  • 🟩 allocations [-6; -6] or [-13.636%; -13.636%]
  • 🟩 execution_time [-919.887ns; -841.313ns] or [-12.605%; -11.528%]

scenario:BenchmarkPartialFlushing/Disabled-24

  • 🟥 allocations [+194738; +197229] or [+6.134%; +6.212%]
  • 🟥 execution_time [+20.510ms; +25.766ms] or [+7.285%; +9.152%]

scenario:BenchmarkPartialFlushing/Enabled-24

  • 🟥 allocations [+194044; +195335] or [+6.095%; +6.136%]
  • 🟥 execution_time [+25.491ms; +31.837ms] or [+9.120%; +11.391%]

scenario:BenchmarkSingleSpanRetention/no-rules-24

  • 🟩 execution_time [-11.854µs; -9.881µs] or [-4.719%; -3.934%]

scenario:BenchmarkSingleSpanRetention/with-rules/match-all-24

  • 🟩 execution_time [-12.014µs; -9.903µs] or [-4.742%; -3.908%]

scenario:BenchmarkSingleSpanRetention/with-rules/match-half-24

  • 🟩 execution_time [-12.234µs; -9.833µs] or [-4.833%; -3.884%]

scenario:BenchmarkTracerAddSpans-24

  • 🟩 allocations [-2; -2] or [-8.000%; -8.000%]
  • 🟩 execution_time [-234.801ns; -209.599ns] or [-5.834%; -5.207%]

Comment thread ddtrace/tracer/tracer.go
Comment thread internal/log/log.go
var (
mu sync.RWMutex // guards below fields
level = LevelWarn
level atomic.Int32 = atomic.Int32{}

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.

🚫 [golangci] reported by reviewdog 🐶
var-declaration: should omit type atomic.Int32 from declaration of var level; it will be inferred from the right-hand side (revive)

}

// TODO - this is just a temporary hack to avoid accessing mutex locked resource in a hotpath
var TraceID128BitEnabled atomic.Bool = atomic.Bool{}

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.

🚫 [golangci] reported by reviewdog 🐶
var-declaration: should omit type atomic.Bool from declaration of var TraceID128BitEnabled; it will be inferred from the right-hand side (revive)

@darccio darccio closed this Feb 3, 2026
gh-worker-dd-mergequeue-cf854d Bot pushed a commit that referenced this pull request Mar 17, 2026
### What does this PR do?

Turns `internal/log` to use `atomic.Int32` to avoid using `log.mu` locking when checking the level.

### Motivation

Complete #1905 work using the current benchmarking platform.

### Reviewer's Checklist

- [x] Changed code has unit tests for its functionality at or near 100% coverage.
- [x] There is a benchmark for any new code, or changes to existing code.
- [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.

Unsure? Have a question? Request a review!

Co-authored-by: kemal.akkoyun <[email protected]>
gh-worker-dd-mergequeue-cf854d Bot pushed a commit that referenced this pull request Mar 24, 2026
### What does this PR do?

Turns `trace.priority` into `atomic.Pointer` to avoid using `trace.mu` locking.

### Motivation

Complete #1905 work using the current benchmarking platform.

### Reviewer's Checklist

- [x] Changed code has unit tests for its functionality at or near 100% coverage.
- [x] There is a benchmark for any new code, or changes to existing code.
- [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.

Unsure? Have a question? Request a review!


Co-authored-by: dario.castane <[email protected]>
gh-worker-dd-mergequeue-cf854d Bot pushed a commit that referenced this pull request Mar 25, 2026
…ED` once, use a global `bool` (#4548)

### What does this PR do?

Stop checking environment variable `DD_TRACE_128_BIT_TRACEID_GENERATION_ENABLED` on span start. Checks it once, use a global `bool`.

It'd be better to load it on `newConfig` and make the value available, but this one happens in `newSpanContext`, which modification will cause a lot of changes in multiple files, including tests.

### Motivation

Complete #1905 work using the current benchmarking platform.

### Reviewer's Checklist

- [x] Changed code has unit tests for its functionality at or near 100% coverage.
- [x] There is a benchmark for any new code, or changes to existing code.
- [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.

Unsure? Have a question? Request a review!


Co-authored-by: kakkoyun <[email protected]>
Co-authored-by: kemal.akkoyun <[email protected]>
mtoffl01 pushed a commit that referenced this pull request Mar 25, 2026
…ED` once, use a global `bool` (#4548)

Stop checking environment variable `DD_TRACE_128_BIT_TRACEID_GENERATION_ENABLED` on span start. Checks it once, use a global `bool`.

It'd be better to load it on `newConfig` and make the value available, but this one happens in `newSpanContext`, which modification will cause a lot of changes in multiple files, including tests.

Complete #1905 work using the current benchmarking platform.

- [x] Changed code has unit tests for its functionality at or near 100% coverage.
- [x] There is a benchmark for any new code, or changes to existing code.
- [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.

Unsure? Have a question? Request a review!

Co-authored-by: kakkoyun <[email protected]>
Co-authored-by: kemal.akkoyun <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants