Skip to content

fix(ddtrace/tracer): check DD_TRACE_128_BIT_TRACEID_GENERATION_ENABLED once, use a global bool#4548

Merged
gh-worker-dd-mergequeue-cf854d[bot] merged 5 commits into
mainfrom
dario.castane/remove-trace-128-bit-envvar-check
Mar 25, 2026
Merged

fix(ddtrace/tracer): check DD_TRACE_128_BIT_TRACEID_GENERATION_ENABLED once, use a global bool#4548
gh-worker-dd-mergequeue-cf854d[bot] merged 5 commits into
mainfrom
dario.castane/remove-trace-128-bit-envvar-check

Conversation

@darccio

@darccio darccio commented Mar 16, 2026

Copy link
Copy Markdown
Member

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

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • There is a benchmark for any new code, or changes to existing code.
  • New code is free of linting errors. You can check this by running make lint locally.
  • New code doesn't break existing tests. You can check this by running make test locally.
  • 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!

@darccio
darccio requested review from a team as code owners March 16, 2026 17:11
@darccio
darccio force-pushed the dario.castane/remove-trace-128-bit-envvar-check branch from 5693fd9 to c0b5220 Compare March 16, 2026 17:12
@datadog-prod-us1-4

datadog-prod-us1-4 Bot commented Mar 16, 2026

Copy link
Copy Markdown

✅ Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

🎯 Code Coverage (details)
Patch Coverage: 57.14%
Overall Coverage: 59.84% (+0.04%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 4bb0128 | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback!

@codecov

codecov Bot commented Mar 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 55.55556% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.55%. Comparing base (4e1ef08) to head (4bb0128).

Files with missing lines Patch % Lines
internal/config/config.go 20.00% 4 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
ddtrace/tracer/option.go 83.92% <100.00%> (-2.89%) ⬇️
ddtrace/tracer/spancontext.go 88.19% <100.00%> (-1.74%) ⬇️
internal/config/config.go 57.62% <20.00%> (+1.40%) ⬆️

... and 263 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pr-commenter

pr-commenter Bot commented Mar 16, 2026

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2026-03-25 09:58:14

Comparing candidate commit 4bb0128 in PR branch dario.castane/remove-trace-128-bit-envvar-check with baseline commit 4e1ef08 in branch main.

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

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

  • 🟩 = significantly better candidate vs. baseline
  • 🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

scenario:BenchmarkOTelApiWithCustomTags/datadog_otel_api-25

  • 🟩 execution_time [-119.843ns; -84.757ns] or [-2.999%; -2.121%]

scenario:BenchmarkStartSpanConfig/scenario_WithStartSpanConfig-25

  • 🟩 execution_time [-72.947ns; -59.053ns] or [-3.106%; -2.514%]

scenario:BenchmarkStartSpanConfig/scenario_none-25

  • 🟩 execution_time [-74.829ns; -62.771ns] or [-2.997%; -2.514%]

@kakkoyun kakkoyun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

Comment thread ddtrace/tracer/spancontext.go Outdated

// traceID128BitEnabled caches DD_TRACE_128_BIT_TRACEID_GENERATION_ENABLED
// at init time so that newSpanContext avoids calling BoolEnv on every span.
var traceID128BitEnabled = sharedinternal.BoolEnv("DD_TRACE_128_BIT_TRACEID_GENERATION_ENABLED", true)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is okay for now, however, this will be problematic when we try to parallelize the test runs (which is our goal).

We should avoid adding global state like this if it is possibel.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I know, but checking an environment variable is the same 😁 We need to find a better way to have this kind of configs accessible from any code path in the span lifecycle.

@darccio
darccio requested a review from kakkoyun March 19, 2026 06:56
Comment thread ddtrace/tracer/spancontext.go Outdated

// traceID128BitEnabled caches DD_TRACE_128_BIT_TRACEID_GENERATION_ENABLED
// at init time so that newSpanContext avoids calling BoolEnv on every span.
var traceID128BitEnabled = sharedinternal.BoolEnv("DD_TRACE_128_BIT_TRACEID_GENERATION_ENABLED", true)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If a customer changes this at runtime (os.Setenv("DD_TRACE_128_BIT_TRACEID_GENERATION_ENABLED", "false")) and then restart its tracer the value won't get updated right ?

I don't think that's ideal, as we are doing this kind of env var modification customer must be doing it as well.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@genesor I can move the initialization to the newConfig method. WDYT? This guarantees that restarting the tracer gets the updated value.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, that would be a great middle ground.

I also agree with you that we need a way to get configs from anywhere in the codebase, linked or not with a tracer instance.

cc @mtoffl01

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done in 94d83a3. The next commit is to update tests to manipulate the global state in the right moment.

@darccio
darccio requested a review from a team as a code owner March 24, 2026 17:11
@darccio
darccio force-pushed the dario.castane/remove-trace-128-bit-envvar-check branch from 979fd6a to 58561c1 Compare March 24, 2026 17:18
@darccio
darccio requested a review from genesor March 24, 2026 17:28
darccio and others added 2 commits March 24, 2026 17:33
…t in tests

newConfig now stores traceID128BitEnabled from the env var on every tracer
start, so Swap calls before startTestTracer/newTracer were immediately
overwritten by the default (true). Move Swap to after tracer creation so the
atomic is set correctly before spans are started.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]>
@darccio

darccio commented Mar 24, 2026

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Swish!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@genesor genesor left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@kakkoyun kakkoyun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants