-
Notifications
You must be signed in to change notification settings - Fork 3.1k
feat(ci): record merge-queue lane assignment as telemetry #76593
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+221
−1
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| #!/usr/bin/env node | ||
|
|
||
| // Builds the property bag for the `trunk_lane_targets` event from a PR's | ||
| // changed files and the target set trunk-impacted-targets.js computed for them. | ||
| // | ||
| // WHAT THIS MEASURES: the cost side of lane assignment — how often a PR widens, | ||
| // which rule widened it, and how many lanes it ends up claiming. It cannot | ||
| // measure the safety side. A lane is wrong when two conflicting PRs get | ||
| // disjoint targets and merge in parallel, and neither the file list nor the | ||
| // target set can show that; it takes Trunk's record of what actually ran | ||
| // together plus master's post-merge result. Read a falling lane count as | ||
| // cheaper queueing, never as evidence the rules are correct. | ||
| // | ||
| // Raw paths are deliberately not sent. A PR can touch thousands of them, they | ||
| // blow past property limits, and in aggregate the directory histogram answers | ||
| // the same questions. The exception is tripwire_files, which names the handful | ||
| // of paths that forced ALL, because that is the field that says which rule to | ||
| // go tune. | ||
| // | ||
| // Input: changed file paths, one per line, on stdin | ||
| // IMPACTED_TARGETS — the JSON uploaded to Trunk, {"impactedTargets": ...} | ||
| // Output: JSON object of event properties on stdout | ||
|
|
||
| const fs = require('fs') | ||
| const { isTripwire } = require('./trunk-impacted-targets') | ||
|
|
||
| // Enough to name the culprit without turning a wide PR into a huge payload. | ||
| const MAX_LISTED = 20 | ||
|
|
||
| function domainOf(target) { | ||
| const prefix = target.split(':')[0] | ||
| return ['py', 'fe', 'rust', 'svc', 'node', 'tools', 'agents', 'prose'].includes(prefix) ? prefix : 'other' | ||
| } | ||
|
|
||
| function buildProperties(changedFiles, impactedTargets) { | ||
| const isAll = impactedTargets === 'ALL' | ||
| const targets = Array.isArray(impactedTargets) ? impactedTargets : [] | ||
| const isProse = targets.length === 1 && targets[0] === 'prose' | ||
|
|
||
| const targetDomains = {} | ||
| for (const target of targets) { | ||
| const domain = domainOf(target) | ||
| targetDomains[domain] = (targetDomains[domain] || 0) + 1 | ||
| } | ||
|
|
||
| const topDirs = {} | ||
| const products = new Set() | ||
| for (const file of changedFiles) { | ||
| const segments = file.split('/') | ||
| topDirs[segments[0]] = (topDirs[segments[0]] || 0) + 1 | ||
| if (segments[0] === 'products' && segments.length > 1) { | ||
| products.add(segments[1]) | ||
| } | ||
| } | ||
|
|
||
| const tripwireFiles = changedFiles.filter(isTripwire) | ||
|
|
||
| return { | ||
| changed_file_count: changedFiles.length, | ||
| changed_top_dirs: topDirs, | ||
| changed_products: [...products].sort().slice(0, MAX_LISTED), | ||
| changed_product_count: products.size, | ||
| is_all: isAll, | ||
| is_prose: isProse, | ||
| target_count: targets.length, | ||
| targets: targets.slice(0, MAX_LISTED), | ||
| target_domains: targetDomains, | ||
| tripwire_files: tripwireFiles.slice(0, MAX_LISTED), | ||
| // Separates the three ways a PR ends up in one lane: a rule that | ||
| // deliberately widened it, a path no rule claimed (the early warning | ||
| // that the script needs a rule for a directory someone just added), and | ||
| // the degraded case where the diff itself failed and the file list | ||
| // never reached the script. | ||
| widening_reason: !isAll | ||
| ? null | ||
| : changedFiles.length === 0 | ||
| ? 'diff_unavailable' | ||
| : tripwireFiles.length > 0 | ||
| ? 'tripwire' | ||
| : 'unclassified_path', | ||
| } | ||
| } | ||
|
|
||
| module.exports = { buildProperties } | ||
|
|
||
| if (require.main === module) { | ||
| const changedFiles = fs | ||
| .readFileSync(0, 'utf8') | ||
| .split('\n') | ||
| .map((line) => line.trim()) | ||
| .filter(Boolean) | ||
| let impactedTargets | ||
| try { | ||
| impactedTargets = JSON.parse(process.env.IMPACTED_TARGETS || '{}').impactedTargets | ||
| } catch (error) { | ||
| console.error(`Could not read IMPACTED_TARGETS (${error.message}); reporting the file side only`) | ||
| } | ||
| process.stdout.write(JSON.stringify(buildProperties(changedFiles, impactedTargets))) | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| // Run with: node --test .github/scripts/trunk-lane-telemetry.test.js | ||
|
|
||
| const test = require('node:test') | ||
| const assert = require('node:assert/strict') | ||
|
|
||
| const { buildProperties } = require('./trunk-lane-telemetry') | ||
|
|
||
| // widening_reason is the field the dashboard acts on: a tripwire hit is a rule | ||
| // doing its job, while an unclassified path means a directory exists that no | ||
| // rule claims yet. Collapsing the two would hide the second behind the noise of | ||
| // the first, which is every workflow edit. | ||
| test('widening is attributed to the rule that caused it', () => { | ||
| const cases = [ | ||
| [['.github/workflows/ci.yml'], 'ALL', 'tripwire'], | ||
| [['terraform/main.tf'], 'ALL', 'unclassified_path'], | ||
| [['products/alpha/frontend/Scene.tsx'], ['fe:product:alpha'], null], | ||
| // The compute step widens and exits early when the merge base or diff | ||
| // is unavailable, so the file list never reaches the script. Reading | ||
| // that as unclassified_path would fake a missing-rule alert. | ||
| [[], 'ALL', 'diff_unavailable'], | ||
| ] | ||
| for (const [files, targets, expected] of cases) { | ||
| assert.equal(buildProperties(files, targets).widening_reason, expected, files[0]) | ||
| } | ||
| }) | ||
|
|
||
| // ALL is the string "ALL", not an array, so anything reading .length off it | ||
| // reports a target_count of 0 for both the widest and the narrowest outcome. | ||
| test('an ALL change set reports no targets but is flagged', () => { | ||
| const props = buildProperties(['bin/start'], 'ALL') | ||
| assert.equal(props.is_all, true) | ||
| assert.equal(props.target_count, 0) | ||
| assert.deepEqual(props.targets, []) | ||
| assert.deepEqual(props.tripwire_files, ['bin/start']) | ||
| }) | ||
|
|
||
| test('file paths are summarized rather than sent', () => { | ||
| const files = ['products/alpha/backend/api.py', 'products/beta/frontend/X.tsx', 'posthog/models/team.py'] | ||
| const props = buildProperties(files, ['py:core', 'fe:product:beta']) | ||
| assert.deepEqual(props.changed_top_dirs, { products: 2, posthog: 1 }) | ||
| assert.deepEqual(props.changed_products, ['alpha', 'beta']) | ||
| assert.equal(props.changed_file_count, 3) | ||
| assert.deepEqual(props.target_domains, { py: 1, fe: 1 }) | ||
| }) |
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
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.