Send APM_TRACING configs as sdk_config when the library advertises SDK_CONFIGURATION - #7628
Send APM_TRACING configs as sdk_config when the library advertises SDK_CONFIGURATION#7628rachelyangdog wants to merge 6 commits into
Conversation
Libraries advertising the SDK_CONFIGURATION remote config capability no longer read the lib_config object of an APM_TRACING config. They read a sdk_config field carrying the same settings keyed by their canonical environment variable name, with values in environment variable form. system-tests only ever emitted lib_config, so every APM_TRACING config was silently ignored by such a library. The RC helpers now read the capability bit off the /v0.7/config requests and translate the payload when it is advertised. lib_config stays the internal representation, so the "empty config keeps the previous value" semantics of the debugger helpers are untouched, and libraries that do not advertise the bit keep receiving exactly what they receive today. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
|
|
Bit 49 is SDK_CONFIGURATION in the remote config source of truth (dd-source remote-config/shared/libs/rc/capabilities.go), but libdatadog gives the same bit to ASM_RAW_RESPONSE_BODY. dd-trace-php therefore advertises it while still reading lib_config, and got sent a payload it does not understand: DEBUGGER_INPRODUCT_ENABLEMENT failed on every php weblog. Require the per-setting APM_TRACING capabilities to be absent as well. Dropping them is the point of the unified bit, and a library still advertising them still understands lib_config, so the two are told apart without relying on a bit whose meaning is currently ambiguous. Also resolve the payload shape before set_and_wait_rc clears the recorded requests. Reading the capabilities waits for the next remote config request, and one recorded between the clear and the update is exactly the stale acknowledgement the clear exists to discard, which wait_for_rc_apply_state then took for the acknowledgement of the update. That returned before the tracer had applied it and failed test_trace_sampling_rate_override_env on every language. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Update: two bugs found in the first CI run, both fixed in 79eb84b1. Capability bit 49 is ambiguous today (this is the interesting one)
// CapabilitySDKConfiguration is the capability for remotely configuring any in-scope
// SDK setting at runtime via a single, env-var-keyed capability bit
CapabilitySDKConfiguration = Capability{
Name: "SDK_CONFIGURATION",
bitIndex: 49,
}But libdatadog gives the same bit to a different capability: DDOG_REMOTE_CONFIG_CAPABILITIES_ASM_RAW_RESPONSE_BODY = 49,dd-trace-php ships that enum, so it advertises bit 49 today while still reading Fix: the switch now also requires the per-setting APM_TRACING capabilities to be gone. Dropping
Important The bit collision itself still needs an owner. libdatadog and dd-source disagree about bit 49, and dd-trace-js#9392 is about to start using it for real. One of the two has to move before 2. Stale-ACK race in the parametric helper
That failed Fix: resolve the payload shape at the top of Not caused by this PR
Testing
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b083d711e
ℹ️ 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".
| if advertises_sdk_configuration and not advertises_legacy: | ||
| return True |
There was a problem hiding this comment.
When a libdatadog-based tracer such as PHP sends an early, partial capability set containing bit 49 before its legacy APM capabilities are registered, this branch classifies that set as the unified SDK contract. Both callers consume the first non-empty poll, and uses_sdk_configuration() then caches the result, so subsequent APM_TRACING updates are sent as sdk_config to a tracer that only reads lib_config. Wait for a capability set that conclusively distinguishes the contracts rather than treating the temporary absence of legacy bits as evidence.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, and fixed in 23eb101 — thanks, this was a real race, not a false positive.
The repo's own helper documents exactly the window you describe: "capabilities are added as products start but never removed. Earlier requests may therefore carry a partial set (e.g. only AppSec capabilities, before APM capabilities are registered)" (utils/docker_fixtures/_test_agent.py). And bit 49 is an AppSec capability in libdatadog (ASM_RAW_RESPONSE_BODY), so during that window PHP shows bit 49 with none of the legacy APM bits — read as "migrated", memoized, and sdk_config sent to a library that only reads lib_config.
The flaw was treating absence as evidence, when absence is also what "not registered yet" looks like. Now the absence only counts once the library has advertised some APM_TRACING capability:
if not capabilities & APM_TRACING_CAPABILITIES:
return None # nothing to conclude yet
if capabilities & LEGACY_APM_TRACING_CAPABILITIES:
return False # per-setting bits -> lib_config
return Capabilities.SDK_CONFIGURATION in capabilitiesAPM_TRACING_CAPABILITIES excludes SDK_CONFIGURATION itself, so the ambiguous bit cannot serve as its own evidence; a unit test pins that. Verified against the real bitmask captured from the failing PHP job: full set -> lib_config, mid-startup (ASM + bit 49) -> inconclusive -> lib_config, nodejs post-migration -> sdk_config.
One small correction: only the parametric caller consumes the most recent non-empty poll. The end-to-end one unions across all requests, so it was less exposed — but exposed all the same.
dd-go#14029 replaced the list of {key, value} entries with a map keyed
by environment variable name, matching jsonconf.SDKConfig:
Config map[string]string `json:"config,omitempty"`
Libraries still read the list for configs stored before that change, so
this was not breaking anything, but the backend no longer produces it.
Verified against dd-trace-js a013e21 (post-migration): the debugger
in-product enablement scenario and the whole parametric dynamic
configuration suite pass, with the tracer applying the settings it
receives as env-var keys.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| _sdk_configuration_support: dict[str, bool] = {} | ||
|
|
||
|
|
||
| def library_supports_sdk_configuration() -> bool: |
There was a problem hiding this comment.
This seems like almost the exact same function as uses_sdk_configuration; can we collapse them into one shared helper in this utils file?
| "dynamic_instrumentation_enabled": "DD_DYNAMIC_INSTRUMENTATION_ENABLED", | ||
| "dynamic_sampling_enabled": None, | ||
| "exception_replay_enabled": "DD_EXCEPTION_REPLAY_ENABLED", | ||
| "live_debugging_enabled": None, |
There was a problem hiding this comment.
What are the settings "that only ever existed as a remote config and has no environment variable counterpart"? Can we just delete them from this list, then?
Codex flagged that treating the absence of the per-setting capabilities as evidence of the unified contract is racy. Capabilities are added as products start, AppSec ones first, and bit 49 is an AppSec capability in libdatadog, so an early poll from a libdatadog library shows bit 49 with no APM_TRACING bit yet. That was classified as the unified contract and memoized, sending sdk_config to a library that only reads lib_config. Absence now only counts once some APM_TRACING capability is advertised; until then the answer is inconclusive and lib_config is sent. Mikayla asked to collapse the two near-identical wrappers. They now share resolve_sdk_configuration_support(), which owns the memo (there were two separate ones), the contract call and the inconclusive policy. The waits and error handling stay per caller, where they differ for a reason: the end-to-end path runs inside setup methods that must never fail, the parametric one inside test bodies. Mikayla also asked whether the settings mapped to None could just be dropped. They can't: they would then report as unknown settings, which is the signal for a genuinely forgotten mapping, and the exhaustiveness test would fail. Named them in the comment instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nfiguration-payload # Conflicts: # utils/_remote_config.py
3 participants
Motivation
dd-trace-js#9392 is the first library to move to the unified Config at Runtime contract, and it fails 7 system-tests jobs:
DEBUGGER_INPRODUCT_ENABLEMENT× 5 weblogs —Test_Debugger_InProduct_Enablement_Dynamic_Instrumentation::test_inproduct_enablement_diPARAMETRIC× 2 — 10 behaviour tests + 7 capability tests intest_dynamic_configuration.pyThat PR swaps the whole APM_TRACING contract:
APM_TRACING_SAMPLE_RATE,_LOGS_INJECTION,_HTTP_HEADER_TAGS,_CUSTOM_TAGS,_ENABLED,_SAMPLE_RULES,_ENABLE_DYNAMIC_INSTRUMENTATION,_ENABLE_CODE_ORIGIN,_ENABLE_LIVE_DEBUGGINGSDK_CONFIGURATION(bit 49)lib_config: {dynamic_instrumentation_enabled: true}sdk_config: {service_name, env, config: [{key: "DD_DYNAMIC_INSTRUMENTATION_ENABLED", value: "true"}]}The
lib_configbranch is removed, not kept as a fallback —RCClientManager.addConfigonly readsconf.sdk_config?.config. system-tests only ever emitslib_config, so every APM_TRACING config becomes a no-op: DI is never enabled, probes never reachEMITTING, and the test fails.What does this PR do?
The RC helpers now read the capability bit the library advertises on
/v0.7/configand, whenSDK_CONFIGURATIONis set, send the same settings in the new shape.service_targetis untouched — it still drives matching and priority.utils/_remote_config.py—to_sdk_config_payload()rewrites an APM_TRACING config fromlib_configtosdk_config: each setting keyed by its canonicalDD_*name, serialized to its environment variable form (booleans, sample rate,tracing_header_tags→"h:tag,…",tracing_tags→"k:v,…",tracing_service_mapping→"a:b,…",tracing_sampling_rules→ JSON with the[{key, value_glob}]tag clauses folded into a map).library_supports_sdk_configuration()reads the bit.use_sdk_config; thesend_*wrappers detect it.lib_configstays the internal representation andprev_payloadskeeps it, so the "empty config keeps the previous value" inheritance the debugger helpers rely on is unchanged.tests/parametric/test_dynamic_configuration.py— translation happens in_set_rc, the single choke point that ownstest_agent.assert_rc_capability()accepts the legacy per-setting bit orSDK_CONFIGURATION, since one bit now covers them all.tests/parametric/capabilities.yml— nodejs's per-setting bits scoped to<7.0.0-0.tests/test_library_conf.py—Test_HeaderTags_DynamicConfigrouted through the same translation (missing_featurefor nodejs today, but it builds its own APM_TRACING payload).tests/test_the_test/test_remote_config.py— 7 unit tests covering the translation, the value serializers, and that the mapping stays exhaustive over what the builders can emit.Nothing changes for any library that does not advertise the bit — today that is every library, including nodejs on
main.Notes for reviewers
capabilities.yml:SDK_CONFIGURATIONis deliberately not listed as expected yet. dd-trace-jsmainis7.0.0-preand does not advertise it until #9392 lands; a pre-release is allowed to report capabilities beyond the expected set, so the<7.0.0-0bound alone covers both sides of the transition without a chicken-and-egg break. A follow-up should add'>=7.0.0-0': [SDK_CONFIGURATION]once #9392 merges. The bound assumes the removal ships in 7.0.0; if a 7.0.0 is cut before #9392 lands, the bound needs moving.dynamic_sampling_enabledandlive_debugging_enabledare remote-config-only settings with no environment variable, so they cannot be expressed assdk_configand are dropped for libraries on the new contract. Both are already ignored by dd-trace-js, so nothing regresses; the mapping records them explicitly asNonerather than guessing a name.provenanceon sampling rules is carried through in theDD_TRACE_SAMPLING_RULESJSON. dd-trace-js'sSamplingRulereads it, so_dd.p.dm-11/-12should still be produced through the env-var path — worth confirming on the #9392 run, as it is a library-side behaviour, not a payload one.Testing
./run.sh TEST_THE_TEST— 445 passed./format.sh— cleantest_inproduct_enablement_dithrough the js reader's allowlist/merge logic: unset →{}, enable →DD_DYNAMIC_INSTRUMENTATION_ENABLED=true, empty → inheritstrue, disable →false.🤖 Generated with Claude Code