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 2 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
|
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