ci(e2e): shard the metrics lane into per-domain matrix jobs - #1948
ci(e2e): shard the metrics lane into per-domain matrix jobs#1948mitasovr wants to merge 2 commits into
Conversation
The e2e-metrics CI lane ran all 35 metric fixtures in one serial pytest session (~10 min). Fan it out into one matrix job per metric domain — the *.test.yaml file-name prefix up to the first underscore (ai, collab, git, tasks, wiki, ...) — so domains run in parallel, each against its own CH+MariaDB+analytics stack, halving the lane's wall-clock and isolating failures per domain. - conftest: new pytest option `--yaml-mask GLOB` narrows yaml-rig collection by fnmatch on the fixture file name; a mask that matches nothing is a hard UsageError so a misconfigured shard cannot go green while testing nothing. Within a shard tests stay serial (the session rig is not xdist-safe). - workflow: the `build` job derives the shard list dynamically from the checked-out file names (a new domain shards itself automatically) and guards that every fixture lands in exactly one shard — a file no mask would pick up fails the build in seconds instead of silently never running in CI. - each shard uploads coverage-inputs-metrics-<domain>; the metric coverage gate downloads them by pattern with merge-multiple. That is sound because metric_definitions.json (the builtin registry snapshot from MariaDB) is identical in every shard, and coverage itself is computed statically from ALL metrics/*.test.yaml in the checkout, so sharding cannot hide a fixture from the gate. Branch protection keeps requiring only the umbrella "Run E2E suite" check — it aggregates the whole matrix; the per-domain "metrics (<d>)" contexts change as domains land and must not be required directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
|
📝 WalkthroughWalkthroughThe E2E workflow derives metric domains, runs each domain in a separate matrix job with filtered pytest collection, uploads per-domain coverage artifacts, and merges those artifacts in the coverage gate. Local usage documentation now includes domain filtering examples. ChangesMetrics sharding and coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Build
participant e2e-metrics
participant Pytest
participant metric-coverage-gate
Build->>e2e-metrics: Pass metric-shards matrix
e2e-metrics->>Pytest: Run --yaml-mask domain_*
Pytest-->>e2e-metrics: Generate domain coverage
e2e-metrics->>metric-coverage-gate: Upload coverage-inputs-metrics-domain
metric-coverage-gate->>metric-coverage-gate: Merge coverage-inputs-metrics-*
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/e2e-bronze-to-api.yml:
- Around line 103-109: Update the fixture discovery in the inline Python script
to recursively collect all metrics/**/*.test.yaml files, matching
discover_tests(). Ensure domain extraction, shard matrix generation, and the
bad-name partition check use the complete recursive fixture set.
- Around line 485-486: Update the E2E suite metrics step to construct the mask
through an environment variable derived from matrix.domain, optionally
restricting it to safe file-name characters, and pass it to e2e.sh as the
double-quoted "${YAML_MASK}" rather than embedding matrix.domain in a
single-quoted shell argument.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b5f6ccb8-7d76-4110-8f08-d195d21c6e11
📒 Files selected for processing (4)
.github/workflows/e2e-bronze-to-api.ymlsrc/ingestion/tests/e2e/README.mdsrc/ingestion/tests/e2e/conftest.pysrc/ingestion/tests/e2e/e2e.sh
| python3 - <<'EOF' >> "$GITHUB_OUTPUT" | ||
| import fnmatch, json, pathlib, sys | ||
| names = sorted(p.name for p in pathlib.Path("metrics").glob("*.test.yaml")) | ||
| if not names: | ||
| sys.exit("no metrics/*.test.yaml found — wrong working directory?") | ||
| domains = sorted({n.split("_", 1)[0] for n in names}) | ||
| bad = [n for n in names if sum(fnmatch.fnmatch(n, f"{d}_*") for d in domains) != 1] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use recursive fixture discovery for shard derivation.
discover_tests() collects metrics/**/*.test.yaml, but this uses glob(). A domain that exists only in a subdirectory will not enter the matrix or partition check, so its fixtures can be skipped while the coverage gate still sees them.
Proposed fix
- names = sorted(p.name for p in pathlib.Path("metrics").glob("*.test.yaml"))
+ names = sorted(p.name for p in pathlib.Path("metrics").rglob("*.test.yaml"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| python3 - <<'EOF' >> "$GITHUB_OUTPUT" | |
| import fnmatch, json, pathlib, sys | |
| names = sorted(p.name for p in pathlib.Path("metrics").glob("*.test.yaml")) | |
| if not names: | |
| sys.exit("no metrics/*.test.yaml found — wrong working directory?") | |
| domains = sorted({n.split("_", 1)[0] for n in names}) | |
| bad = [n for n in names if sum(fnmatch.fnmatch(n, f"{d}_*") for d in domains) != 1] | |
| python3 - <<'EOF' >> "$GITHUB_OUTPUT" | |
| import fnmatch, json, pathlib, sys | |
| names = sorted(p.name for p in pathlib.Path("metrics").rglob("*.test.yaml")) | |
| if not names: | |
| sys.exit("no metrics/*.test.yaml found — wrong working directory?") | |
| domains = sorted({n.split("_", 1)[0] for n in names}) | |
| bad = [n for n in names if sum(fnmatch.fnmatch(n, f"{d}_*") for d in domains) != 1] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/e2e-bronze-to-api.yml around lines 103 - 109, Update the
fixture discovery in the inline Python script to recursively collect all
metrics/**/*.test.yaml files, matching discover_tests(). Ensure domain
extraction, shard matrix generation, and the bad-name partition check use the
complete recursive fixture set.
| - name: E2E suite — metrics (${{ matrix.domain }}) | ||
| run: ./e2e.sh test metrics/ --yaml-mask '${{ matrix.domain }}_*' --tb=short -q |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repo files matching workflow:"
git ls-files | grep -F '.github/workflows/e2e-bronze-to-api.yml' || true
if [ -f .github/workflows/e2e-bronze-to-api.yml ]; then
echo "--- lines 440-505 ---"
sed -n '440,505p' .github/workflows/e2e-bronze-to-api.yml | nl -ba -v440
fi
echo "--- occurrences of matrix.domain / yaml-mask ---"
rg -n "matrix\.domain|yaml-mask|yaml_mask|YAML_MASK|metrics/" .github/workflows/e2e-bronze-to-api.yml . 2>/dev/null | head -200
echo "--- e2e.sh signature and yaml-mask handling ---"
if [ -f e2e.sh ]; then
ast-grep outline e2e.sh --match 'yaml' --view expanded || true
rg -n "yaml.mask|yaml_mask|yaml-mask|argparse|parse|mask" e2e.sh .github -S || true
sed -n '1,220p' e2e.sh
fiRepository: constructorfabric/insight
Length of output: 295
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- lines 440-505 ---"
awk 'NR>=440 && NR<=505 {printf "%4d\t%s\n", NR, $0}' .github/workflows/e2e-bronze-to-api.yml
echo "--- occurrences of matrix.domain / yaml-mask ---"
rg -n "matrix\.domain|yaml-mask|yaml_mask|YAML_MASK|metrics/" .github/workflows/e2e-bronze-to-api.yml .github e2e.sh 2>/dev/null || true
echo "--- e2e.sh yaml-mask handling ---"
if [ -f e2e.sh ]; then
rg -n "yaml.mask|yaml_mask|yaml-mask|argos|argparse|parse|mask|subprocess|eval|evalf|system" e2e.sh .github -S || true
awk '{printf "%4d\t%s\n", NR, $0}' e2e.sh | sed -n '1,180p'
fiRepository: constructorfabric/insight
Length of output: 6568
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- workflow matrix discovery and jobs ---"
awk 'NR>=1 && NR<=135 {printf "%4d\t%s\n", NR, $0}' .github/workflows/e2e-bronze-to-api.yml
awk 'NR>=380 && NR<=425 {printf "%4d\t%s\n", NR, $0}' .github/workflows/e2e-bronze-to-api.yml
awk 'NR>=540 && NR<=575 {printf "%4d\t%s\n", NR, $0}' .github/workflows/e2e-bronze-to-api.yml
echo "--- existing files named e2e.sh or equivalent ---"
find . -maxdepth 4 -type f \( -iname 'e2e.sh' -o -name 'e2e*' \) -print 2>/dev/null | sort
echo "--- relevant shell source snippets if present ---"
for f in $(git ls-files | grep -E '(^|/)e2e(\.sh)?$' + || true); do
[ -z "$f" ] && continue
echo "== $f =="
sed -n '1,80p' "$f"
done
echo "--- run block structure around matrix usage ---"
awk 'NR>=486 && NR<490 {printf "%4d\t%s\n", NR, $0}' .github/workflows/e2e-bronze-to-api.ymlRepository: constructorfabric/insight
Length of output: 11902
🌐 Web query:
GitHub Actions workflow yml shell run syntax env output run after run
💡 Result:
To share data between steps in a GitHub Actions job, you should use environment files instead of the deprecated set-output command [1][2][3]. GitHub provides two primary mechanisms for this, depending on your needs: 1. Step Outputs (GITHUB_OUTPUT) Use this method for structured data that you need to access via step IDs, use in conditionals, or pass to subsequent jobs [1][2]. Define the output by appending a key-value pair to the file path located at the $GITHUB_OUTPUT environment variable [4][5]. The step must have an id defined to reference the output later [6][4]. Example: - name: Set output id: my-step run: echo "my_key=my_value" >> "$GITHUB_OUTPUT" - name: Use output run: echo "The value is ${{ steps.my-step.outputs.my_key }}" 2. Environment Variables (GITHUB_ENV) Use this method to make variables available to the shell environment of all subsequent steps in the same job [1][5]. Define the variable by appending a key-value pair to the file path located at the $GITHUB_ENV environment variable [5][7]. The variable is then available as a standard environment variable in following steps [5][7]. Example: - name: Set env var run: echo "MY_VAR=my_value" >> "$GITHUB_ENV" - name: Use env var run: echo "The value is $MY_VAR" Why direct export doesn't work: GitHub Actions executes each step in an isolated process [7]. Because steps are sibling processes rather than parent-child processes, standard shell commands like export do not persist variables between steps; the variables disappear when the step process terminates [7]. Writing to GITHUB_OUTPUT or GITHUB_ENV allows the runner to capture these values and inject them into future steps [2][7]. Note on deprecation: The legacy command echo "::set-output name={name}::{value}" is deprecated [3][8][9]. You should always use the environment file syntax (>> $GITHUB_OUTPUT) instead [2][3][9].
Citations:
- 1: https://timesofcloud.com/github-actions/step-outputs/
- 2: https://www.codegenes.net/blog/github-actions-how-to-share-a-calculated-value-between-job-steps/
- 3: https://github.blog/changelog/2022-10-10-github-actions-deprecating-save-state-and-set-output-commands/
- 4: https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-commands
- 5: https://docs.github.com/en/enterprise-cloud@latest/actions/reference/workflows-and-actions/workflow-commands
- 6: https://stackoverflow.com/questions/57819539/how-to-share-a-calculated-value-between-job-steps
- 7: https://www.kenmuse.com/blog/exporting-environment-variables-fails-in-github-actions/
- 8: https://github.blog/changelog/2023-07-24-github-actions-update-on-save-state-and-set-output-commands/
- 9: https://hynek.me/til/set-output-deprecation-github-actions/
🌐 Web query:
GitHub Actions pull_request files changed workflow_dispatch can trigger workflow file changes
💡 Result:
GitHub Actions workflows are triggered by specific events, and the triggers function as follows: Pull Request Trigger: The pull_request event triggers a workflow when a pull request is opened, synchronized (new commits pushed), or reopened [1]. To determine which files have changed within a pull request, you can use git commands (e.g., git diff) [1] or specialized third-party actions like tj-actions/changed-files [2][3]. These tools compare the head of the pull request branch against the base branch to identify modified files [1][3]. Workflow Dispatch Trigger: The workflow_dispatch event allows you to trigger a workflow manually via the GitHub UI, the GitHub CLI, or the REST API [4][5]. To use this trigger, the workflow file must exist on the default branch of the repository [6][4]. If you want to test a workflow using workflow_dispatch from a feature branch, you must ensure the workflow file exists on the default branch [6]. Developers sometimes add both pull_request and workflow_dispatch triggers to the same workflow file to allow for both automated PR-based execution and manual testing [5]. Triggering Workflow File Changes: GitHub Actions workflows do not automatically trigger themselves simply because a workflow file is changed in a repository; rather, changes to workflow files in the default branch are registered by GitHub so they can be executed by future events [5]. If a workflow is not triggering as expected after a change, it may need to be re-registered by merging the changes into the default branch [5]. Events triggered by the GITHUB_TOKEN during a workflow run generally do not trigger new workflow runs to prevent infinite loops, though workflow_dispatch and repository_dispatch events are exceptions that always create new runs [6].
Citations:
- 1: https://www.baeldung.com/ops/github-actions-pr-files-modified
- 2: https://github.com/tj-actions/changed-files?tab=readme-ov-file
- 3: https://github.com/marketplace/actions/changed-files
- 4: https://docs.github.com/en/actions/how-tos/manage-workflow-runs/manually-run-a-workflow
- 5: https://stackoverflow.com/questions/70151645/how-can-i-trigger-a-workflow-dispatch-from-the-github-api
- 6: https://docs.github.com/en/actions/how-tos/write-workflows/choose-when-workflows-run/trigger-a-workflow
🌐 Web query:
GitHub Actions expression ${{ matrix.domain }} context pull_request head_ref branch protection status checks
💡 Result:
In GitHub Actions, the matrix context and pull request references are distinct components of a workflow, and they interact according to the following principles: Context Availability The matrix context contains the specific configuration values defined for a current job [1][2]. You cannot directly access pull request information like github.head_ref within the strategy section of a job definition because the strategy (matrix) is evaluated before the job is sent to a runner [3][4]. If you need to dynamically define a matrix based on pull request data, you must perform that logic in a separate "setup" job and output the matrix definition as a JSON string, then consume it using the fromJSON function in the dependent job's strategy [5][6]. Pull Request Head Reference The github.head_ref context provides the source branch of a pull request and is only available when the workflow is triggered by the pull_request or pull_request_target events [1][2]. It can be used in job-level if conditionals to determine whether a job should execute for a specific branch (e.g., if: ${{ github.head_ref == 'main' }}) [7]. Branch Protection and Status Checks Required status checks are a branch protection feature that ensures specific jobs complete successfully before a pull request can be merged [8][9]. To use a job as a required status check: 1. The job must be triggered on the pull_request event [10]. 2. The job name must be unique across all workflows in the repository to prevent ambiguous status results [8]. 3. For a new status check to appear in the branch protection settings list, the workflow must have successfully run at least once in the repository within the last seven days [11][10]. 4. If your workflow uses a matrix, each combination in the matrix produces a separate status check [1][2]. You should ensure that the final check name is deterministic, as branch protection requires identifying specific status check names [8].
Citations:
- 1: https://docs.github.com/en/actions/reference/workflows-and-actions/contexts
- 2: https://github.com/github/docs/blob/main/content/actions/reference/workflows-and-actions/contexts.md
- 3: https://docs.github.com/en/actions/reference/workflows-and-actions/variables
- 4: https://github.com/github/docs/blob/main/content/actions/reference/workflows-and-actions/variables.md
- 5: https://docs.github.com/en/actions/how-tos/write-workflows/choose-what-workflows-do/run-job-variations
- 6: https://github.com/quipper/monorepo-deploy-actions/blob/82b24beb540286a3d914e4bd300ef326b51866cb/.github/workflows/environment-matrix.yaml
- 7: https://raw.githubusercontent.com/github/docs/refs/heads/main/content/actions/reference/workflows-and-actions/events-that-trigger-workflows.md
- 8: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches?require-status-checks-before-merging=
- 9: https://docs.github.com/en/pull-requests/reference/status-checks
- 10: https://stackoverflow.com/questions/68554735/github-action-status-check-missing-from-the-list-of-checks-in-protected-branch-s
- 11: https://docs.github.com/en/pull-requests/how-tos/merge-and-close-pull-requests/troubleshooting-required-status-checks
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- shell quote test for workflow interpolation ---"
python3 - <<'PY'
import shlex
domains = ["safe", "bad'];malicious=true", "backslash'command", "normal"]
template = "./e2e.sh test metrics/ --yaml-mask '${domain}_*' --tb=short -q"
print("template:", template)
for d in domains:
command = template.format(domain=d)
print("\ninput:", d)
print("shell:", command)
try:
print("tokens:", shlex.split(command))
except Exception as e:
print("shlex split error:", type(e).__name__, ":", e)
PY
echo "--- workflow matrix outputs exact values if a malformed quote existed ---"
python3 - <<'PY'
import json
print(json.dumps(['safe', "bad');malicious=true", "backslash'command", "normal"]))
PYRepository: constructorfabric/insight
Length of output: 1086
Do not pass fixture-derived domain masks into shell source.
matrix.domain comes from generated metric shard names, so any ' in those names breaks the single-quoted --yaml-mask argument. Pass the mask via env: and reference it as "${YAML_MASK}"; narrowing discovery to allowed file-name characters is also useful.
Proposed fix
- name: E2E suite — metrics (${{ matrix.domain }})
- run: ./e2e.sh test metrics/ --yaml-mask '${{ matrix.domain }}_*' --tb=short -q
+ env:
+ YAML_MASK: ${{ matrix.domain }}_*
+ run: ./e2e.sh test metrics/ --yaml-mask "$YAML_MASK" --tb=short -q📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: E2E suite — metrics (${{ matrix.domain }}) | |
| run: ./e2e.sh test metrics/ --yaml-mask '${{ matrix.domain }}_*' --tb=short -q | |
| - name: E2E suite — metrics (${{ matrix.domain }}) | |
| env: | |
| YAML_MASK: ${{ matrix.domain }}_* | |
| run: ./e2e.sh test metrics/ --yaml-mask "$YAML_MASK" --tb=short -q |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 486-486: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/e2e-bronze-to-api.yml around lines 485 - 486, Update the
E2E suite metrics step to construct the mask through an environment variable
derived from matrix.domain, optionally restricting it to safe file-name
characters, and pass it to e2e.sh as the double-quoted "${YAML_MASK}" rather
than embedding matrix.domain in a single-quoted shell argument.
Source: Linters/SAST tools
1 participant
What
The
e2e-metricsCI lane ran all 35 metric fixtures in one serial pytest session (~10 min wall-clock). This PR fans it out into one matrix job per metric domain — the*.test.yamlfile-name prefix up to the first_(ai,collab,git,tasks,wiki, …) — so domains run in parallel, each against its own CH + MariaDB + analytics stack.How
--yaml-mask GLOB(new pytest option inconftest.py): narrows yaml-rig collection by fnmatch on the fixture file name, e.g../e2e.sh test metrics/ --yaml-mask 'tasks_*'. A mask that matches nothing raises a hardUsageError(exit 2) — a misconfigured shard cannot go green while testing nothing. Within a shard tests stay serial (the session rig is not xdist-safe).buildjob derives the domain list from the checked-out file names, so a new domain (e.g.crm_*) gets its own shard the moment its first fixture lands — no workflow edit. A partition guard asserts every fixture matches exactly one shard mask; a file no shard would run failsbuildin seconds instead of silently never running in CI.coverage-inputs-metrics-<domain>; the metric-coverage-gate downloads them by pattern withmerge-multiple. Sound becausemetric_definitions.json(the builtin registry snapshot from MariaDB) is identical in every shard, and coverage itself is computed statically from ALLmetrics/*.test.yamlin the checkout — sharding cannot hide a fixture from the gate.fail-fast: false— a redtasksshard doesn't cancelcollab.Branch protection
No change needed: the umbrella "Run E2E suite" check aggregates the whole matrix (
needs.e2e-metrics.resultis the matrix aggregate). The per-domainmetrics (<domain>)contexts should NOT be required directly — the domain set changes as new domains land.Expected effect
Old metrics lane ≈ 10.3 min, of which ~3 min is per-session fixed cost (image load + stack boot + migrations + gold build). The largest shard (
tasks, 12 fixtures incl. jira enrich) should land around 5–6 min, roughly halving the post-buildcritical path, with per-domain failure isolation as a bonus.Verification
--yaml-mask tasks_closed) select 1 test.pytest.UsageError, exit 2.["ai","collab","git","tasks","wiki"]; partition guard verified.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Bug Fixes