fix(e2e/discovery): Capture fixture service diagnostics on failure - #54205
Conversation
dumpDebugInfo queried /discovery/debug, which was removed in fd2b24d ("discovery: remove old discovery check", #39417). Every failure since has logged system-probe-lite's 404 body ("Not found") or the Go module's ("404 page not found") instead of any discovery state, so the most useful diagnostic in the suite has been dead for months. Replace it with endpoints which still exist, and add the information needed to tell "discovery did not report the service" apart from "the fixture service was not running", which the output previously could not distinguish: - GET /discovery/state for the active implementation. - POST /discovery/services for the fixture services' PIDs. This is the only endpoint reporting what discovery actually sees; it is caller-driven, so it has to be told which PIDs to inspect. - ps ELAPSED per fixture PID, since discovery ignores processes younger than discovery.service_collection_min_process_age (1 minute by default). - systemctl status and journalctl per fixture service. The journal is the only place a fixture's own output ends up, so a service which exits at startup, or crash-loops via Restart=always/RestartSec=1, previously left no trace at all in the test output. - ss -ltnp, since discovery only reports a process once it has a listening socket. PIDs come from each unit's cgroup rather than its main PID, because the process the tests match on is not always the main one: node-json-server runs through npm, which spawns the node process that serves the port. DSCVR-621
Files inventory check summaryFile checks results against ancestor 8699dd37: Results for datadog-agent_7.83.0~devel.git.444.bc82557.pipeline.128209768-1_amd64.deb:No change detected |
The diagnostics run only after a test has already failed, and several of the commands exit non-zero in exactly the situations worth debugging. Verified on Ubuntu 22.04 with systemd 249, matching the image the tests run on: ps -o pid,ppid,etimes,comm,args -p <exited pid> -> exit 1 curl -s --unix-socket <missing socket> ... -> exit 7 A fixture PID read from the unit's cgroup can exit before ps runs, which is precisely what a crash-looping service does, and the agent may be down when the dump is taken. In both cases MustExecute would have failed the test with an error about the diagnostic instead of reporting the original failure. Route every diagnostic through a helper which keeps the output and discards the exit status, so this cannot be reintroduced one command at a time. DSCVR-621
Verified against a system-probe-lite built from this tree: the endpoint is
POST-only, rejects a request with no Content-Type, and answers a correctly
formed request with service data.
It also ignores unknown JSON fields without an error, so a body with a
misspelled field is indistinguishable from one that matched no services --
both return {"services":[],...}. Record that, since the body has to be built
by hand here: core.Params lives in the main module, which this module does not
depend on, so nothing makes the two drift visibly.
DSCVR-621
Execute() returns no output once a command fails: it blanks stdout and folds it into the error (client/host.go, "return \"\", fmt.Errorf(\"%v: %w\", stdout, err)"), so handling the error here instead would mean digging the output being collected back out of an error string. Letting the remote shell absorb the status keeps it on the normal return path. Also note that the redirect is redundant with the framework running commands through session.CombinedOutput, so nobody removes it expecting stderr to disappear, or keeps it believing stderr depends on it. DSCVR-621
There was a problem hiding this comment.
More details
The new failure diagnostics preserve useful evidence across the realistic failure cases checked: multiple fixture PIDs produce valid discovery requests, exited PIDs still leave ps output, and failing commands do not abort diagnostic collection. No additional tests recommended: the change is isolated to failure-path logging and the exercised input classes are already represented by the existing fixture setup.
📊 Validated against 3 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit bc82557 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
…54205) ### What does this PR do? Makes `dumpDebugInfo` in the discovery E2E suite produce usable output again. It queried `/discovery/debug`, which was removed in fd2b24d ("discovery: remove old discovery check", #39417). Every failure since has logged a 404 body instead of any discovery state — `Not found` in `system-probe-lite` mode (system-probe-lite's `NOTFOUND` constant) and `404 page not found` in `system-probe` mode (Go's `ServeMux` default). Both strings are visible in every failure of the two jobs behind DSCVR-621. Replaced with endpoints that still exist, plus the information needed to tell *"discovery did not report the service"* apart from *"the fixture service was not running"* — a distinction the previous output could not make at all: - `GET /discovery/state` — which implementation is live. - `POST /discovery/services` for the fixture services' PIDs. This is the only endpoint that reports what discovery actually sees. It is caller-driven, so it must be told which PIDs to inspect, and it is POST-only with a JSON body in `system-probe-lite`. - `ps` `ELAPSED` per fixture PID. Discovery never asks about a process younger than `discovery.service_collection_min_process_age` (1 minute by default), so a value below that explains a missing service on its own, and one that keeps resetting across dumps means the service is crash-looping rather than booting slowly. - `systemctl status` and `journalctl` per fixture service. The journal is the only place a fixture's own output ends up. - `ss -ltnp`. Discovery only reports a process once it has a listening socket. PIDs come from each unit's cgroup rather than its main PID, because the process the tests match on is not always the main one: `node-json-server` runs through npm, which spawns the `node` process that serves the port. ### Motivation DSCVR-621. Investigating that flake, the suite's own diagnostics contributed nothing: `/discovery/debug` returned a 404 body in all four failing subtests of both jobs, and nothing recorded the fixture services' state, so it was impossible to tell from CI output whether discovery had failed to report a running service or the service had never started. Determining that required reconstructing process lifetimes from `workload-list` dumps. In the two failures behind the ticket the fixture service was in fact not running — `ruby3.0` appears in only one of four `workload-list` dumps of job 1874204604, created 9s before the dump was taken. `journalctl` would have said why in one line. ### Describe how you validated your changes Since a green CI run does not exercise any of this — `dumpDebugInfo` only runs after a failure, and I confirmed the passing `new-e2e-discovery` job contains none of the new log markers — the endpoints and commands were verified directly. **Endpoints**, against a `system-probe-lite` built from this tree and driven with the exact command strings this PR sends: | Request | Response | |---|---| | `GET /discovery/state` | `{"implementation":"system-probe-lite"}` | | `POST /discovery/services`, `{"new_pids":[…]}` + `Content-Type` | `{"services":[{"pid":…,"generated_name":…}],…}` | | `GET /discovery/debug` (the call being removed) | `Not found` — the exact string seen in every DSCVR-621 failure | | `POST /discovery/services` with no `Content-Type` | `Bad request` | | `POST` with a misspelled field | `{"services":[],…}` — unknown fields ignored silently | | malformed JSON | `Bad request` | The misspelled-field row is why the field name is called out in a comment: an empty result is indistinguishable from "matched no services", and `core.Params` cannot be imported here because it belongs to the main module, which `test/new-e2e` does not depend on. **Commands and their exit codes**, on Ubuntu 22.04 / systemd 249 / cgroup v2, matching the image the tests run on: `ps` for an exited PID exits 1 and `curl` to a missing socket exits 7, so both are routed through a helper that keeps the output and drops the status — otherwise a crash-looping fixture (a PID that dies between the cgroup read and `ps`) would have replaced the real failure with an error about the diagnostic. Also confirmed there that `ps`'s header is `ELAPSED`, comma-separated PID lists work, a missing `cgroup.procs` is handled, and `ss -ltn` prints `0.0.0.0:<port>` followed by padding. Additionally: - `dda inv linter.go --targets=./test/new-e2e/tests/discovery` — clean. - `new-e2e-discovery` passes on this branch. - `ss` is already used against these images elsewhere in the E2E suite (`test/new-e2e/tests/agent-metric-pipelines/common/adp.go`, `test/new-e2e/tests/agent-platform/common/bound-port/unix.go`). - No assertions are added or changed — this only affects what is logged when a test has already failed. Not verified: the `system.slice/<svc>.service/cgroup.procs` path against the actual fixture units. The cgroup v2 layout was checked on a matching host, but not with these unit names. ### Additional Notes Diagnostics only, deliberately: this does not attempt to fix the flake. #54206 builds on this one and addresses the cause. Co-authored-by: vincent.whitchurch <vincent.whitchurch@datadoghq.com> c73bbfb
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: 2271844 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | quality_gate_metrics_logs | memory utilization | +0.65 | [+0.40, +0.90] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_logs | % cpu utilization | +0.62 | [-0.37, +1.61] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_mean_fs_load | memory utilization | +0.19 | [+0.15, +0.23] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle_all_features | memory utilization | +0.03 | [-0.01, +0.07] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_idle | memory utilization | +0.02 | [-0.04, +0.08] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle | memory utilization | -0.09 | [-0.14, -0.04] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_no_fs_load | memory utilization | -0.13 | [-0.22, -0.04] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_private_action_runner | memory utilization | -0.33 | [-0.44, -0.21] | 1 | Logs bounds checks dashboard |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | observed_value | links |
|---|---|---|---|---|---|
| ✅ | quality_gate_idle | intake_connections | 10/10 | 3 ≤ 4 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | 148.54MiB ≤ 154MiB | bounds checks dashboard |
| ✅ | quality_gate_idle | total_bytes_received | 10/10 | 730.57KiB ≤ 819.20KiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | intake_connections | 10/10 | 3 ≤ 4 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | 492.95MiB ≤ 512MiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | total_bytes_received | 10/10 | 1.12MiB ≤ 1.25MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | 4 ≤ 6 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | 185.43MiB ≤ 195MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_logs | total_bytes_received | 10/10 | 264.17MiB ≤ 292MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 373.73 ≤ 2000 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | 3 ≤ 6 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | 420.47MiB ≤ 430MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | total_bytes_received | 10/10 | 0.94GiB ≤ 1.04GiB | bounds checks dashboard |
| ✅ | quality_gate_private_action_runner | memory_usage | 10/10 | 71.98MiB ≤ 75MiB | bounds checks dashboard |
| ✅ | quality_gate_security_idle | cpu_usage | 10/10 | 29.88 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_idle | memory_usage | 10/10 | 300.02MiB ≤ 330MiB | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | cpu_usage | 10/10 | 63.39 ≤ 200 | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | memory_usage | 10/10 | 279.46MiB ≤ 310MiB | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | cpu_usage | 10/10 | 23.28 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | memory_usage | 10/10 | 282.95MiB ≤ 320MiB | bounds checks dashboard |
Explanation
Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%
Performance changes are noted in the perf column of each table:
- ✅ = significantly better comparison variant performance
- ❌ = significantly worse comparison variant performance
- ➖ = no significant change in performance
A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".
For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:
-
Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
-
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
-
Its configuration does not mark it "erratic".
Replicate Execution Details
We run multiple replicates for each experiment/variant. However, we allow replicates to be automatically retried if there are any failures, up to 8 times, at which point the replicate is marked dead and we are unable to run analysis for the entire experiment. We call each of these attempts at running replicates a replicate execution. This section lists all replicate executions that failed due to the target crashing or being oom killed.
Note: In the below tables we bucket failures by experiment, variant, and failure type. For each of these buckets we list out the replicate indexes that failed with an annotation signifying how many times said replicate failed with the given failure mode. In the below example the baseline variant of the experiment named experiment_with_failures had two replicates that failed by oom kills. Replicate 0, which failed 8 executions, and replicate 1 which failed 6 executions, all with the same failure mode.
| Experiment | Variant | Replicates | Failure | Logs | Debug Dashboard |
|---|---|---|---|---|---|
| experiment_with_failures | baseline | 0 (x8) 1 (x6) | Oom killed | Debug Dashboard |
The debug dashboard links will take you to a debugging dashboard specifically designed to investigate replicate execution failures.
❌ Retried Profiling Replicate Execution Failures (ddprof)
Note: Profiling replicas may still be executing. See the debug dashboard for up to date status.
| Experiment | Variant | Replicates | Failure | Debug Dashboard |
|---|---|---|---|---|
| quality_gate_idle | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_idle | comparison | 10 | Oom killed | Debug Dashboard |
| quality_gate_idle_all_features | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_idle_all_features | comparison | 10 | Oom killed | Debug Dashboard |
| quality_gate_metrics_logs | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_metrics_logs | comparison | 10 | Oom killed | Debug Dashboard |
| quality_gate_security_idle | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_security_no_fs_load | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_security_no_fs_load | comparison | 10 | Crashed (exit code: 134) | Debug Dashboard |
CI Pass/Fail Decision
✅ Passed. All Quality Gates passed.
- quality_gate_security_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_security_no_fs_load, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_no_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_private_action_runner, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
What does this PR do?
Makes
dumpDebugInfoin the discovery E2E suite produce usable output again.It queried
/discovery/debug, which was removed in fd2b24d ("discovery: remove old discovery check", #39417). Every failure since has logged a 404 body instead of any discovery state —Not foundinsystem-probe-litemode (system-probe-lite'sNOTFOUNDconstant) and404 page not foundinsystem-probemode (Go'sServeMuxdefault). Both strings are visible in every failure of the two jobs behind DSCVR-621.Replaced with endpoints that still exist, plus the information needed to tell "discovery did not report the service" apart from "the fixture service was not running" — a distinction the previous output could not make at all:
GET /discovery/state— which implementation is live.POST /discovery/servicesfor the fixture services' PIDs. This is the only endpoint that reports what discovery actually sees. It is caller-driven, so it must be told which PIDs to inspect, and it is POST-only with a JSON body insystem-probe-lite.psELAPSEDper fixture PID. Discovery never asks about a process younger thandiscovery.service_collection_min_process_age(1 minute by default), so a value below that explains a missing service on its own, and one that keeps resetting across dumps means the service is crash-looping rather than booting slowly.systemctl statusandjournalctlper fixture service. The journal is the only place a fixture's own output ends up.ss -ltnp. Discovery only reports a process once it has a listening socket.PIDs come from each unit's cgroup rather than its main PID, because the process the tests match on is not always the main one:
node-json-serverruns through npm, which spawns thenodeprocess that serves the port.Motivation
DSCVR-621. Investigating that flake, the suite's own diagnostics contributed nothing:
/discovery/debugreturned a 404 body in all four failing subtests of both jobs, and nothing recorded the fixture services' state, so it was impossible to tell from CI output whether discovery had failed to report a running service or the service had never started. Determining that required reconstructing process lifetimes fromworkload-listdumps.In the two failures behind the ticket the fixture service was in fact not running —
ruby3.0appears in only one of fourworkload-listdumps of job 1874204604, created 9s before the dump was taken.journalctlwould have said why in one line.Describe how you validated your changes
Since a green CI run does not exercise any of this —
dumpDebugInfoonly runs after a failure, and I confirmed the passingnew-e2e-discoveryjob contains none of the new log markers — the endpoints and commands were verified directly.Endpoints, against a
system-probe-litebuilt from this tree and driven with the exact command strings this PR sends:GET /discovery/state{"implementation":"system-probe-lite"}POST /discovery/services,{"new_pids":[…]}+Content-Type{"services":[{"pid":…,"generated_name":…}],…}GET /discovery/debug(the call being removed)Not found— the exact string seen in every DSCVR-621 failurePOST /discovery/serviceswith noContent-TypeBad requestPOSTwith a misspelled field{"services":[],…}— unknown fields ignored silentlyBad requestThe misspelled-field row is why the field name is called out in a comment: an empty result is indistinguishable from "matched no services", and
core.Paramscannot be imported here because it belongs to the main module, whichtest/new-e2edoes not depend on.Commands and their exit codes, on Ubuntu 22.04 / systemd 249 / cgroup v2, matching the image the tests run on:
psfor an exited PID exits 1 andcurlto a missing socket exits 7, so both are routed through a helper that keeps the output and drops the status — otherwise a crash-looping fixture (a PID that dies between the cgroup read andps) would have replaced the real failure with an error about the diagnostic. Also confirmed there thatps's header isELAPSED, comma-separated PID lists work, a missingcgroup.procsis handled, andss -ltnprints0.0.0.0:<port>followed by padding.Additionally:
dda inv linter.go --targets=./test/new-e2e/tests/discovery— clean.new-e2e-discoverypasses on this branch.ssis already used against these images elsewhere in the E2E suite (test/new-e2e/tests/agent-metric-pipelines/common/adp.go,test/new-e2e/tests/agent-platform/common/bound-port/unix.go).Not verified: the
system.slice/<svc>.service/cgroup.procspath against the actual fixture units. The cgroup v2 layout was checked on a matching host, but not with these unit names.Additional Notes
Diagnostics only, deliberately: this does not attempt to fix the flake. #54206 builds on this one and addresses the cause.