feat(ci): record merge-queue lane assignment as telemetry - #76593
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
Reviews (1): Last reviewed commit: "feat(ci): record merge-queue lane assign..." | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
This PR adds non-gating telemetry to the Trunk merge-queue impacted-targets workflow so PostHog can continuously observe how lane assignment behaves (lane “cost”), without uploading raw file paths.
Changes:
- Extend
.github/workflows/trunk-impacted-targets.ymlto compute a lane-assignment summary in the untrustedcomputejob and POST it as atrunk_lane_targetsevent from the trusteduploadjob. - Add a new Node script (
trunk-lane-telemetry.js) that summarizes changed files (top-dir histogram, product list) and target sets, including awidening_reason. - Add unit tests for the summarizer (
trunk-lane-telemetry.test.js).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| .github/workflows/trunk-impacted-targets.yml | Adds lane-summary output from compute and emits PostHog telemetry from upload while preserving the trust boundary. |
| .github/scripts/trunk-lane-telemetry.js | Implements summarization of changed files + impacted targets into a compact event property bag. |
| .github/scripts/trunk-lane-telemetry.test.js | Adds Node test coverage for widening attribution, ALL handling, and path summarization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7302038 to
56bdccc
Compare
Summarizes each PR's change set and the targets uploaded to Trunk, so lane cost can be tracked continuously instead of replayed from git history. Emitted from the upload job, which already holds credentials and never checks out the repo; the compute job runs PR-controlled code and stays free of them. The summary crosses that boundary as a JSON string, consumed through env: and jq --argjson. Sends counts, a directory histogram, and the target set rather than raw paths, plus which rule widened a PR to ALL. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The write-only key is committed so the workflow also runs in forks, where PR numbers restart at 1. A bare pr-<number> made a fork's PR share a person with ours, which is the grain person-level analysis keys on. The repo was already a property, so only the identity was ambiguous. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ff20198 to
22af026
Compare
|
This pull request was merged into |
Problem
We tune the merge-queue lane rules by replaying
git logthrough the target script by hand. That works for a one-off question but gives no continuous signal: no baseline, no regression alert, and no way to notice that a newly added top-level directory is silently forcing every PR that touches it into a single lane.Changes
Each PR's lane assignment is now summarized and sent to the
trunk_lane_targetsevent.{ "changed_file_count": 1, "changed_top_dirs": { ".github": 1 }, "changed_products": [], "is_all": true, "target_count": 0, "targets": [], "target_domains": {}, "tripwire_files": [".github/workflows/ci.yml"], "widening_reason": "tripwire" }Counts and a directory histogram, not raw paths. A PR can touch thousands of files, they exceed property limits, and in aggregate the histogram answers the same questions. The exception is
tripwire_files, which names the handful of paths that forcedALL— that is the field that says which rule to go tune.widening_reasonseparates the three ways a PR lands in one lane:tripwire(a rule did its job),unclassified_path(no rule claimed a path, the early warning that the script needs a new rule for a directory someone just added), anddiff_unavailable(the compute step widened and bailed before the file list existed). Collapsing those would bury the second behind the noise of the first, which is every workflow edit.Important
Emitted from the
uploadjob, notcompute. That split is a security boundary this workflow's header documents at length:computeruns PR-controlled code and is deliberately kept free of credentials, because a script can prepend a directory toGITHUB_PATHso a later step'sjqorcurlresolves to something attacker-supplied.computeis the obvious place to put this, since that's where the file list lives, and it is the wrong one. The summary crosses the boundary as a JSON string and is consumed throughenv:andjq --argjson, exactly like the existing target payload.No new secret. It reuses the write-only project token already committed in
hogli.yaml's telemetry block, which cannot read data — which is also why this works on fork PRs.Non-gating throughout:
continue-on-erroron both new steps, and the POST is capped at 20s. A telemetry problem must never be the reason a PR fails to enter the queue.What this does and does not measure
It measures lane cost — how often we widen, which rule widened us, how many lanes a PR claims.
It cannot measure lane correctness. A lane is wrong when two conflicting PRs get disjoint targets, merge in parallel, and break master. Neither the file list nor the target set can show that; it needs Trunk's record of what actually ran together plus master's post-merge result. A falling lane count is cheaper queueing, never evidence the rules are right — the script header says this in the code so a future reader hits it before building a dashboard on the wrong premise.
I looked for a Trunk batch id to make that join possible later.
setImpactedTargetsdoes not document one, so the response body is recorded verbatim (capped at 500 bytes) rather than parsed, and correlating a lane with its batch still needs Trunk's own export.How did you test this code?
node --test .github/scripts/trunk-lane-telemetry.test.js— 3 pass. The cases coverwidening_reasonattribution across all four outcomes, theALL-is-a-string trap (anything reading.lengthoff it reports 0 targets for both the widest and narrowest result), and that paths are summarized rather than sent.bin/hogli lint:workflows— 7 checks across 123 workflows, pass.actionlinton the changed workflow reports one SC2001 style note. It is pre-existing: the same rule fires onorigin/master's copy of the file at the samesed, and I confirmed that before touching anything.ALLcase is the JSON above.I did not send a live event — that needs the workflow running in CI.
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Automatic notifications
Docs update
Not applicable. The rationale lives in the new script's header and in the workflow comments.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
I asked Claude (Claude Code) whether instrumenting this was a good idea and to build it if so. The useful part of the answer was the scoping: the obvious version measures cost and reads like it measures quality, so the distinction is written into the code rather than left to whoever builds the dashboard.
Independent of #76481 and #76483, so it branches off master rather than stacking. Landing it after those two gives a baseline on the rules we're keeping instead of a week of data to discard.
One thing surfaced while dry-running that I have not fixed here:
listProductsreads every directory underproducts/, so stray.ruff_cacheand__pycache__directories becomepy:product:.ruff_cacheandpy:product:__pycache__targets. Harmless for correctness (they overlap consistently), but they inflate every widened set by two and pollute the target namespace. Worth a separate cleanup./writing-testsinvoked before adding the test cases.