ci(perf): upload trace-only artifact on perf-probe failure - #727
Merged
Conversation
Add perf-probe-trace-{attempt} artifact containing only Playwright trace.zip
files from the diagnostic re-run, plus a step summary and ::notice pointing
PR authors at it. The existing perf-probe-baseline artifact is unchanged.
The 90 MB perf-probe-baseline bundle is impractical to download from China
(~40 KB/s sustained, both via mihomo and direct to Azure blob), so authors
cannot inspect traces when a regression fires. A trace-only split brings the
typical download to a few minutes and gives the on-failure pointer a single
named target.
Refs #698
|
Note Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported. |
Astro-Han
added
the
task
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds an env-driven perf scenario filter, a CLI to list failing scenarios, and CI changes that gate trace captures on failing scenarios and upload Playwright trace artifacts with run-summary instructions when perf comparisons fail. ChangesPerf probe trace artifact capture and documentation
Sequence Diagram(s)sequenceDiagram
participant GitHub_Actions
participant list_fail_script
participant TraceCaptureJob
participant ArtifactStore
GitHub_Actions->>list_fail_script: run list-failing-scenarios on perf-compare.json
list_fail_script-->>GitHub_Actions: outputs default=<csv>, low_end=<csv>
GitHub_Actions->>TraceCaptureJob: conditionally run trace captures (set PAWWORK_PERF_SCENARIOS)
TraceCaptureJob->>ArtifactStore: upload trace.zip from base/head dirs
ArtifactStore-->>GitHub_Actions: artifact URL
GitHub_Actions->>GitHub_Actions: append Perf trace artifact to $GITHUB_STEP_SUMMARY
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Suggested priority: P3 (only low-risk paths changed (.github/workflows/perf-probe-baseline.yml)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
Astro-Han
added
tech-debt
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/perf-probe-baseline.yml (1)
334-334: Consider updating to actions/upload-artifact@v7.0.1.The SHA
043fb46d1a93c77aae656e7c1c64a875d1fc6a0acorrectly matches the v7 tag. A newer patch version (v7.0.1) is available, released on 2026-04-10. If you want to stay current with the latest patch, update the action to the v7.0.1 release.🤖 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/perf-probe-baseline.yml at line 334, Update the GitHub Actions step that references actions/upload-artifact by replacing the pinned SHA "actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a" with the newer patch tag "actions/upload-artifact@v7.0.1" (the step using the actions/upload-artifact reference should be updated to use `@v7.0.1` instead of the SHA).
🤖 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.
Nitpick comments:
In @.github/workflows/perf-probe-baseline.yml:
- Line 334: Update the GitHub Actions step that references
actions/upload-artifact by replacing the pinned SHA
"actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a" with the
newer patch tag "actions/upload-artifact@v7.0.1" (the step using the
actions/upload-artifact reference should be updated to use `@v7.0.1` instead of
the SHA).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 92f44a6b-0328-4c04-8ec5-01ee959e6adf
📒 Files selected for processing (1)
.github/workflows/perf-probe-baseline.yml
Perf delta summaryComparator: pass
|
github-actions
Bot
added
app
Astro-Han
force-pushed
the
claude/perf-trace-artifact-i698
branch
from
May 18, 2026 08:01
1511c50 to
d4b34c5
Compare
github-actions
Bot
removed
app
Add a PAWWORK_PERF_SCENARIOS env filter on top of the existing profile gate in shouldRunScenario, and resolve the failing scenario list from perf-compare-confirm.json before the diagnostic trace steps. Each of the four Capture trace steps now passes only its profile's failing scenarios to the spec, and skips entirely when that profile has no failure. Today the four trace steps re-run the full perf spec (all scenarios in the profile), even when a regression touched only one scenario. Measured on a forced-fail run, baseline + confirm + trace together cost ~34 min on the 30-min job ceiling, so any real perf regression risks being cut off before the trace artifact is produced. Filtering trace to the failing scenarios brings the worst-case fail path to ~22-26 min, leaving headroom for setup and future scenario growth. Confirm steps still run the whole profile to preserve the workflow's false-positive guard: a scenario must fail twice across two independent runs before its trace is recorded. Refs #698
github-actions
Bot
added
app
Astro-Han
force-pushed
the
claude/perf-trace-artifact-i698
branch
from
May 18, 2026 08:37
3060d1c to
cc91973
Compare
Address crosscheck findings on the perf trace artifact pipeline: - Gate Upload perf trace artifact and Print perf trace artifact pointer on steps.failing_scenarios.outputs being non-empty, so a top-level missing_* comparator failure (or any other path where no per-scenario regression is detected) no longer prints a pointer to an empty artifact. - Wrap both gates with always() so a trace capture step that exits non-zero partway still uploads whatever trace.zip files were produced before the failure, instead of being skipped by the default cancel-on-prior-failure behavior. - Have list-failing-scenarios.ts try/catch the JSON parse and write directly to process.env.GITHUB_OUTPUT instead of relying on shell stdout redirection, so stray script output or a malformed input cannot silently corrupt the step outputs that downstream conditions depend on. A ::warning:: surfaces the parse failure for the run author. - Add packages/app/script/list-failing-scenarios.ts to the workflow paths filter so future edits to the selector script actually trigger this workflow. Refs #698
…hanges Add packages/app/script/list-failing-scenarios.ts to is_low_end_path() so a PR that touches only this selector script still exercises the low_end output contract that the script writes, mirroring how compare-perf.ts and merge-perf-artifacts.ts already gate the low-end profile. Refs #698
Astro-Han
added a commit
that referenced
this pull request
May 18, 2026
Astro-Han
added a commit
that referenced
this pull request
May 18, 2026
This was referenced May 18, 2026
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.
Summary
When
perf-probe-baselinefails the comparator twice, upload a separate small artifactperf-probe-trace-{attempt}containing only Playwrighttrace.zipfiles from the diagnostic re-run, and re-run only the scenarios that actually regressed for trace capture. The on-failure pointer is gated on at least one scenario actually failing.The existing
perf-probe-baseline-{attempt}bundle is unchanged for callers who want the full report.Why
Closes #698.
The original scope (just upload + pointer) is necessary but not sufficient. Investigation found two structural problems on the fail path:
The trace files were already inside the existing
perf-probe-baselineartifact (it bundlese2e/test-results/**), but that bundle is ~90 MB because it also carries the HTML report, videos, and merged perf JSON for both base and head. Measured sustained download from China is about 40 KB/s — the same via mihomo or direct toproductionresultssa17.blob.core.windows.net— which puts the full bundle at ~40 minutes per regression. The trace files are effectively unreachable.The fail path runs the perf spec 12 times (baseline ×4 + confirm ×4 + diagnostic trace ×4). Measured per-step durations on a forced-fail run put the total at ~34 min against the 30-min job ceiling. Any real perf regression risks being cut off before the trace step's artifact is uploaded, leaving the trace artifact from (1) just as unreachable in practice.
This PR addresses both:
::noticepointing at it.perf-compare-confirm.json). Worst-case fail path drops to ~22-26 min, leaving headroom for setup and future scenario growth.Related Issue
Closes #698
Human Review Status
Pending.
Review Focus
PAWWORK_PERF_SCENARIOSenv inprofiles.tsis now AND-ed with the existing profile gate; absent/empty env means "no extra filter", which preserves current behavior.list-failing-scenarios.tswritesdefault=...andlow_end=...directly to$GITHUB_OUTPUTand tolerates malformed input via try/catch +::warning::(so a parse failure cannot silently corrupt step outputs that downstream conditions depend on).always() && ...both compares failed... && at least one profile has failing scenarios. Thealways()keeps partial traces uploadable if a capture step crashed mid-way; the scenario-non-empty check prevents the pointer from advertising an empty artifact when the comparator failed only with top-levelmissing_*.Risk Notes
if-no-files-found: warnis kept (consistent with the surrounding workflow); trace-step crashes still surface via the step's own exit code.How To Verify
Crosscheck
Multi-model review (Claude opus + Codex high) flagged 13 findings; this PR applies the agreed P1/P2 fixes and rejects the P3 / hypothetical ones:
Screenshots or Recordings
N/A — CI workflow change only.
Checklist
bug,enhancement,task, ordocumentation), at least one primary routing label (app,ui,platform,harness, orci), and exactly one priority label (P0toP3), or I requested maintainer labelingdev, and my PR title and commit messages use Conventional Commits in English