[Debugger] Add system-test coverage for the debugger guardrails - #7287
[Debugger] Add system-test coverage for the debugger guardrails#7287dudikeleti wants to merge 24 commits into
Conversation
|
|
|
Bits couldn’t fix the CI🟢 Investigated · 🔴 Fix failed · ⚪ Validate · ⚪ Ready The newly enabled View in Datadog | Reviewed commit 2c71ee0 |
dudikeleti
changed the title
3babba7 to
f2c565e
Compare
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Adds focused end-to-end system-test coverage for debugger “guardrails” (evaluation timeout, snapshot size cap, and capture-timeout reporting) in the system-tests framework, wiring up new fixtures/endpoints across multiple weblogs and gating execution via orchestrator support checks and per-language manifests.
Changes:
- Introduces new debugger guardrail tests (evaluation-timeout, snapshot-size cap, capture-timeout reason) plus new probe fixtures and a dedicated
DEBUGGER_CAPTURE_TIMEOUTscenario. - Extends Node.js/Java/.NET test apps with a
/debugger/snapshot/capture-timeoutendpoint and updates Dockerfiles to include the debugger route modules. - Updates CI orchestration and “test-the-test” coverage (scenario mapping, workflow step, and
_is_supportedgating) and marks tests appropriately in language manifests.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/scripts/ci_orchestrators/workflow_data.py | Restricts DEBUGGER_CAPTURE_TIMEOUT scenario to weblogs that include the new fixture. |
| utils/build/docker/nodejs/uds-express4.Dockerfile | Copies the Express debugger routes into the UDS Express4 image. |
| utils/build/docker/nodejs/fastify/debugger/index.js | Adds capture-timeout fixture route and local variable construction for snapshot capture. |
| utils/build/docker/nodejs/fastify.Dockerfile | Copies fastify debugger routes into the image. |
| utils/build/docker/nodejs/express5.Dockerfile | Copies Express debugger routes into the Express5 image. |
| utils/build/docker/nodejs/express4.Dockerfile | Copies Express debugger routes into the Express4 image. |
| utils/build/docker/nodejs/express4-typescript/debugger/index.ts | Adds capture-timeout fixture route for the TS Express4 app. |
| utils/build/docker/nodejs/express4-typescript.Dockerfile | Copies TS debugger routes into the image. |
| utils/build/docker/nodejs/express/debugger/index.js | Adds capture-timeout fixture route for Express apps. |
| utils/build/docker/java/spring-boot/src/main/java/com/datadoghq/system_tests/springboot/debugger/DebuggerController.java | Adds capture-timeout endpoint/fixture for Java Spring Boot. |
| utils/build/docker/dotnet/weblog/Controllers/DebuggerController.cs | Adds capture-timeout endpoint/fixture for .NET weblog. |
| utils/_context/_scenarios/init.py | Adds DEBUGGER_CAPTURE_TIMEOUT scenario with tight capture/serialization budgets. |
| tests/test_the_test/test_compute_libraries_and_scenarios.py | Updates expected scenario list to include DEBUGGER_CAPTURE_TIMEOUT. |
| tests/test_the_test/test_ci_orchestrator.py | Adds regression test ensuring _is_supported gating matches expected fixture-enabled weblogs. |
| tests/test_the_test/scenarios.json | Maps new guardrail tests to scenarios (DEBUGGER_CAPTURE_TIMEOUT and DEBUGGER_PROBES_SNAPSHOT). |
| tests/debugger/utils/probes/probe_snapshot_size_cap.json | New probe fixture for snapshot-size cap behavior. |
| tests/debugger/utils/probes/probe_evaluation_timeout_regex.json | New probe fixture for ReDoS-style regex evaluation timeout. |
| tests/debugger/utils/probes/probe_evaluation_timeout_collection_filter.json | New probe fixture for expensive collection-filter evaluation timeout. |
| tests/debugger/utils/probes/probe_capture_timeout_reason.json | New probe fixture to validate notCapturedReason='timeout'. |
| tests/debugger/utils.py | Extracts captures_contain_data helper and adds line mapping for CaptureTimeout. |
| tests/debugger/test_debugger_probe_snapshot.py | Extends default max-length test expectations (string truncation metadata). |
| tests/debugger/test_debugger_guardrails.py | Adds new guardrail-focused debugger compliance tests. |
| tests/debugger/test_debugger_condition_errors.py | Switches condition-error tests to use the shared captures_contain_data helper. |
| manifests/ruby.yml | Marks new guardrail tests as missing_feature for Ruby. |
| manifests/python.yml | Marks new guardrail tests as missing_feature for Python. |
| manifests/php.yml | Marks new guardrail tests as missing_feature / incomplete_test_app where applicable. |
| manifests/nodejs.yml | Adds per-weblog declarations for the capture-timeout fixture availability; marks others missing_feature. |
| manifests/java.yml | Enables capture-timeout guardrail test for select Java weblogs; marks other guardrails missing_feature. |
| manifests/golang.yml | Marks new guardrail tests missing_feature/incomplete_test_app for Go. |
| manifests/dotnet.yml | Enables capture-timeout guardrail test for .NET; marks other guardrails missing_feature. |
| docs/understand/weblogs/end-to-end_weblog.md | Documents the new /debugger/snapshot/capture-timeout endpoint semantics. |
| .github/workflows/run-end-to-end.yml | Adds CI workflow step to execute the new DEBUGGER_CAPTURE_TIMEOUT scenario. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: f2c565e096
ℹ️ 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".
19a1599 to
273ee3c
Compare
…reasons and evaluation timeout behavior, with new rows disabled in manifests until tracer support is ready.
…re timeout reporting. 1MB snapshot cap and notCapturedReason=timeout behavior.
…ves the setup-time negative snapshot wait so evaluation-timeout assertions rely on collected scenario output instead of a fixed sleep.
…upported `notCapturedReason`.
…t relying on runner timing.
Detect delayed duplicate snapshots, validate complete backend request sizes, and avoid tracer-specific error messages. Tune probe limits and fixtures for reliable timeout and pruning coverage.
Preserve the fixture-specific scenario matrix after the orchestrator refactor and remove the unreachable legacy allowlist.
Keep the scenario categorized as dd_trace and explicitly exclude weblogs without the required fixture, preserving category semantics and the intended 14-weblog matrix.
05addb3 to
05e83b5
Compare
Bits found no code fix to apply🟢 Investigated · ⚪ No code fix needed
View in Datadog | Reviewed commit 05e83b5 · Any feedback? Reach out in #deveng-pr-agent |
grantseltzer
left a comment
grantseltzer
left a comment
There was a problem hiding this comment.
LGTM, but please take a look at few small comments.
| assert evaluation_errors, ( | ||
| "Evaluation-timeout snapshot has an empty evaluationErrors[]; the tracer must " | ||
| "report that expression evaluation was aborted." | ||
| ) |
There was a problem hiding this comment.
This only checks that evaluationErrors is non-empty. Do we want to check the specific evaluation failures?
There was a problem hiding this comment.
We intentionally avoid checking the exact failure message because timeout wording isn’t standardized across tracers. The shared contract currently requires a populated evaluationErrors[] and no captured data.
| class Test_Debugger_Snapshot_Size_Guardrail(_DebuggerSnapshotGuardrailTest): | ||
| """RFC guardrail for the completed snapshot size.""" | ||
|
|
||
| # Before enabling, verify this exceeds 1 MB without timing out; otherwise use a dedicated scenario or remove it. |
There was a problem hiding this comment.
Yes, theoretically. The fixture is designed to exceed 1 MiB using a large string and collection. The test also requires a payload-size pruning marker, so a timeout alone cannot produce a false pass. However, it remains disabled pending per-tracer activation and end-to-end verification.
| self.assert_all_probes_are_emitting() | ||
| self.assert_all_weblog_responses_ok() | ||
|
|
||
| for probe_id in self.probe_ids: |
There was a problem hiding this comment.
The for loop returns on the first probe, so any second probe is silently ignored, and the trailing raise AssertionError is only reachable when probe_ids is empty. Every probe file here has exactly one probe. Just assert that and index directly.
| def __init__( | ||
| self, | ||
| name: str, | ||
| doc: str, | ||
| weblog_env: dict[str, str | None], | ||
| ) -> None: |
| excluded_scenarios: | ||
| - APPSEC_MISSING_RULES # C++ 1.2.0 freezes when the rules file is missing | ||
| - APPSEC_CORRUPTED_RULES # C++ 1.2.0 freezes when the rules file is corrupt | ||
| - DEBUGGER_CAPTURE_TIMEOUT |
There was a problem hiding this comment.
If I'm not mistaken this scenario is used by only one test and is the only scenario of that test so this should be replaced by a manifest entry
There was a problem hiding this comment.
I may be missing some context here, since I’m not very familiar with this code. Based on the AI agent’s explanation, though, my understanding is that the current approach is preferable because the manifest already marks all debugger tests as irrelevant for cpp_httpd.
As I understand it, this intentionally prevents the orchestrator from scheduling the dedicated weblogs scenario without the capture-timeout fixture, while manifests only control pytest activation. Adding another manifest entry would therefore be redundant and would increase the CI matrix.
Does that reasoning make sense, or am I misunderstanding how this works?
Fail clearly when a fixture registers multiple probes instead of silently validating only the first.
Motivation
Add focused system-test coverage for the debugger guardrails required for GA.
Changes
evaluationErrors[]populated and no captured data.notCapturedReason=timeout, without incorrectly satisfying the snapshot-size guardrail.DEBUGGER_CAPTURE_TIMEOUTscenario with a 10 ms capture budget, CI orchestration, and compatible weblog filtering./debugger/snapshot/capture-timeoutfixture to supported .NET, Java, and Node.js weblogs and document the endpoint.test_default_max_lengthto verify that truncated strings reporttruncated=true.captures_contain_datahelper intotests.debugger.utils.Workflow
🚀 Once your PR is reviewed and the CI green, you can merge it!
🛟 #apm-shared-testing 🛟
Reviewer checklist
tests/ormanifests/is modified ? I have the approval from R&P teambuild-XXX-imagelabel is present