From a4c46566a3b86b165aa67df2bcdbdec3a1935530 Mon Sep 17 00:00:00 2001 From: Georges-Antoine Assi Date: Mon, 3 Aug 2026 07:28:26 -0400 Subject: [PATCH 1/2] feat(ci): record merge-queue lane assignment as telemetry 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) --- .github/scripts/trunk-lane-telemetry.js | 99 ++++++++++++++++++++ .github/scripts/trunk-lane-telemetry.test.js | 44 +++++++++ .github/workflows/trunk-impacted-targets.yml | 73 ++++++++++++++- 3 files changed, 215 insertions(+), 1 deletion(-) create mode 100644 .github/scripts/trunk-lane-telemetry.js create mode 100644 .github/scripts/trunk-lane-telemetry.test.js diff --git a/.github/scripts/trunk-lane-telemetry.js b/.github/scripts/trunk-lane-telemetry.js new file mode 100644 index 000000000000..b73169e39756 --- /dev/null +++ b/.github/scripts/trunk-lane-telemetry.js @@ -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))) +} diff --git a/.github/scripts/trunk-lane-telemetry.test.js b/.github/scripts/trunk-lane-telemetry.test.js new file mode 100644 index 000000000000..0225bdd08372 --- /dev/null +++ b/.github/scripts/trunk-lane-telemetry.test.js @@ -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 }) +}) diff --git a/.github/workflows/trunk-impacted-targets.yml b/.github/workflows/trunk-impacted-targets.yml index b0465376a192..6dadd08a3d16 100644 --- a/.github/workflows/trunk-impacted-targets.yml +++ b/.github/workflows/trunk-impacted-targets.yml @@ -54,6 +54,7 @@ jobs: contents: read outputs: targets: ${{ steps.targets.outputs.targets }} + telemetry: ${{ steps.telemetry.outputs.properties }} steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: @@ -94,12 +95,30 @@ jobs: fi echo "$changed" | sed 's/^/ /' + printf '%s\n' "$changed" > "$RUNNER_TEMP/changed-files.txt" # The script prints "ALL" or a JSON array on stdout and never # exits non-zero; it degrades to "ALL" internally on any error. computed="$(printf '%s\n' "$changed" | node .github/scripts/trunk-impacted-targets.js)" printf 'targets=%s\n' "$(jq -cn --argjson t "$computed" '{impactedTargets: $t}')" >> "$GITHUB_OUTPUT" + # Summarizes the change set and the targets for the lane-cost + # dashboard. Never gates: a telemetry problem must not keep a PR out + # of the queue. The early exits above leave no file behind, which + # the script reports as the degraded case rather than as a PR that + # touched nothing. + - name: Summarize lane assignment + id: telemetry + if: ${{ !cancelled() }} + continue-on-error: true + env: + IMPACTED_TARGETS: ${{ steps.targets.outputs.targets }} + run: | + set -uo pipefail + touch "$RUNNER_TEMP/changed-files.txt" + properties="$(node .github/scripts/trunk-lane-telemetry.js < "$RUNNER_TEMP/changed-files.txt")" + printf 'properties=%s\n' "$properties" >> "$GITHUB_OUTPUT" + upload: name: Upload impacted targets to Trunk needs: compute @@ -167,4 +186,56 @@ jobs: -X POST "https://api.trunk.io/v1/setImpactedTargets" \ -H "Content-Type: application/json" \ -H "$auth_header" \ - --data "$payload" + --data "$payload" \ + --output "$RUNNER_TEMP/trunk-response.json" + + # Lane-cost telemetry. Runs here rather than in `compute` because + # `compute` executes PR-controlled code and is deliberately kept free + # of credentials; this job holds them and never checks out the repo. + # The summary crosses that boundary as a JSON string and is consumed + # through env: and jq --argjson, never interpolated into a command. + # + # The key is the write-only token from hogli.yaml's telemetry block: + # it cannot read data, which is why it is committed rather than a + # secret, and why this works on fork PRs too. + - name: Record lane telemetry + if: ${{ !cancelled() }} + continue-on-error: true + env: + POSTHOG_TELEMETRY_HOST: https://us.i.posthog.com + POSTHOG_TELEMETRY_API_KEY: phc_JYFXrbqdzueOYb0wFUTnCglFKZuC4xRXBW790ewdcvn + LANE_PROPERTIES: ${{ needs.compute.outputs.telemetry }} + run: | + set -uo pipefail + + if [ -z "$LANE_PROPERTIES" ]; then + echo "::warning::No lane summary available; skipping telemetry." + exit 0 + fi + + # Trunk does not document a batch id on this response, so + # whatever it returns is recorded verbatim (capped) rather + # than parsed. Correlating a lane with the PRs Trunk actually + # ran beside it still needs Trunk's own record. + trunk_response="$(head -c 500 "$RUNNER_TEMP/trunk-response.json" 2>/dev/null || echo '')" + + event="$(jq -n \ + --arg key "$POSTHOG_TELEMETRY_API_KEY" \ + --arg distinct_id "pr-$PR_NUMBER" \ + --arg repo "$REPO_OWNER/$REPO_NAME" \ + --argjson pr_number "$PR_NUMBER" \ + --arg head_sha "$PR_SHA" \ + --arg target_branch "$TARGET_BRANCH" \ + --arg is_fork "$IS_FORK" \ + --arg run_id "$RUN_ID" \ + --arg trunk_response "$trunk_response" \ + --argjson lane "$LANE_PROPERTIES" \ + '{api_key: $key, event: "trunk_lane_targets", distinct_id: $distinct_id, + properties: ($lane + {repo: $repo, pr_number: $pr_number, head_sha: $head_sha, + target_branch: $target_branch, is_fork: ($is_fork == "true"), + workflow_run_id: $run_id, trunk_response: $trunk_response})}')" + + curl --silent --show-error --max-time 20 \ + -X POST "$POSTHOG_TELEMETRY_HOST/i/v0/e/" \ + -H "Content-Type: application/json" \ + --data "$event" > /dev/null From 22af026e818e7c75a1a6aacc3b99cc74a4ecb40e Mon Sep 17 00:00:00 2001 From: Georges-Antoine Assi Date: Mon, 3 Aug 2026 09:18:00 -0400 Subject: [PATCH 2/2] fix(ci): qualify the lane telemetry distinct_id with the repository The write-only key is committed so the workflow also runs in forks, where PR numbers restart at 1. A bare pr- 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) --- .github/workflows/trunk-impacted-targets.yml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/.github/workflows/trunk-impacted-targets.yml b/.github/workflows/trunk-impacted-targets.yml index 6dadd08a3d16..9562a85248c8 100644 --- a/.github/workflows/trunk-impacted-targets.yml +++ b/.github/workflows/trunk-impacted-targets.yml @@ -219,9 +219,15 @@ jobs: # ran beside it still needs Trunk's own record. trunk_response="$(head -c 500 "$RUNNER_TEMP/trunk-response.json" 2>/dev/null || echo '')" + # The key is committed, so any fork that keeps these files + # reports into the same project, and PR numbers restart at 1 + # in every fork. Qualifying the id with the repository keeps + # one person per pull request instead of merging ours with a + # fork's PR of the same number. Same spelling as a GitHub + # cross-repo reference, so it reads as the PR it names. event="$(jq -n \ --arg key "$POSTHOG_TELEMETRY_API_KEY" \ - --arg distinct_id "pr-$PR_NUMBER" \ + --arg distinct_id "$REPO_OWNER/$REPO_NAME#$PR_NUMBER" \ --arg repo "$REPO_OWNER/$REPO_NAME" \ --argjson pr_number "$PR_NUMBER" \ --arg head_sha "$PR_SHA" \