Exchange test statistics through S3#11579
Conversation
|
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
bric3
requested review from
AlexeyKuznetsov-DD and
erikayasuda
and removed request for
a team
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
![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: 1cb05e35a6
ℹ️ 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".
| - .gitlab/count_tests.sh "$GRADLE_TARGET" "$testJvm" "./results" "./test_counts_${CI_JOB_ID}.json" | ||
| - export TEST_COUNTS_S3_PREFIX="test-counts/${CI_PIPELINE_ID}" | ||
| - export TEST_COUNTS_FILE="./test_counts_${CI_JOB_ID}.json" | ||
| - export TEST_COUNTS_S3_URI="s3://${TEST_COUNTS_S3_BUCKET}/${TEST_COUNTS_S3_PREFIX}/test_counts_${CI_JOB_ID}.json" |
There was a problem hiding this comment.
When a test job is retried manually or by the template's retry: max: 2, the retry stays in the same CI_PIPELINE_ID but gets a new CI_JOB_ID, so this key leaves the first attempt's test_counts_<old job id>.json in the same S3 prefix while the retry uploads another file. The aggregate job then downloads every test_counts_*.json from that prefix, which double-counts tests and can preserve stale failed/zero-test attempts in the summary; use a stable per-job key that is overwritten by retries or filter to the latest attempt before aggregation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Nice finding... BTW
@bric3 WDYT? maybe we should use pipelineId + test name?
like: 1223456789_test_base: [17, 1/4] ?
There was a problem hiding this comment.
YUp I saw that, I made a follow-up PR: #11580
bric3
changed the title
AlexeyKuznetsov-DD
left a comment
AlexeyKuznetsov-DD
left a comment
There was a problem hiding this comment.
LGTM. The only question I have - is there any cleanup needed for S3?
- Set some expiration date?
- Or just delete after aggregation?
- Cron job to cleanup every month?
- ...
- PROFIT :)
|
@AlexeyKuznetsov-DD none of the above the bucket have a 3 day expiration (or at least 1 is the closest) |
bric3
added
tag: no release notes
|
/merge -f --reason="Do not impact shipped code" |
|
View all feedbacks in Devflow UI.
The expected merge time in
Warning This change was merged without running any pre merge CI checks Reason: Do not impact shipped code |
gh-worker-dd-mergequeue-cf854d
Bot
merged commit 61cb4e0
into
master
3 participants
What Does This Do
Uploads each CI test job's test statistics JSON report to S3 under a pipeline-specific prefix, then has the aggregate job download those files before building the summary report.
This replaces the previous mechanism relying on GitLab artifacts, that was performing a sequential download of every test job artifacts content (including things useless here). And it re-enables test count aggregation for branch pipelines.
And a PR branch the job should now take ~52 seconds (with ~50 seconds setting up container), while it was ~10 minutes (on merge queue it could be longer)
Motivation
The aggregate job needs test count files from many test jobs, but relying on GitLab's default artifact download is very long, in merge queue it can take as long as 20 minutes, also this job is on the critical path. Using S3 keeps the aggregate input explicit and lets branch pipelines produce the same test count summary.
Additional Notes
Follow-up
Jira ticket: [PROJ-IDENT]