Repository navigation
debugger: run in-product enablement tests with system-probe for Go - #8140
andreimatei wants to merge 3 commits into
Conversation
|
|
🎉 All green!🧪 All tests passed 🔗 Commit SHA: 2f6d6a3 | Docs | View more details | Give us feedback! |
abe91e0 to
8d82c08
Compare
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d82c083ad
ℹ️ 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".
| self.set_probes([probe]) | ||
|
|
||
| self.send_rc_apm_tracing_and_probes(dynamic_instrumentation_enabled=enabled, reset=reset) | ||
| if expect_installed and context.library == "golang": |
There was a problem hiding this comment.
When this scenario runs for Go, these branches add synchronization behavior that every other library skips. The repository explicitly prohibits runtime branching on context.library in test logic and requires language-specific paths to be separate test methods activated through manifests; split the Go flow rather than branching inside the shared setup. .cursor/rules/pr-review.mdcL14-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in 2f6d6a3: the test no longer branches on the library. It waits for the probes to be installed before hitting the endpoint when Dynamic Instrumentation is enabled, and for one agent refresh after disabling it, for every library. Both are needed for Go (system-probe installs the probes after the tracer gets the config, and removes them only after the agent's next refresh) and cost little for the others.
| # while Live Debugger is in use. Until then, it keeps the | ||
| # probes installed, including the ones sent together with the | ||
| # disablement. | ||
| time.sleep(AGENT_REFRESH_INTERVAL + 1) |
There was a problem hiding this comment.
For Go, this setup now sleeps for six seconds after every disablement, while the new probe wait above can block for another 15 seconds. Explicit waits and sleeps are prohibited inside end-to-end setup_* execution because they delay the scenario lifecycle; configure agent_interface_timeout or library_interface_timeout on the scenario and keep setup limited to generating observable activity. .cursor/rules/pr-review.mdcL32-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This setup changes the Remote Config state four times and checks the outcome of each change before making the next one, so it can't defer its waits to a scenario-level timeout; it already waited for the probe statuses after each step before this PR, like the other debugger setups. The wait for the installation returns as soon as the probes are installed. The wait after disabling is a fixed one because no probe status reports a removal; it covers one agent refresh (5 seconds while Live Debugger is in use).
| self.send_rc_symdb() | ||
| # The config is not necessarily acknowledged before the symbols get | ||
| # uploaded (see _wait_for_rc_acknowledgement), so wait for an upload. | ||
| self.wait_for_symbols_upload(timeout=60) |
There was a problem hiding this comment.
Both setup_symdb_upload and setup_event_metadata call this helper, so a missing or delayed upload now blocks each setup for up to 60 seconds before validation begins. End-to-end setup methods must not contain explicit waits; rely on the scenario-level library-interface timeout and validate the captured upload in the corresponding test methods. .cursor/rules/pr-review.mdcL32-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in cd77a71: removed the wait from the setup. DEBUGGER_SYMDB now sets library_interface_timeout to 30 seconds instead (DebuggerScenario takes it as a parameter now), which gives the library time to upload symbols once, after all the setups.
Before this patch, every debugger RC update waited for the library to acknowledge the configs, for up to 30 seconds. The Go tracer does not apply Dynamic Instrumentation configs, the agent's system-probe does, so it reports them with apply state UNKNOWN and never acknowledges them: every update stalled for 30 seconds before the wait gave up, which only logs a warning. For Go, wait only for the tracer to receive the update; probe statuses report on the application of the configs. The symdb test relied on the stall to give the library time to upload symbols; DEBUGGER_SYMDB now waits longer for the library interface at the end of the setups instead.
Make the mocked RC backend issue a new director targets version on every response to the agent, like the production backend, which uses a time-based version. Before this patch, the mocked backend served the same pre-built response, with the director targets at the version of the RC state, until the test sent a new state. The agent only sends files to a client whose targets version differs from its own, so a client that started subscribing to a product (e.g. a tracer subscribing to LIVE_DEBUGGING after Dynamic Instrumentation is enabled remotely) never received the configs of that product already in the state. In production, the client receives them after the agent's next refresh. send_state now builds a sequence of responses for the state, re-signing the director metadata with versions state version * 1000 + n, and the proxy serves the next one on each agent request, sticking to the last one. The responses are built on the test runner side because the proxy image cannot sign TUF metadata. In backend mode, send_state maps the reported targets version back to the state version. Only scenarios with rc_backend_enabled are affected.
Make DEBUGGER_INPRODUCT_ENABLEMENT a DebuggerScenario and enable Test_Debugger_InProduct_Enablement_Dynamic_Instrumentation for Go starting with v2.6.0, the first dd-trace-go release that supports enabling Dynamic Instrumentation through Remote Config. Before this patch, the scenario was a plain DdTraceEndToEndScenario. Go Dynamic Instrumentation is implemented in the agent's system-probe, which only DebuggerScenario sets up, so Go probes could never be installed. The scenario also mocked Remote Config at the tracer-facing endpoint instead of acting as the backend for the agent, so system-probe would not have received the probes anyway. DebuggerScenario uses the backend mode, like the other debugger scenarios. Since configs now reach the tracer through the agent, which polls the backend every 5 seconds, the per-step timeout of the in-product enablement tests goes from 5 to 15 seconds. The in-product Dynamic Instrumentation test now waits for the probes to be installed before hitting the probed endpoint when Dynamic Instrumentation is enabled: for Go, system-probe installs them after the tracer receives the config. After disabling Dynamic Instrumentation, it waits for one agent refresh before hitting the endpoint: for Go, system-probe removes the probes, including the ones sent together with the disablement, only after the agent's next refresh.
8d82c08 to
2f6d6a3
Compare
Motivation
Test_Debugger_InProduct_Enablement_Dynamic_Instrumentation was marked
missing_feature for Go, so nothing tested enabling Dynamic Instrumentation
through Remote Config for Go. dd-trace-go supports it since v2.6.0. Making
the test run for Go required three changes, one per commit.
Changes
debugger: don't wait for Go to acknowledge debugger configs
Every debugger RC update waited for the library to acknowledge the configs,
for up to 30 seconds. The Go tracer does not apply Dynamic Instrumentation
configs, the agent's system-probe does, so it reports them with apply state
UNKNOWN and never acknowledges them: every update stalled for 30 seconds
before the wait gave up, which only logs a warning. For Go, wait only for
the tracer to receive the update; probe statuses report on the application
of the configs. The symdb test relied on the stall to give the library time
to upload symbols; DEBUGGER_SYMDB now waits longer for the library
interface at the end of the setups instead. Locally, Go
DEBUGGER_PROBES_SNAPSHOT goes from 327s to 81s.
remote-config: give each mocked backend response a new director version
The mocked RC backend served the same response, with the director targets
at the version of the RC state, until the test sent a new state. The agent
only sends files to a client whose targets version differs from its own, so
a client that starts subscribing to a product (e.g. a tracer subscribing to
LIVE_DEBUGGING after Dynamic Instrumentation is enabled remotely) never
received the configs of that product already in the state. The production
backend gives the director targets a new, time-based version on every
response, so the client receives them after the agent's next refresh.
send_state now builds a sequence of responses for the state, re-signing the
director metadata with versions state version * 1000 + n, and the proxy
serves the next one on each agent request, sticking to the last one. The
responses are built on the test runner side because the proxy image cannot
sign TUF metadata. In backend mode, send_state maps the reported targets
version back to the state version. Only scenarios with rc_backend_enabled
(the debugger ones and TRACING_CONFIG_NONDEFAULT_4) are affected.
debugger: run in-product enablement tests with system-probe for Go
Make DEBUGGER_INPRODUCT_ENABLEMENT a DebuggerScenario and enable
Test_Debugger_InProduct_Enablement_Dynamic_Instrumentation for Go starting
with v2.6.0. The scenario was a plain DdTraceEndToEndScenario: Go Dynamic
Instrumentation is implemented in the agent's system-probe, which only
DebuggerScenario sets up, and the scenario mocked Remote Config at the
tracer-facing endpoint, so system-probe would not have received the probes
anyway. The per-step timeout goes from 5 to 15 seconds since configs now
reach the tracer through the agent, which refreshes every 5 seconds.
The test now waits for the probes to be installed before hitting the probed
endpoint when Dynamic Instrumentation is enabled, and waits for one agent
refresh after disabling it: for Go, system-probe installs the probes after
the tracer receives the config, and removes them, including the ones sent
together with the disablement, only after the agent's next refresh.
Testing
Locally: for Go (net-http), DEBUGGER_INPRODUCT_ENABLEMENT,
DEBUGGER_PROBES_SNAPSHOT and DEBUGGER_SYMDB pass; DEBUGGER_INPRODUCT_ENABLEMENT
also passes for Node.js (express5) and .NET (default weblog). Other languages
and weblogs rely on CI.
Workflow
system-tests-reviewers: ask to one of youre co-worker familiar with the tested feature.system-tests-core(slack)🚀 Once your PR is reviewed and the CI green, you can merge it!
🛟 #apm-shared-testing 🛟