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..9562a85248c8 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,62 @@ 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 '')" + + # 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 "$REPO_OWNER/$REPO_NAME#$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