Skip to content

refactor(config): migrate spanAttributeSchemaVersion#4711

Merged
gh-worker-dd-mergequeue-cf854d[bot] merged 4 commits into
mainfrom
config-revamp/span-attribute-schema-version
May 4, 2026
Merged

refactor(config): migrate spanAttributeSchemaVersion#4711
gh-worker-dd-mergequeue-cf854d[bot] merged 4 commits into
mainfrom
config-revamp/span-attribute-schema-version

Conversation

@funyjane

@funyjane funyjane commented May 1, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Adds a public Config.SpanAttributeSchemaVersion() getter on internal/config.Config. The field itself was already declared and initialised by loadConfig; this just exposes a public accessor.
  • Removes the duplicate spanAttributeSchemaVersion field from the legacy tracer config struct and the c.spanAttributeSchemaVersion = int(namingschema.GetVersion()) assignment in newConfig.
  • Rewires the two readers (telemetry.go startup config and tracer.go StartSpan hot path) to read through the new getter.
  • Getter avoids defer because it is read once per local-root span — matches the hot-path pattern from the peer-service migration (refactor(config): peer service revamp #4653) and the README guidance.
  • namingschema.LoadFromEnv() is preserved: contrib packages still consume the namingschema global via GetVersion().

Jira: APMAPI-1887
EPIC: APMAPI-1881 — Go Config Revamp: Migrate tracer

Test plan

  • go build ./...
  • go vet ./ddtrace/tracer/... ./internal/config/...
  • go test ./internal/config/...
  • go test ./ddtrace/tracer/... (full suite)
  • Existing attribute_schema_is_set_v0/v1/wrong_value (tracer_test.go) and defaults-with-schema-v1 (option_test.go) pass without modification.

Move the spanAttributeSchemaVersion field from the legacy tracer config
struct to internal/config.Config. The field is already declared and
initialised by loadConfig; this change exposes a public getter and
rewires the two readers (startTelemetry and the StartSpan hot path) to
use it. The getter avoids defer since it is read once per local-root
span, matching the README's hot-path guidance.

namingschema.LoadFromEnv() is preserved because contrib packages still
consume the namingschema global via GetVersion().

Refs: APMAPI-1887 (EPIC APMAPI-1881)
@funyjane
funyjane requested review from a team as code owners May 1, 2026 13:46
@codecov

codecov Bot commented May 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 16.66667% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.02%. Comparing base (625c602) to head (89c5a54).

Files with missing lines Patch % Lines
internal/config/config.go 0.00% 5 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
ddtrace/tracer/option.go 85.05% <ø> (ø)
ddtrace/tracer/telemetry.go 88.88% <ø> (ø)
ddtrace/tracer/tracer.go 88.42% <100.00%> (ø)
internal/config/config.go 60.18% <0.00%> (+60.18%) ⬆️

... and 444 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.

@datadog-datadog-prod-us1

datadog-datadog-prod-us1 Bot commented May 1, 2026

Copy link
Copy Markdown

Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

🎯 Code Coverage (details)
Patch Coverage: 20.00%
Overall Coverage: 61.38% (+3.87%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 89c5a54 | Docs | Datadog PR Page | Give us feedback!

@pr-commenter

pr-commenter Bot commented May 1, 2026

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2026-05-04 11:50:14

Comparing candidate commit 89c5a54 in PR branch config-revamp/span-attribute-schema-version with baseline commit 625c602 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 209 metrics, 9 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 ----------------------------------'

@mtoffl01 mtoffl01 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.

one nit, otherwise LGTM!

Comment thread ddtrace/tracer/telemetry.go Outdated
{Name: "debug_stack_enabled", Value: c.internalConfig.DebugStack()},
{Name: "profiling_hotspots_enabled", Value: c.internalConfig.ProfilerHotspotsEnabled()},
{Name: "trace_span_attribute_schema", Value: c.spanAttributeSchemaVersion},
{Name: "trace_span_attribute_schema", Value: c.internalConfig.SpanAttributeSchemaVersion()},

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.

let's delete this entirely; SpanAttributeSchemaVersion is now automatically reported to telemetry at startup via the config provider's telemetry implementation.

If existing tests fail as a result , you can probably just modify their expectations

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

thx, fixed it.

funyjane added 2 commits May 1, 2026 20:30
…y entry

The config provider auto-reports DD_TRACE_SPAN_ATTRIBUTE_SCHEMA via its
telemetry implementation when loadConfig calls p.GetString, so the legacy
explicit entry in startTelemetry is duplicate. Update the test expectation
to match the auto-reported default ("") instead of the legacy int (0).
Comment thread internal/config/config.go
}

// SpanAttributeSchemaVersion returns the configured DD_TRACE_SPAN_ATTRIBUTE_SCHEMA version.
// Read on the span-creation hot path; avoids defer to minimise lock cost.

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.

Nice!

@funyjane

funyjane commented May 4, 2026

Copy link
Copy Markdown
Collaborator Author

/merge

@gh-worker-devflow-routing-ef8351

gh-worker-devflow-routing-ef8351 Bot commented May 4, 2026

Copy link
Copy Markdown

View all feedbacks in Devflow UI.

2026-05-04 11:11:16 UTC ℹ️ Start processing command /merge


2026-05-04 11:11:22 UTC ℹ️ MergeQueue: waiting for PR to be ready

This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
It will be added to the queue as soon as checks pass and/or get approvals. View in MergeQueue UI.
Note: if you pushed new commits since the last approval, you may need additional approval.
You can remove it from the waiting list with /remove command.


2026-05-04 11:52:13 UTC ℹ️ MergeQueue: merge request added to the queue

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


2026-05-04 12:06:58 UTC ℹ️ MergeQueue: This merge request was merged

@gh-worker-dd-mergequeue-cf854d
gh-worker-dd-mergequeue-cf854d Bot merged commit ac16240 into main May 4, 2026
222 checks passed
@gh-worker-dd-mergequeue-cf854d
gh-worker-dd-mergequeue-cf854d Bot deleted the config-revamp/span-attribute-schema-version branch May 4, 2026 12:06
mtoffl01 added a commit that referenced this pull request May 7, 2026
### What does this PR do?

- Adds a public `Config.MaxTagsHeaderLen()` getter on
`internal/config.Config`.
- Initializes the field in `loadConfig` from
`DD_TRACE_X_DATADOG_TAGS_MAX_LENGTH`, preserving the existing clamping
semantics (negative → 0; > `MaxPropagatedTagsLength` → clamped down to
that value).
- Adds `DefaultMaxTagsHeaderLen` and `MaxPropagatedTagsLength` exported
constants in `internal/config`.
- Removes the duplicate env resolution from `tracer/option.go` and the
`defaultMaxTagsHeaderLen` / `maxPropagatedTagsLength` constants there.
- The propagator is now constructed using
`c.internalConfig.MaxTagsHeaderLen()` instead of locally re-reading the
env var.

### Motivation

Continued config-migration work: move env resolution of
`DD_TRACE_X_DATADOG_TAGS_MAX_LENGTH` into `internal/config` to match the
pattern established by recent migrations (e.g. #4711, #4653).

---------

Co-authored-by: Claude Opus 4.7 (1M context) <[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