Introduce Mocked Backend V2 - #7668
Conversation
|
@codex review |
|
|
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness/robustness issues in scenario replay handling and a couple of codebase-convention/guarding problems that should be fixed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces a new in-process “mocked backend v2” HTTP server + interface so end-to-end scenarios can capture what the Agent submits without requiring real backend connectivity (and applies it to the APM_TRACING_E2E_SINGLE_SPAN scenario).
Changes:
- Add
MockBackendV2Server(local HTTP server) andinterfaces.backend_v2interface to log + deserialize incoming backend requests. - Wire a new
mocked_backend_v2scenario flag to point Agent backend URLs to the mock server and start/stop it with the scenario lifecycle. - Switch the single-span E2E scenario/test to validate spans via
interfaces.backend_v2, and add dependencies for content decoding.
File summaries
| File | Description |
|---|---|
| utils/mocked_backend/backend_v2.py | New local HTTP server that records and deserializes backend intake requests. |
| utils/interfaces/_backend_v2.py | New interface validator owning the mock server lifecycle and span/trace accessors. |
| utils/interfaces/_core.py | Adds “configured” guarding to interfaces (with a new runtime check in get_data). |
| utils/interfaces/init.py | Exposes the new backend_v2 interface singleton. |
| utils/_context/containers.py | Ensures Agent containers can resolve host.docker.internal when needed. |
| utils/_context/_scenarios/endtoend.py | Adds mocked_backend_v2 flag, wires Agent env to mock URL, and starts/stops the server. |
| utils/_context/_scenarios/init.py | Enables mocked backend v2 for the single-span E2E scenario (and disables proxy-for-agent). |
| tests/apm_tracing_e2e/test_single_span.py | Switches assertions to read spans from interfaces.backend_v2. |
| requirements.txt | Adds Brotli/zstandard/requests-toolbelt for content decoding and deserialization support. |
| manifests/java.yml | Adjusts the single-span test manifest entry (YAML key syntax). |
Review details
- Files reviewed: 9/10 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
![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: b42ff6faa8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@codex review |
There was a problem hiding this comment.
🟡 Changes recommended
The mocked backend v2 wiring needs xdist-safe port selection (worker-specific) and more robust teardown/error checking to avoid flaky or cross-worker interference.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
Previously missed (4) — in code that hasn't changed since the last review.
utils/_context/_scenarios/endtoend.py:274
- When running tests with pytest-xdist, each worker would currently point the agent at the default mocked-backend URL (worker_id="master"), which can cause port collisions and cross-talk between workers. Use the current xdist worker id when computing the mocked backend URL.
utils/interfaces/_backend_v2.py:26 - The mocked backend server is started with the default worker_id ("master"), which can bind all xdist workers to the same host port. Pass the current xdist worker id through to MockBackendV2Server so each worker gets a deterministic, non-conflicting port via get_host_port().
utils/interfaces/_backend_v2.py:47 - This docstring claims a third return value (trace_format), but the generator yields only (data, trace). Please update the docstring to match the actual return type to avoid confusing test authors.
This issue also appears on line 86 of the same file.
utils/scripts/replay_scenarios.sh:7
NOT_SUPPPORTEDis misspelled (and the comment has a typo), which makes the script harder to maintain. Rename it toNOT_SUPPORTEDand fix the comment spelling; update the reference in the loop accordingly.
utils/interfaces/_backend_v2.py:86
- This docstring claims a third return value (trace_format), but the generator yields only (data, span). Please update the docstring to match the actual return type to avoid confusing test authors.
Returns data, span and trace_format
- Files reviewed: 16/17 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
There are a few concrete correctness/operability issues (notably schema tests returning instead of skipping, and backend_v2 trace parsing assuming deserialized content is always present) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
Previously missed (3) — in code that hasn't changed since the last review.
utils/_context/_scenarios/endtoend.py:274
get_mocked_backend_v2_container_url()is called without a worker identifier, andMockBackendV2Serverdefaults to worker_id="master". If this scenario runs under pytest-xdist, multiple workers will try to bind/use the same host port (4901), causing port collisions/flaky startup. Consider plumbing the xdist worker id (e.g. via PYTEST_XDIST_WORKER or fixture-provided worker_id) through to both the server port selection and the agent env URL.
utils/interfaces/_backend_v2.py:66get_traces()assumes every backend_v2 entry hasdata["request"]["content"]. If request deserialization fails, the deserializer storestraceback/raw_contentand omitscontent, which would currently raise a KeyError here and hide the real deserialization error. Add an explicit guard that raises a clearer error (or skips the entry) whencontentis missing.
utils/scripts/replay_scenarios.sh:7- Spelling issues make this script harder to read/maintain: the comment has "backedn" and the variable is named
NOT_SUPPPORTED(3 P’s). Rename it toNOT_SUPPORTEDand update the reference.
tests/schemas/test_schemas.py:186
- Same as above:
returnsilently passes the test. Usepytest.skip(...)so CI reports an intentional skip for OTEL_COLLECTOR.
def test_agent(self):
if context.scenario.name in ("OTEL_COLLECTOR",): # need to clean that point...
return
- Files reviewed: 18/19 changed files
- Comments generated: 1
- Review effort level: Lite
|
@codex review |
There was a problem hiding this comment.
🔵 Needs a closer look
There are a couple of concrete issues in the updated scripts/server implementation (notably the replay scenario membership check and a resource-close issue) that should be addressed before approval.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
utils/mocked_backend/backend_v2.py:46
- The zstd decode path creates a stream reader but never closes it, which can leak resources over many requests. Wrap the reader in a context manager so it is always closed.
utils/scripts/replay_scenarios.sh:21 - This membership check uses regex substring matching against the flattened array, which can yield false positives if one scenario name is a substring of another. Use a delimiter-based exact match instead.
utils/scripts/replay_scenarios.sh:7 - Typo in the comment ("backedn") and awkward phrasing; consider fixing to "backend" to keep the script self-explanatory.
- Files reviewed: 18/19 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e34481f624
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| def get_mocked_backend_v2_container_url() -> str: | ||
| """The URL a container can use to reach the mock server on the docker host.""" | ||
| return f"http://{HOST_DOCKER_INTERNAL}:{get_mocked_backend_v2_port()}" |
There was a problem hiding this comment.
In the GitLab environment inspected, utils/ci/gitlab/templates.yml:13-18 runs against the remote DinD daemon at tcp://docker:2376, so host.docker.internal inside the Agent resolves to that daemon's host namespace, not to the separate CI job container where this Python server binds port 4901. Because this commit also removes APM_TRACING_E2E_SINGLE_SPAN from the GitLab exclusions, the Agent cannot reach this URL, no backend-v2 traces are captured, and both single-span assertions fail; expose an address reachable from nested containers or keep the scenario excluded in DinD.
Useful? React with 👍 / 👎.
Motivation
Unify how data are collected by datadog, by adding a Mocked backend that works in all situations (agentless, SSI, classic end-to-end).
Changes
And apply it to
APM_TRACING_E2E_SINGLE_SPANscenarioWorkflow
🚀 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