Wait for the agent to resolve container tags in origin detection tests - #7609
Wait for the agent to resolve container tags in origin detection tests#7609ddmatan wants to merge 4 commits into
Conversation
A single onboarding test that fails on a backend verification currently fails its CI job for good, and that job is a hard gate on the SSI release pipeline. Re-running the job by hand fixes it, which is the signal that these failures are not test results at all. Re-run the failed test in place instead, on the VM that is already provisioned, so a rerun costs seconds rather than a full re-provision. Each rerun issues a new weblog request and therefore verifies a brand new trace, which is what actually clears these flakes: re-reading the same trace id never will, that trace is already complete. `--only-rerun` keeps this narrow. A genuine injection regression still fails on the first attempt. Backend timeouts are excluded too, since they only fail after a 5 minute poll and re-running them would risk turning a clean test failure into a job timeout. To keep reruns honest, upload the onboarding JUnit report to Test Optimization. The report was already being generated and archived, only the upload was missing. Tests are tagged with the number of reruns they needed, so a test that only passed because it was re-run stays visible instead of disappearing behind a green job. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
|
The ci-runner tag is the sha256 of system-tests.Dockerfile + requirements.txt, so adding pytest-rerunfailures changed it and build_ci_image failed the CI_IMAGE-vs-computed-tag check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![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: 38e03eed0e
ℹ️ 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".
| # scenario name, so a detached `--reruns 2` would be parsed as the scenario "2". | ||
| RERUN_ARGS=(--reruns=2 --reruns-delay=15 | ||
| --only-rerun="failed to validate trace_id" | ||
| --only-rerun="Could not find runtime-id") |
There was a problem hiding this comment.
When a current backend trace omits the runtime-id tag entirely, utils/onboarding/backend_interface.py:85 raises KeyError: 'runtime-id' while indexing root_span["meta"]["runtime-id"]; execution never reaches the later Could not find runtime-id assertion. Consequently this filter does not rerun the common missing-tag case, so that backend flake can still block the SSI release. Either normalize the lookup to produce the intended assertion or include the actual exception in the rerun filter.
Useful? React with 👍 / 👎.
cbeauchesne
left a comment
cbeauchesne
left a comment
There was a problem hiding this comment.
Thanks for working on improving the reliability of these tests. However, I don’t think adding retry logic is the right direction here.
The underlying issue is that these scenarios depend on a real backend whose availability and performance we do not control. Retries may reduce the visible failure rate, but they do not address that design flaw. They would also add significant complexity to the framework and could hide genuine failures or make them slower to diagnose.
The known solution is to use a mocked backend that we control. I think our effort should go toward implementing that solution rather than building increasingly complex retry mechanisms around the current behavior (see https://datadoghq.atlassian.net/browse/APMSP-2478). We already have many bits in place, as we use a mocked backend for other scenarios. The biggest part will be the piping between SSI components, and this mock.
For those reasons, I don’t think we should move forward with this approach.
Origin detection asserts on `_dd.tags.container`, which the agent attaches from its tagger, not the backend: the tracer sends only a container id. The tagger discovers containers asynchronously, and the agent holds a payload for at most 12s waiting for it before sending the trace untagged. So a tagger that is slow to see the weblog container produces a trace with nothing to assert on, and the test fails with the container running fine. Wait for `datadog-agent tagger-list` to know the weblog containers before sending the request we measure. Best effort: if it never catches up we send the request anyway, so a real regression fails exactly as it does today. Replaces the earlier pytest-rerunfailures approach, which waited for the same thing by accident and needed a new dependency to do it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ddmatan
changed the title
3 participants
Origin detection tests fail intermittently with the trace present, on time, fully enriched — and only
_dd.tags.containermissing. The container is running fine, so it looks like a backend problem. It isn't.That tag is attached by the agent, not the backend. The tracer sends only a container id (
Datadog-Container-ID); the agent resolves it into tags through its tagger, which discovers containers asynchronously. It holds the payload up to 12s waiting for that, then sends the trace untagged and moves on. Nothing errors.Where that comes from:
architecture— tracer sends the id, agent calls the tagger and attaches_dd.tags.container; the backend's only job is rewriting it intoddtagsdatadog-agent/pkg/trace/agent/agent.go—tagContainersTags, written fromconf.ContainerTags(containerID)datadog-agent/pkg/trace/containertags/buffer.go—maxBufferDuration = 12s, then shipped untaggedSo the test measures too early: we wait for the weblog port, but never for the agent. This waits for
datadog-agent tagger-listto know the weblog containers before sending the request we measure.Gotchas:
origin_detection=True, and only on the SSH path (krunvm is untouched).@cbeauchesne this drops the retry logic entirely. On the mocked backend — I don't think it would have caught this particular failure, since the tag never reaches the backend in the first place. Happy to talk it through.
🤖 Written with Claude Code