feat(bench): add run attribution to ClickHouse uploads#3347
Conversation
Adds run_label, pr_number, baseline/feature refs, triggered_by, run_type, and github_run_id/url to tempo_bench_runs inserts. Includes migration SQL for the new columns. Closes RETH-669 Amp-Thread-ID: https://ampcode.com/threads/T-019d2df7-e93f-71eb-a278-4de1939ddf17
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5eea85420c
ℹ️ 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".
| BENCH_BASELINE_REF: ${{ steps.refs.outputs.baseline-ref }} | ||
| BENCH_FEATURE_REF: ${{ steps.refs.outputs.feature-ref }} | ||
| BENCH_RUN_TYPE: ${{ github.event_name == 'schedule' && 'nightly' || github.event_name == 'issue_comment' && 'pr_comment' || 'manual' }} | ||
| BENCH_JOB_URL: ${{ env.BENCH_JOB_URL }} |
There was a problem hiding this comment.
Stop overwriting BENCH_JOB_URL with empty context
In the Upload results to ClickHouse step, setting BENCH_JOB_URL: ${{ env.BENCH_JOB_URL }} can blank out the URL exported earlier via core.exportVariable('BENCH_JOB_URL', jobUrl). The env expression context only includes workflow/job/step-declared env vars, not runtime runner env, so this expression resolves to an empty string and overrides the real value for this step. As a result, upload-clickhouse.sh inserts empty github_run_url values, defeating the new run attribution field.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds benchmark run attribution metadata to ClickHouse uploads so tempo_bench_runs rows can be linked back to the triggering PR/workflow run and compared across baseline/feature refs.
Changes:
- Extend
tempo_bench_runsinserts with run attribution fields (label, PR number, baseline/feature refs, trigger info, GitHub run id/url). - Add a ClickHouse migration to introduce the new columns.
- Populate baseline/feature refs and run type in the bench GitHub Actions workflow.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
contrib/bench/upload-clickhouse.sh |
Adds new attribution columns/values to the ClickHouse insert generation. |
contrib/bench/migrations/001_add_run_attribution.sql |
Adds ClickHouse ALTER TABLE statements for the new attribution columns. |
.github/workflows/bench.yml |
Exposes baseline/feature refs + run type (and passes job URL) to the upload step. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Use an explicit manual/nightly run type for benchmark uploads so PR-comment runs are recorded as manual while dispatched nightlies can be tagged explicitly. Fall back to the workflow run URL when a job URL is not exported so manual workflow_dispatch uploads still retain GitHub attribution. Co-authored-by: YK <46377366+yongkangc@users.noreply.github.com> Amp-Thread-ID: https://ampcode.com/threads/T-019d3de6-3008-71ef-934d-cac5531a5f8e
Resolve workflow_dispatch PR attribution from the benchmarked feature commit when the workflow branch itself does not map to the PR. Co-authored-by: YK <46377366+yongkangc@users.noreply.github.com> Amp-Thread-ID: https://ampcode.com/threads/T-019d3e31-e63a-7608-9f69-b46c03444ac6
Closes RETH-669
Adds run_label, pr_number, baseline/feature refs, triggered_by, run_type,
and github_run_id/url to tempo_bench_runs inserts. Includes migration
SQL for the new columns.
Added in these on clickhouse: