ci: dogfood ddtest for the internal suite (standalone pilot) - #19870
Draft
gnufede wants to merge 46 commits into
Draft
ci: dogfood ddtest for the internal suite (standalone pilot)#19870gnufede wants to merge 46 commits into
gnufede wants to merge 46 commits into
Conversation
Add an opt-in, manually-triggered CI path that runs the `internal` riot suite through ddtest (plan + run + file splitting) instead of the legacy `riot run <hash> -- --ddtrace` path, without any changes to ddtest and without touching the generated-config pipeline. ddtest stays unchanged; all riot knowledge lives in a new Python bridge (`scripts/ddtest-riot.py`) that uses riot as a library: - riotfile: the `internal` suite path is now a queryable env var (`DDTEST_SUITE_PATH`) instead of a literal baked into the pytest command, so `riot run internal` is unchanged with or without ddtest, and the bridge can read the suite location per venv. - scripts/ddtest-riot.py: `hashes <pattern>` lists riot venvs; `venv-env <hash>` emits the env that activates a venv (VIRTUAL_ENV, PATH with venv bin first, PYTHONPATH site-packages, RIOT_*, DDTEST_SUITE_PATH), mirroring `riot run --pass-env`. Pure hint-based path math so it works without interpreters on PATH. Registered in run-script-doctests.py. - .gitlab/ddtest.yml: a standalone `ddtest-internal-pilot` job, gated `when: manual` + `allow_failure: true`, that builds its own venv, downloads ddtest, activates the venv via the bridge, and runs `ddtest plan`/`run`. Self-contained (no gen_gitlab_config.py wiring, no build_base_venvs dep). - .gitlab-ci.yml: include the pilot. What: opt-in ddtest dogfood path for the `internal` suite. Why: validate ddtest plan/run + file splitting in real dd-trace-py CI as a first step toward replacing gitlab-level parallelization, keeping riot knowledge out of ddtest. E2E testing: trigger `ddtest-internal-pilot` from the GitLab UI on a branch; it runs `riot generate --python=3.10`, downloads ddtest, activates the `internal` Py3.10 venv via `scripts/ddtest-riot.py venv-env 4f70b3c`, runs `ddtest plan -p python -f pytest --tests-location tests/internal --min-parallelism 1 --max-parallelism 1`, then `ddtest run --ci-node 0 --ci-node-workers ncpu`, and asserts ddtrace/pytest import in the venv.
Codeowners resolved asResolved from the full PR diff against |
Circular import analysis
|
Dependency direction analysis
|
Contributor
|
Document the end state we are building toward: running dd-trace-py's test suites through ddtest (plan/run + file splitting) instead of the legacy `riot run <hash> -- --ddtrace` path, replacing gitlab-level parallelization. Generalizes the standalone `internal` pilot (`.gitlab/ddtest.yml`) to all test suites via gen_gitlab_config.py, behind a per-suite `ddtest: true` suitespec key. ddtest stays unchanged; riot knowledge stays in the Python bridge. Covers: principles (single-venv ddtest, riot knowledge in the bridge, suitespec-inherited selection, opt-in per suite, K from suitespec parallelism), the riotfile/suitespec/gen_gitlab_config.py/tests.yml changes, the CI graph (ddtest plan downstream of suitespec generation, so it does not run on deselected suites/venvs), the rollout sequence, and what ddtest does not change (including why snapshot suites parallelize safely with per-job testagent services). What: design doc for generalizing the ddtest pilot to all test jobs. Why: capture the agreed design (Option 1: ddtest unchanged, riot knowledge in the bridge) before implementing the gen_gitlab_config.py integration. E2E testing: doc only; no behavior change.
gnufede
added
changelog/no-changelog
riot's Session.run applies Python str.format(cmdargs=...) to the venv
command before passing it to bash. The unescaped `${DDTEST_SUITE_PATH}` was
parsed as a str.format field `{DDTEST_SUITE_PATH}` (the `$` is literal),
raising `KeyError: 'DDTEST_SUITE_PATH'` during `riot run internal`.
Escape the braces as `${{DDTEST_SUITE_PATH}}` so str.format emits
`${DDTEST_SUITE_PATH}`, which bash then expands from the venv env to
`tests/internal/`. The resulting command is identical to the pre-change
`pytest ... tests/internal/`.
What: fix the riotfile regression from the ddtest dogfooding Step 1.
Why: riot's command formatting is str.format, not bash; `${VAR}` must be
escaped for str.format to survive to bash.
E2E testing: `riot run internal -- --ddtrace` now formats to
`pytest ... --ddtrace ${DDTEST_SUITE_PATH}/` and bash expands it to
`pytest ... --ddtrace tests/internal/`; `riot list internal` unchanged.
The staged deletion of .gitlab/ddtest.yml (part of the uncommitted full-picture work) was swept into the riotfile fix commit by a stash/pop, so the remote had the file deleted while .gitlab-ci.yml still included it — GitLab rejected pipeline creation with "Local file .gitlab/ddtest.yml does not exist!". Restore the pilot file from b4040cc so the pushed branch is self-consistent: .gitlab-ci.yml includes .gitlab/ddtest.yml and the file exists. The full-picture round (which removes both the pilot and its include together) remains uncommitted in the working tree. What: restore the standalone pilot file that the pushed .gitlab-ci.yml still references. Why: keep the pushed branch consistent (include + file both present) until the full picture lands as one unit. E2E testing: GitLab pipeline creation no longer 400s on the missing include; the manual ddtest-internal-pilot job is available again.
The pilot was `when: manual`, but manual jobs cannot be triggered on a branch before it merges. Flip the rules to run automatically on every pipeline (push and merge_request_event) so the ddtest path is exercised on every push and its results are observable without merging. `allow_failure: true` stays so a pilot failure does not block merges while the ddtest path is being validated; drop it once the pilot is green. What: make ddtest-internal-pilot auto-run. Why: observe ddtest dogfooding results without needing to merge first. E2E testing: on the next push, ddtest-internal-pilot starts automatically (stage: tests) and runs riot generate + ddtest plan/run in the Py3.10 internal venv via scripts/ddtest-riot.py.
BenchmarksBenchmark execution time: 2026-08-27 07:19:27 Comparing candidate commit c576ee2 in PR branch Found 0 performance improvements and 5 performance regressions! Performance is the same for 581 metrics, 10 unstable metrics, 1 known flaky benchmarks, 17 flaky benchmarks without significant changes.
|
The pilot rebuilt ddtrace inline via `riot generate`, which compiles every native extension and OOMKilled the default-sized pod (exit 137) before ddtest plan/run ever ran. Move the pilot into the child pipeline (include .gitlab/ddtest.yml from .gitlab/tests.yml instead of the parent .gitlab-ci.yml) so it can `needs: build_base_venvs` and restore the prebuilt .riot/venv_py310 artifact. The pilot no longer runs `riot generate` — it reuses the venv build_base_venvs already produced (with ddtrace compiled), so it needs only default runner resources and never recompiles. Also fix --tests-location: ddtest treats it as a raw glob (it does not combine it with python_files), so "tests/internal" matched nothing; use "tests/internal/**/test*.py" to actually discover the 50 files. What: pilot reuses build_base_venvs instead of recompiling ddtrace. Why: recompiling native extensions inline OOMs the pod; the venv already exists in the child pipeline via build_base_venvs. E2E testing: on the next push, ddtest-internal-pilot runs in the child pipeline (stage: riot), restores the Py3.10 venv, downloads ddtest, activates it via scripts/ddtest-riot.py, and runs plan/run — no compile.
Update the final-picture design doc with the lesson from the pilot OOM: ddtest jobs must live in the child pipeline and reuse build_base_venvs (never run `riot generate` / recompile ddtrace). An earlier pilot built its own venv inline in the parent pipeline and OOMKilled recompiling native extensions on a default-sized pod. - Promote "never compiles ddtrace, reuses build_base_venvs" to principle 1. - Correct the pilot rollout step: child pipeline (included from .gitlab/tests.yml), needs: build_base_venvs, no riot generate, auto-run, allow_failure: true. Note the OOM and the fix. - Drop the stale "allow_failure: true initially / drop it once green" wording from the gen_gitlab_config.py step (per earlier feedback). What: design-doc-only update from the pilot OOM learning. Why: make the "never recompile" invariant explicit so the full-picture implementation and any future pilot inherit it. E2E testing: doc only; no behavior change.
The child pipeline's stages are rewritten by gen_gitlab_config.py from the selected suites' stages; the `riot`/`exploration` stages in the tests.yml template are overwritten. The `internal` suite is stage `core`, so `core` is always present — but `riot` is not, and the pilot's `stage: riot` made GitLab reject pipeline creation with "chosen stage riot does not exist". Use `stage: core` (the internal suite's stage), which is always in the generated stages. What: fix pilot stage to one that survives gen_gitlab_config's rewrite. Why: gen_gitlab_config rewrites stages from selected suites; riot is not among them. E2E testing: on the next push, the child pipeline accepts the pilot job and ddtest-internal-pilot runs in stage core.
The bridge emitted KEY=value lines unquoted, but riot's RIOT_VENV_FULL_PKGS and _CI_DD_TAGS values contain spaces and '<'/'~' (e.g. 'hypothesis<6.45.1'). When the pilot `eval`s the output, bash read '<' as a redirection and failed with "syntax error near unexpected token `newline'". Shell-quote values with shlex.quote so the output is safe to eval; ddtest parses lines by splitting on the first '=', so a single-quoted value round-trips. What: quote venv-env values so eval is safe. Why: unquoted values with spaces/metacharacters break the pilot's eval. E2E testing: `eval "$(python scripts/ddtest-riot.py venv-env <hash>)"` no longer errors; every value parses as a single shell token.
ddtest run failed with `ModuleNotFoundError: No module named 'envier'`. build_base_venvs only installs ddtrace into the base venv (venv_py310); the per-hash deps (envier, pytest, ...) are installed into the prefix (venv_py310_<ident>) lazily by `riot run` via VenvInstance.prepare. The pilot skipped `riot run` entirely, so the prefix was never prepared and envier was missing. Add a prepare step — `riot -P -v run -s <hash> -- true` — before activating the venv. `-s` skips the ddtrace reinstall (already in the base from build_base_venvs) but still installs the per-hash deps into the prefix, exactly as the legacy `riot run -s <hash> -- --ddtrace` job does before pytest. ddtest then runs against the prepared prefix. What: prepare the per-hash deps venv before ddtest. Why: build_base_venvs does not install per-hash deps; riot run does, and ddtest needs them present. E2E testing: on the next push, `python -c "import ddtrace, pytest"` passes (envier importable) and ddtest run executes the internal tests.
The pilot prepared the per-hash venv with `riot -P -v run -s <hash> -- true` — a no-op riot command run solely to trigger VenvInstance.prepare. That's a hack: it shells out to riot just to call prepare, then ddtest runs outside riot. Replace it with a `prepare` subcommand in the bridge that calls riot's `VenvInstance.prepare(env, skip_deps=True)` directly — the same library API `riot run` calls internally. skip_deps=True skips the ddtrace reinstall (already in the base from build_base_venvs); the per-hash deps (envier, ...) are installed into the prefix. Same effect as the dummy riot call, but via the library API. This is Option A (pragmatic): ddtest still runs outside riot, the bridge still reconstructs the activation env. The long-term direction (Option B) is to run ddtest inside riot so riot activates natively and the bridge's env reconstruction is unnecessary; that's deferred because it requires solving riot's baked-command obstacle. What: bridge `prepare` subcommand; pilot calls it instead of riot run. Why: the dummy riot run was a hack; the library API is the direct equivalent. E2E testing: on the next push, the pilot runs `python scripts/ddtest-riot.py prepare 4f70b3c` (installs envier etc. via VenvInstance.prepare), then ddtest plan/run with envier importable.
The pilot's `prepare` step ran but `envier` was still missing at `ddtest run` time, with no log line showing whether prepare actually installed deps. Two changes to diagnose: 1. bridge `prepare` subcommand prints "Preparing/Prepared riot venv <hash>" markers so the CI log shows prepare ran (or silently no-op'd). Exceptions propagate (nonzero exit). 2. pilot verifies `import ddtrace, pytest, envier` immediately after prepare+activate, before ddtest plan/run. If prepare skipped the deps install (e.g. prefix existed but empty), the failure surfaces here with a clear cause instead of a confusing ModuleNotFoundError deep in ddtest run. What: add prepare logging + early import verification. Why: envier missing after prepare with no signal; need to see whether prepare ran and whether deps are present. E2E testing: on the next push, the log shows "Preparing riot venv 4f70b3c" then either "Prepared" or an error, and `python -c "import ...envier"` passes or fails loudly before ddtest.
The bridge computed the base venv path from the Python hint ("3.10" ->
venv_py310), but riot's Interpreter.venv_path uses the full version
(version().replace(".","") -> venv_py31020 for Python 3.10.20 in CI).
So in CI the bridge pointed at venv_py310 while riot's real venv was
venv_py31020. Two consequences:
1. prepare: inst.prepare(skip_deps=True) saw the prefix at the wrong
path (venv_py310_<ident>), and since it didn't exist there, the
install was skipped or went to the wrong place — envier never landed
in the venv ddtest ran in.
2. venv-env: PYTHONPATH pointed at venv_py310/.../site-packages
(nonexistent) instead of venv_py31020/.../site-packages, so
`import envier` failed even if installed.
Fix _inst_base_venv to prefer riot's own inst.py.venv_path (the real
path riot uses, including the patch version) and fall back to the
hint-based base_venv() only when the interpreter is not probeable
(local dev without that Python on PATH). This is the env-reconstruction
fragility flagged earlier; using riot's own path property avoids
diverging from riot's venv layout.
What: use inst.py.venv_path for the base venv path in the bridge.
Why: the hint-based path (venv_py310) diverges from riot's real path
(venv_py31020) when the interpreter has a patch version.
E2E testing: in CI, prepare installs envier into the correct prefix
(venv_py31020_<ident>), venv-env PYTHONPATH points at
venv_py31020/.../site-packages, and `import envier` succeeds.
The bridge emitted `KEY=value` lines, which `eval` sets as shell variables but does NOT export. Child processes (python) therefore don't see them: `PYTHONPATH` was set in the shell but `python -c "import pytest"` failed with `ModuleNotFoundError: No module named 'pytest'` because python never received PYTHONPATH. Emit `export KEY=value` so the variables are exported and inherited by python subprocesses ddtest spawns. Verified locally: after the fix, `eval "$(scripts/ddtest-riot.py venv-env <hash>)"` exports PYTHONPATH and `python -c "import pytest"` succeeds. What: prefix env lines with `export`. Why: non-exported shell vars aren't inherited by child processes; python needs PYTHONPATH (and VIRTUAL_ENV/PATH) in its environment. E2E testing: in CI, `python -c "import ddtrace, pytest, envier"` after eval succeeds; ddtest plan/run proceed.
ddtest run with --ci-node-workers ncpu spawned ~47 workers (one per
file) on the many-core k8s pod. All 47 wrote to the same pytest-cov
sqlite database, corrupting it:
sqlite3.OperationalError: no such table: other_db.file
The legacy path avoids this: testrunner.yml sets
PYTEST_XDIST_AUTO_NUM_WORKERS=4 ("auto/logical doesn't work well
in k8s so we manually set it"). Match it: use 4 workers instead of
ncpu.
What: cap ddtest ci-node-workers at 4.
Why: ncpu on k8s pods is too high; 47 parallel workers corrupt
pytest-cov's sqlite DB.
E2E testing: ddtest run uses 4 xdist workers (matching legacy),
coverage DB no longer corrupts.
Generalize the ddtest dogfooding from a standalone pilot to the generated-config pipeline. Suites opt in via `ddtest: true` in suitespec; gen_gitlab_config.py emits ddtest-plan and ddtest-run jobs (per venv, fanned out via parallel matrix) instead of the legacy riot job for that suite. ddtest stays unchanged; riot knowledge stays in the bridge (scripts/ddtest-riot.py). Changes: - scripts/gen_gitlab_config.py: SuiteVenvInfo carries per-venv (hash, python_hint) pairs; _emit_ddtest_jobs emits ddtest-plan (matrix over hashes) + ddtest-run (matrix over hashes x K nodes) with matrix-matched needs for per-hash plan artifacts. - .gitlab/tests.yml: .ddtest_base(+_snapshot), .ddtest_plan (no snapshot variant), .ddtest_run(+_snapshot) templates. Prepare via bridge, activate, verify import. 4 workers (not ncpu). Raw glob --tests-location. - tests/suitespec.yml: internal: ddtest: true. - .gitlab/ddtest.yml: removed (pilot superseded); include removed in same commit (missing-include lesson). - docs/ddtest-dogfooding.md: updated. What: full-picture ddtest integration via gen_gitlab_config.py. Why: replace gitlab-level parallelization with ddtest plan/run. E2E testing: gen_gitlab_config emits ddtest-plan + ddtest-run for internal; each plan prepares venv + writes .testoptimization/; each run downloads plan + runs files via ddtest run --ci-node-workers 4.
gnufede
force-pushed
the
gnufede/ddtest-dogfooding
branch
from
August 26, 2026 12:58
7267505 to
7ae1d3f
Compare
Add `ddtest: true` (with `allow_failure: true` for safety) to all 81 suites that have parallelism > 1 or venvs_per_job > 1 across all suitespec files. `internal` was already opted in (without allow_failure, since it's proven). This generalizes the ddtest dogfooding from the `internal` pilot to every suite that was previously parallelized at the gitlab level. Opting in replaces the legacy riot job for that suite with ddtest-plan + ddtest-run jobs (emitted by gen_gitlab_config.py). `allow_failure: true` on the 81 new suites so a ddtest regression does not block merges while each suite's ddtest path is validated in CI; drop it per-suite as each goes green. `internal` keeps allow_failure off (already proven by the pilot). What: ddtest: true on all 81 parallelized suites. Why: generalize ddtest dogfooding beyond the internal pilot. E2E testing: gen_gitlab_config emits ddtest-plan + ddtest-run for each opted suite; each plan prepares its venv, each run downloads its plan and runs its files via ddtest.
Remove `allow_failure: true` from all 81 ddtest suites so failures are visible and block merges, as requested. The `internal` suite (proven by the pilot) never had `allow_failure`. The `test_process_tags` snapshot failures (service: 'pytest' vs expected 'tests.internal') are a known `python -m pytest` (ddtest) vs `pytest` (riot) behavioral difference in ddtrace's own pytest plugin, not a ddtest infrastructure bug; they will surface as real failures now and need investigating as a follow-up. What: remove allow_failure: true from all 81 ddtest suites. Why: see failures directly instead of hiding them. E2E testing: ddtest suite failures are now visible in CI.
When running `python -m pytest <files>`, CPython's runpy sets
sys.argv[0] to the pytest module's __init__.py file path (e.g.
/path/to/pytest/__init__.py). detect_service() in
_inferred_base_service.py treated sys.argv[0] as a user-provided
file path, resolved it, and deduce_package_name() walked up to find
pytest/__init__.py, returning "pytest" as the inferred service
name.
When running `pytest <files>` (console script), sys.argv[0] is
"pytest" (a command name, not a file path), which detect_service
skips, then processes the actual test file path, correctly
returning "tests.internal".
Fix: `executable_args = {0}` — always skip sys.argv[0] since it is
never a user-provided test file argument. This makes `python -m pytest`
and `pytest` produce the same service, matching existing
snapshots (no regeneration needed).
This resolves the 18 snapshot test failures
(test_process_tags_{activated,edge_case,error} × 6 Python versions)
seen in the ddtest dogfooding CI.
What: one-line fix in detect_service + 2 test cases.
Why: python -m pytest (ddtest) vs pytest (riot) produced
different span services; the snapshots expect the module name.
E2E testing: both `pytest file.py` and `python -m pytest file.py`
now produce "tests.internal" (matching snapshots).
This reverts commit 4d64c36.
Revert the core ddtrace fix (detect_service in _inferred_base_service.py) and replace it with a test-only workaround: the ddtest .ddtest_base template's before_script now derives DD_SERVICE from DDTEST_SUITE_PATH (converting / to ., e.g. tests/internal -> tests.internal). Root cause: `python -m pytest` (ddtest) sets sys.argv[0] to pytest's __init__.py path, which detect_service picks up as a test path, producing service='pytest' instead of the module name. The legacy `pytest` (riot) invocation skips sys.argv[0] and derives the service from the actual test file path. The core fix (commit 4d64c36) changed detect_service to always skip sys.argv[0], but that's a behavioral change for all ddtrace users. This reverts it and uses a test-only workaround instead: set DD_SERVICE explicitly in the ddtest job env so the ddtrace pytest plugin reports the same service as the legacy path. What: revert core fix; set DD_SERVICE from DDTEST_SUITE_PATH in ddtest templates. Why: avoid changing core ddtrace logic; fix is test-only. E2E testing: snapshot tests in test_process_tags should see service=tests.internal (matching snapshots).
Add "DDTEST_SUITE_PATH": "<path>" to the env and replace the
literal test path in the command with ${{DDTEST_SUITE_PATH}} for
all 80 ddtest suites (internal was already done). This makes the
test location a queryable env var instead of a literal baked
into the pytest command, so the ddtest bridge can read the
suite location per venv and ddtest can use
--tests-location "${DDTEST_SUITE_PATH}/**/test*.py".
The ${{DDTEST_SUITE_PATH}} escaping is for riot's str.format
(${VAR} must be ${{VAR}} to survive to bash).
Special cases handled:
- contrib::django: child venv with --ignore flags — both paths
replaced with ${{DDTEST_SUITE_PATH}}/...
- contrib::aiohttp: two test files — both replaced with
${{DDTEST_SUITE_PATH}}/...
- contrib::pymemcache: --ignore path — replaced with
${{DDTEST_SUITE_PATH}}/autopatch ${{DDTEST_SUITE_PATH}}
- ci_visibility::pytest, ci_visibility::ci_visibility: multi-line
commands with --ignore — paths replaced, envs merged
- contrib::kafka, contrib::pymemcache, contrib::aiohttp: child
venvs with command — parent gets env, child command gets path
replaced
- integration: parent venv gets env + command replaced
- contrib::selenium: -c /dev/null preserved, env merged
- Suites with no env= (dd_coverage, crashtracker, etc.): env= added
before command=
What: DDTEST_SUITE_PATH for all 80 remaining ddtest suites.
Why: ddtest bridge needs the suite path as an env var to
discover tests per venv.
E2E testing: all 81 ddtest suites have DDTEST_SUITE_PATH in env
and ${{DDTEST_SUITE_PATH}} in command; riot list and
gen_gitlab_config work.
K (per-venv CI node count for file splitting) was incorrectly derived from the legacy parallelism/venvs_per_job knobs, which control venv PACKING (how many venvs per CI job) — a different axis from file splitting. This caused a huge job explosion (e.g. parallelism=6 → 6 file-groups per venv × 12 venvs = 72 jobs instead of legacy 6). Fix: K defaults to 1 (no file splitting — each venv runs all its files in one job, closest to legacy semantics). A suite can override K with a new `ddtest_nodes` suitespec key. The legacy parallelism/venvs_per_job knobs are no longer used for K. Set `ddtest_nodes: 2` on the `internal` suite to test K=2 file splitting (12 venvs × 2 = 24 run jobs) as the first non-trivial K. What: decouple ddtest file splitting (K) from legacy venv packing. Why: parallelism/venvs_per_job is venv packing, not file splitting; reusing it for K caused a job explosion. E2E testing: internal has K=2 (24 run jobs); other suites have K=1 (one job per venv, no file splitting).
Remove `ddtest: true` from three suites that can't run under ddtest:
- `contrib::aws_lambda`: `datadog-lambda` pins ddtrace 2.20.1, but
ddtest requires >= 4.10.3. Can't run until datadog-lambda upgrades.
- `contrib::azure_functions:eventhubs`: `_cffi_backend` missing in the
`wait` venv (native dep issue in wait-for-services.py). Pre-existing,
not caused by ddtest, but fails visibly without allow_failure.
- `appsec::appsec_integrations_pygoat`: command is `bash run_pygoat.sh ...`
which starts the pygoat app AND runs pytest. ddtest only runs pytest,
so the app isn't started → Connection refused. ddtest can't handle
bash-script commands that start services.
Also revert the DD_SERVICE workaround (`export DD_SERVICE=$(echo
"${DDTEST_SUITE_PATH}" | tr '/' '.')` in .ddtest_base before_script).
It was wrong: it converts the path to dots (tests.contrib.djangorestframework)
but the expected service is the module name from detect_service (django),
not the path-as-dots. The root cause (python -m pytest vs pytest in
detect_service) needs a proper fix in ddtrace, not a workaround.
What: remove 3 unsuitable suites from ddtest; revert DD_SERVICE workaround.
Why: these suites have deps/commands incompatible with ddtest; the
DD_SERVICE workaround produces wrong services for some suites.
E2E testing: 79 ddtest suites remain; DD_SERVICE is no longer set
(snapshots will fail on service mismatch until detect_service is fixed).
ddtest plan takes ~90s, so file splitting (K=2) only makes sense for suites that take at least 3 minutes (each half > 90s makes the 2x plan overhead worthwhile). For suites < 3 minutes, ddtest adds overhead with no time gain. Changes: - Remove `ddtest: true` from 65 suites that take < 3 minutes (leaving them on the legacy riot path). - Keep `ddtest: true` + `ddtest_nodes: 2` on the 14 suites that take > 3 minutes: internal, integration_agent, integration_testagent, contrib::opentelemetry, contrib::graphql, contrib::botocore, contrib::loguru, contrib::urllib3, contrib::structlog, contrib::molten, contrib::mako, contrib::logbook, llmobs::mistralai, appsec::appsec_threats_fastapi_iast. - Add `ddtest: true` + `ddtest_nodes: 2` to the 9 new suites that were not previously opted in (graphql, loguru, urllib3, structlog, molten, mako, logbook, mistralai, appsec_threats_fastapi_iast). The riotfile DDTEST_SUITE_PATH changes remain for all suites (harmless for non-ddtest suites — legacy riot run still works). What: limit ddtest to >3min suites; K=2 for those, opt out rest. Why: ddtest plan overhead (~90s) only pays off for slow suites. E2E testing: 14 ddtest suites with K=2; 65 suites revert to legacy.
Member
Author
|
@DataDog review |
…ination
Two fixes:
1. Add DDTEST_SUITE_PATH env + ${{DDTEST_SUITE_PATH}} command to the 9 new
ddtest suites (graphql, loguru, urllib3, structlog, molten, mako,
logbook, mistralai, appsec_threats_fastapi_iast). These were added to
suitespec but not to the riotfile, so --tests-location was empty and
ddtest discovered 4,035 files (the whole repo) instead of the
suite's files.
2. Fix PYTHONPATH contamination: the .ddtest_base before_script
activated the venv with eval "$(scripts/ddtest-riot.py venv-env ...)"
which exported PYTHONPATH pointing at the suite venv. The riot run -s
wait -- testagent step (in before_script) inherited that PYTHONPATH
and loaded modules from the wrong venv (e.g. typing_extensions from
the molten venv while running in the wait venv). Fix: activate in a
subshell in before_script (eval "..." && python -c "..."), and
re-activate in each plan/run script.
What: add missing DDTEST_SUITE_PATH; fix PYTHONPATH leaking into wait.
Why: 9 suites had no suite path (4035 files discovered); PYTHONPATH
contaminated the wait venv (typing_extensions version conflict).
E2E testing: each ddtest suite discovers only its files; wait-for-services
runs in a clean env.
![datadog-official[bot]](https://avatars.githubusercontent.com/in/1358509?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
The ddtest switch does not keep each suite's discovery path or pytest selection. Matrix needs also combine plan artifacts from unrelated riot hashes, so some new jobs can skip required tests or use a plan for another environment.
🤖 Datadog Autotest · Commit 0f9bff1 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| continue | ||
| k = _ddtest_k(suite_config) | ||
| LOGGER.info("Suite %s: ddtest (venvs=%d, nodes/venv=%d)", suite, len(venvs), k) | ||
| _emit_ddtest_jobs(f, suite, stage, clean_name, suite_config, venvs, k) |
Contributor
There was a problem hiding this comment.
The integration jobs run the wrong test set and can fail under unsupported coverage or tracing settings.
Assertion details
- Input: Generate integration_agent or integration_testagent through the new ddtest branch.
- Expected:
The generated jobs must keep the suite-specific pytest selection and required arguments. - Actual:
The ddtest branch does not use the riot pytest command. It loses --no-cov and --ignore-glob='*civisibility*'. It then includes tests that the integration suite must exclude.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| # Dedup PYTHON_VERSIONs: several hashes share a Python version, but | ||
| # build_base_venvs only needs to be downloaded once per version. | ||
| seen_py: set[str] = set() | ||
| for _h, py in venvs: |
Contributor
There was a problem hiding this comment.
Large duplicate artifact downloads increase CI time and resource use for every matrix entry.
Assertion details
- Input: Run an enabled suite that supports more than one Python version.
- Expected:
Each matrix entry must download only the base artifact for its current Python version. - Actual:
Each plan and run entry needs the base artifacts for every Python version in the suite. The job uses only its own PYTHON_VERSION.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
scripts/ddtest-riot.py has a PEP 723 inline script metadata (`# /// script` with `riot>=0.22.0`), so it must be invoked via its shebang (`#!/usr/bin/env scripts/uv-run-script`) which uses `uv run --script` to install riot. The ddtest templates were invoking it as `python scripts/ddtest-riot.py ...` which bypasses the shebang and runs with the system Python — which doesn't have `riot` installed: ModuleNotFoundError: No module named 'riot' Fix: use `scripts/ddtest-riot.py ...` (via shebang) instead of `python scripts/ddtest-riot.py ...`, matching how other scripts in the repo are invoked (e.g. `scripts/gen_gitlab_config.py`). What: invoke ddtest-riot.py via shebang, not python. Why: `python` bypasses uv-run-script, so riot isn't installed. E2E testing: ddtest-riot.py prepare/venv-env now run via uv-run-script which installs riot.
Address two PR review findings: #2 (P1): Keep suite-specific pytest selection The integration suites have --no-cov and --ignore-glob='*civisibility*' in their riot commands. ddtest runs 'python -m pytest <files>' and doesn't see these flags. Fix: extract the pytest flags from the riot command (everything between 'pytest' and '{cmdargs}') and pass them as PYTEST_ADDOPTS in the ddtest job variables. ddtest's Python platform appends --ddtrace to PYTEST_ADDOPTS, so the flags are carried through. #3 (P1): Match each run to its plan artifact All plan jobs upload .testoptimization/ as an artifact, and all run jobs download all of them — they overwrite each other. Fix: partition the plan artifact by RIOT_HASH. The plan job moves .testoptimization/ to .testoptimization-${RIOT_HASH}/ and uploads that. The run job renames .testoptimization-${RIOT_HASH}/ back to .testoptimization/ before running ddtest. Each run only uses its own venv's plan. #4 (P2): not addressed — the needs: build_base_venvs matrix downloads all Python versions' artifacts, but each job only uses its own PYTHON_VERSION. This matches the legacy pattern (legacy jobs also needs: build_base_venvs with a matrix), so it's not a regression. What: PYTEST_ADDOPTS for riot flags; partition plan by RIOT_HASH. Why: integration suites lost --no-cov/--ignore-glob; all plan artifacts overwrote each other. E2E testing: integration suites carry --no-cov --ignore-glob; each run downloads only its own plan (partitioned by hash).
When running `python -m pytest <files>`, CPython's runpy sets sys.argv[0] to the pytest module's __init__.py file path (e.g. /path/to/pytest/__init__.py). detect_service() treated this as a user-provided file path, and deduce_package_name() walked up to find pytest/__init__.py, returning "pytest" as the inferred service name. When running `pytest <files>` (console script), sys.argv[0] is "pytest" (a command name), which detect_service skips, then processes the actual test file path, correctly returning "tests.internal". Fix: skip sys.argv[0] when it's a __init__.py file — a __init__.py is a package marker, never a user-provided test file. This is scoped: only __init__.py files are skipped (not all sys.argv[0]), so non-pytest cases like `python my_script.py` (where my_script.py IS the thing being traced) are unaffected. This resolves the snapshot test failures (service: 'pytest' vs expected 'tests.internal'/'tests.contrib.graphql') seen in the ddtest dogfooding CI. Two new test cases covering python -m pytest with module __init__.py as sys.argv[0]. What: skip __init__.py as argv[0] in detect_service (scoped fix). Why: python -m pytest sets argv[0] to pytest's __init__.py; detect_service picks it up as a test path → wrong service. E2E testing: both `pytest file.py` and `python -m pytest file.py` produce the module name (matching snapshots).
…ice" This reverts commit 3247a0f.
When running via `python -m pytest <files>`, CPython's runpy sets sys.argv[0] to the pytest module's __init__.py file path. detect_service() in _inferred_base_service.py treats this as a user-provided file path and returns 'pytest' as the inferred service. When running via the `pytest` console script, sys.argv[0] is 'pytest' (skipped by detect_service), so the service is correctly inferred from the actual test file path. Fix: in the pytest plugin's pytest_load_initial_conftests, after the SessionManager is created, re-derive the service from sys.argv with __init__.py filtered out (matching the console-script behavior). If the corrected service differs from the session manager's service, update it via session.set_service(). This is scoped to the pytest plugin (not detect_service in ddtrace.internal.settings), so non-pytest service inference is unaffected. Resolves the snapshot test failures (service: 'pytest' vs expected 'tests.internal'/'tests.contrib.graphql') seen in the ddtest dogfooding CI. What: re-derive service in pytest plugin when argv[0] is __init__.py. Why: python -m pytest (ddtest) sets argv[0] to pytest's __init__.py; detect_service picks it up → wrong service. Fix in the plugin, not detect_service. E2E testing: both `pytest file.py` and `python -m pytest file.py` produce the module name (matching snapshots).
…se_service The previous fix updated session_manager.service but not BaseServiceProcessor._global_service, which was set from config.service at import time (before the plugin fix runs). The _dd.base_service meta tag is derived from _global_service, so snapshots still saw service='pytest' on _dd.base_service even though session.service was corrected. After correcting the service, also update BaseServiceProcessor._global_service in the tracer's span aggregator dd_processors list, so _dd.base_service matches the corrected service. What: fix BaseServiceProcessor._global_service after service correction. Why: _dd.base_service meta tag was still 'pytest' (from import-time config). E2E testing: _dd.base_service now matches the corrected service (tests.internal / tests.contrib.graphql).
Use list[t.Any] instead of t.List[t.Any] (PEP 585), remove unused type: ignore comment, fix import ordering. All ruff/mypy checks pass on plugin.py.
mypy error: Cannot call function of unknown type [operator] on _proc._global_service = ... because _proc is t.Any (from hasattr). Use setattr() instead of direct attribute access.
mypy error: Cannot call function of unknown type [operator] — schematize_service_name is a module-level variable from a dict lookup, mypy can't infer its type. Add type: ignore[operator] on the setattr line. Verified with scripts/lint typing.
The plan job was fanned out into N×M jobs (one per suite × one per
Python version), causing too much CI runner contention for a step
that's just discovery + Datadog API. Each RIOT_HASH already maps to
exactly one PYTHON_VERSION, so the Python version dimension was
redundant.
Reduce to one plan job per suite (N, not N×M). The plan job loops
over all RIOT_HASHES sequentially: prepare + plan each, partitioning
the plan artifact by hash. The run jobs stay per-hash (they need
parallel execution). Run jobs download the single plan artifact
(which contains all hashes' plans, partitioned by hash) and
restore their own hash's plan.
Generator: plan job has no parallel matrix, sets RIOT_HASHES
variable to a space-separated list of all hashes. Run job
keeps its parallel: matrix over RIOT_HASH × PYTHON_VERSION ×
CI_NODE_INDEX.
Template: .ddtest_plan loops over ${RIOT_HASHES}, artifacts
are .testoptimization-*/ (all hashes).
What: one plan job per suite (loop over hashes sequentially).
Why: N×M plan jobs caused too much runner contention.
E2E testing: internal has 1 plan job (was 12), 24 run jobs (same).
…ssor fix ddtrace.tracer is an attribute, not a module. Use 'import ddtrace as _ddtrace; _ddtrace.tracer._span_aggregator' instead of 'import ddtrace.tracer as _tracer'.
python -m pytest sets argv[0] to pytest/__main__.py (not __init__.py). The fix only checked for __init__.py, so it never triggered for python -m pytest. Add __main__.py to the filter.
…hon -m pytest ddtest hardcodes `python -m pytest` for RunTests and DiscoverTests, which sets sys.argv[0] to pytest's __main__.py. This causes detect_service to infer service='pytest' instead of the module name (e.g. 'tests.contrib.graphql'), breaking snapshot tests. Two changes: 1. ddtest (Go): PyTest.RunTests and DiscoverTests now use commandOverride when set (from --command or DD_TEST_OPTIMIZATION_RUNNER_COMMAND env var), instead of hardcoding `python -m pytest`. This lets the caller use `pytest` (the console script) which sets argv[0]='pytest' (skipped by detect_service → correct service inference). 2. gen_gitlab_config.py: set DD_TEST_OPTIMIZATION_RUNNER_COMMAND=pytest in ddtest job variables, so ddtest runs `pytest <files>` instead of `python -m pytest <files>`. 3. Revert the pytest plugin detect_service fix (ce0f0ca and follow-ups) — no longer needed since `pytest` (console script) sets argv[0]='pytest' which detect_service already handles. What: use pytest console script via DD_TEST_OPTIMIZATION_RUNNER_COMMAND. Why: python -m pytest sets argv[0] to __main__.py → wrong service. E2E testing: ddtest runs `pytest <files>` (argv[0]='pytest'), detect_service skips it → correct module name.
Until the commandOverride change is released, build ddtest from the gnufede/ddtest-dogfooding branch instead of downloading the released binary. This uses the Go toolchain already available in the testrunner image. The .ddtest_base before_script now: git clone --depth 1 --branch gnufede/ddtest-dogfooding https://github.com/DataDog/ddtest.git /tmp/ddtest-src (cd /tmp/ddtest-src && go build -o "${CI_PROJECT_DIR}/bin/ddtest" .) Once the ddtest PR is merged and a new release is published, revert to the curl download with DDTEST_VERSION. What: build ddtest from source branch in CI. Why: use the commandOverride change before it's released. E2E testing: ddtest is built from the branch and used for plan/run.
The testrunner image doesn't have Go installed, so install it from go.dev before building ddtest from the dogfooding branch. curl -fsSL https://go.dev/dl/go1.26.5.linux-amd64.tar.gz | tar xz -C /tmp export PATH="/tmp/go/bin:$PATH" git clone --depth 1 --branch gnufede/ddtest-dogfooding https://github.com/DataDog/ddtest.git /tmp/ddtest-src (cd /tmp/ddtest-src && go build -o "${CI_PROJECT_DIR}/bin/ddtest" .) This adds ~50s to each ddtest job (Go download + build) but lets us use the commandOverride change before it's released. What: install Go in CI to build ddtest from source. Why: testrunner image has no Go; can't build ddtest without it. E2E testing: ddtest is built from the branch with Go installed in-job.
The eval "$(scripts/ddtest-riot.py venv-env ...)" && python -c "..." was NOT a subshell — && chains commands in the same shell, so the exported PYTHONPATH (pointing at the suite venv) leaked into the before_script environment. Then riot run -s --pass-env wait inherited that PYTHONPATH and loaded typing_extensions from the wrong venv (e.g. molten venv while running in the wait venv). Fix: wrap in parentheses to create a real subshell: (eval "..." && python -c "...") The env vars are scoped to the subshell and don't leak. What: use (eval ...) subshell for venv activation in before_script. Why: && doesn't isolate env vars; PYTHONPATH leaked into wait-for-services. E2E testing: wait-for-services runs with a clean PYTHONPATH.
The PYTEST_ADDOPTS extracted from riot commands includes -n 8 --dist=worksteal (xdist worker count). During ddtest's plan phase (which runs pytest --collect-only), xdist tries to start workers and crashes with "Unexpectedly no active workers available". Fix: strip -n, --dist, and --numprocesses flags from PYTEST_ADDOPTS. These are for test execution (run), not collection (plan). ddtest's run phase uses --ci-node-workers for parallelism, not xdist. Verified: molten PYTEST_ADDOPTS is now empty (was "-n 8 --dist=worksteal"); internal is "-v" (was "-v -n auto --dist=worksteal"). What: strip xdist flags from PYTEST_ADDOPTS. Why: xdist during plan's --collect-only crashes with no active workers. E2E testing: plan phase runs without xdist workers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add an opt-in, manually-triggered CI path that runs the
internalriot suite through ddtest (plan + run + file splitting) instead of the legacyriot run <hash> -- --ddtracepath, without any changes to ddtest and without touching the generated-config pipeline.ddtest stays unchanged; all riot knowledge lives in a new Python bridge (
scripts/ddtest-riot.py) that uses riot as a library:internalsuite path is now a queryable env var (DDTEST_SUITE_PATH) instead of a literal baked into the pytest command, soriot run internalis unchanged with or without ddtest, and the bridge can read the suite location per venv.hashes <pattern>lists riot venvs;venv-env <hash>emits the env that activates a venv (VIRTUAL_ENV, PATH with venv bin first, PYTHONPATH site-packages, RIOT_*, DDTEST_SUITE_PATH), mirroringriot run --pass-env. Pure hint-based path math so it works without interpreters on PATH. Registered in run-script-doctests.py.ddtest-internal-pilotjob, gatedwhen: manual+allow_failure: true, that builds its own venv, downloads ddtest, activates the venv via the bridge, and runsddtest plan/run. Self-contained (no gen_gitlab_config.py wiring, no build_base_venvs dep).What: opt-in ddtest dogfood path for the
internalsuite. Why: validate ddtest plan/run + file splitting in real dd-trace-py CI as a first step toward replacing gitlab-level parallelization, keeping riot knowledge out of ddtest.E2E testing: trigger
ddtest-internal-pilotfrom the GitLab UI on a branch; it runsriot generate --python=3.10, downloads ddtest, activates theinternalPy3.10 venv viascripts/ddtest-riot.py venv-env 4f70b3c, runsddtest plan -p python -f pytest --tests-location tests/internal --min-parallelism 1 --max-parallelism 1, thenddtest run --ci-node 0 --ci-node-workers ncpu, and asserts ddtrace/pytest import in the venv.Description
Testing
Risks
Additional Notes