diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0585434..4ef8333 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -188,6 +188,40 @@ jobs: - run: node compat.js working-directory: test/compat + # The GitHub Action in action.yml, run against a local build: the batched + # app must pass its baseline, the N+1 version of it must fail. + action: + needs: build + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + with: + node-version: 22.x + - uses: actions/download-artifact@v4 + with: + name: dist + path: dist + - id: same + uses: ./ + with: + run: examples/slow-apps/clean-batched/server.js --path /orders -n 30 + baseline: examples/slow-apps/clean-batched/perf-baseline.json + version: ./ + comment: 'false' + - id: nplus1 + uses: ./ + with: + run: examples/slow-apps/n-plus-one/server.js --path /orders -n 30 + baseline: examples/slow-apps/clean-batched/perf-baseline.json + version: ./ + comment: 'false' + fail: 'false' + - name: The batched app passed and the N+1 one was caught + run: | + test "${{ steps.same.outputs.regressed }}" = false + test "${{ steps.nplus1.outputs.regressed }}" = true + # Bun and Deno run the ESM build (nesting, stats, exporters, browser entry). other-runtimes: needs: build @@ -215,7 +249,7 @@ jobs: run: test/runtimes/workerd.sh publish-npm: - needs: [build, test, runtime, browser, driver-compat, other-runtimes] + needs: [build, test, runtime, browser, driver-compat, action, other-runtimes] if: github.event_name == 'push' && startsWith(github.ref, 'refs/tags/v') runs-on: ubuntu-latest permissions: diff --git a/CHANGELOG.md b/CHANGELOG.md index b43b9ee..0da428d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `traceperf/auto` wrote each request tree with its own `appendFileSync`, and under load that showed up as the app's "CPU cost" (`writeFileUtf8`). Writes are batched every 20 ms. ### Added +- **GitHub Action** (`uses: thelastbackspace/traceperf@`): runs the baseline check, fails the job on more calls per request, and posts one pull-request comment it updates on every push. `traceperf run --markdown file.md` writes the same summary. - `traceperf run` reads `traceperf.config.json` (or `--config file`); command-line options win. A `login` step logs in before the traffic and sends the returned token or cookies with every request; `${NAME}` in the file is read from the environment. - `npm run eval` scores `traceperf run` on 20 apps in `examples/slow-apps`: 12 with a planted problem, 8 clean ones where the right answer is "no issues". CI fails on any missed problem or false alarm. Before these fixes: 17/17 found, 15 false alarms; after: 17/17, 0. diff --git a/README.md b/README.md index a6b4ba1..6f0a73b 100644 --- a/README.md +++ b/README.md @@ -101,7 +101,7 @@ Checked on real apps, not only on the planted ones: Prisma's REST example, the R - `-c 10` concurrency, `-d 5s` duration or `-n 500` requests, `--method POST`, `--port 3000` (detected by default). - `-H "Authorization: Bearer $TOKEN"` (repeatable): request headers, for APIs behind a login. - `--probe 10`: requests per path sent one at a time before the load (0 for none). -- `--html report.html`, `--output calls.jsonl` (keep the trees), `--json`. +- `--html report.html`, `--output calls.jsonl` (keep the trees), `--json`, `--markdown summary.md` (for a PR comment or job summary). - `--warmup 3`: untimed requests per path first, so cold-start work doesn't look like a problem. - `--no-profile`: don't sample the CPU. - Any start command works: `npx traceperf run -- npm start`. @@ -149,6 +149,27 @@ It fails when a route makes more calls of any kind per request (database queries - run: npx traceperf run dist/server.js --path /orders --path /users/1 --baseline perf-baseline.json ``` +Or use the action, which also comments on the pull request with what changed, and updates that one comment on every push: + + +```yaml +permissions: + pull-requests: write +steps: + - uses: actions/checkout@v4 + - run: npm ci && npm run build && npm run db:seed + - uses: thelastbackspace/traceperf@v1.9.0 + with: + run: dist/server.js --path /orders --path /users/1 # or leave out and use traceperf.config.json + baseline: perf-baseline.json +``` + +> ### traceperf: ✖ more calls per request than the baseline +> - ✖ GET /orders: SELECT * FROM items WHERE order_id = ? 0 → 12 per request (an N+1?) +> - ✖ new problem: N+1 in GET /orders + +Inputs: `run`, `baseline`, `comment` (default `true`), `fail` (default `true`), `version` (traceperf version, default `1`). Output: `regressed`. The same report goes to the job summary; `--markdown file.md` writes it without the action. + `examples/slow-apps/` has 20 small apps: 12 with one planted problem each (blocking in your code and in built-ins, sequential awaits, N+1 over SQL and over HTTP, a slow or flaky dependency, failing requests…) and 8 clean ones where the right answer is "no issues". `npm run eval` scores `run` on them, and CI fails on a missed problem or a false alarm. The same naming works without `run`: `TRACEPERF_PROFILE_BLOCKING=1 node --require traceperf/auto server.js` adds the function names to the exit summary (sampling costs a few percent CPU, so use it while investigating). In code: `profileSlowCalls({ select: root => blockingRanges(root), mergeInlined: true, onProfile })`, then `explain(roots, { hotspots })`. diff --git a/action.yml b/action.yml new file mode 100644 index 0000000..8720426 --- /dev/null +++ b/action.yml @@ -0,0 +1,83 @@ +name: traceperf +description: Fail a pull request that adds database queries or HTTP calls per request (an N+1), and comment what changed +branding: + icon: activity + color: orange +inputs: + run: + description: 'Arguments for `traceperf run`: the app and its paths, e.g. "dist/server.js --path /orders --path /users/1". Leave empty to use traceperf.config.json.' + required: false + default: '' + baseline: + description: Committed baseline file (create it with `npx traceperf run ... --save-baseline perf-baseline.json`) + required: false + default: perf-baseline.json + comment: + description: Post (and update) one comment on the pull request with the result + required: false + default: 'true' + fail: + description: Fail the job when a route makes more calls per request than the baseline + required: false + default: 'true' + version: + description: traceperf version to run (npm version or range, or the path of a local build) + required: false + default: '1' + github-token: + description: Token for the comment (needs pull-requests write permission) + required: false + default: ${{ github.token }} +outputs: + regressed: + description: '"true" when a route makes more calls per request than the baseline' + value: ${{ steps.run.outputs.regressed }} +runs: + using: composite + steps: + - id: run + shell: bash + env: + TP_ARGS: ${{ inputs.run }} + TP_BASELINE: ${{ inputs.baseline }} + TP_VERSION: ${{ inputs.version }} + run: | + report="$RUNNER_TEMP/traceperf.md" + set +e + # shellcheck disable=SC2086 # the arguments are a word list on purpose + case "$TP_VERSION" in + # A local build (this repository's own CI tests the action with it) + ./*|/*) cli=(node "$TP_VERSION/dist/cli.js") ;; + *) cli=(npx --yes "traceperf@$TP_VERSION") ;; + esac + "${cli[@]}" run $TP_ARGS --baseline "$TP_BASELINE" --markdown "$report" + code=$? + set -e + if [ ! -s "$report" ]; then + printf '### traceperf: the run failed before it produced a report (exit %s)\n\nSee the job log.\n' "$code" > "$report" + fi + cat "$report" >> "$GITHUB_STEP_SUMMARY" + echo "report=$report" >> "$GITHUB_OUTPUT" + echo "code=$code" >> "$GITHUB_OUTPUT" + if [ "$code" -ne 0 ]; then echo "regressed=true" >> "$GITHUB_OUTPUT"; else echo "regressed=false" >> "$GITHUB_OUTPUT"; fi + - if: inputs.comment == 'true' && github.event_name == 'pull_request' + shell: bash + env: + GH_TOKEN: ${{ inputs.github-token }} + REPORT: ${{ steps.run.outputs.report }} + PR: ${{ github.event.pull_request.number }} + REPO: ${{ github.repository }} + run: | + # One comment per PR, updated on every push (found by its marker) + body="$(printf '\n%s' "$(cat "$REPORT")")" + id="$(gh api "repos/$REPO/issues/$PR/comments" --paginate --jq '.[] | select(.body | startswith("")) | .id' | head -n 1)" + if [ -n "$id" ]; then + gh api --method PATCH "repos/$REPO/issues/comments/$id" -f body="$body" > /dev/null + else + gh api --method POST "repos/$REPO/issues/$PR/comments" -f body="$body" > /dev/null + fi + - if: inputs.fail == 'true' && steps.run.outputs.code != '0' + shell: bash + run: | + echo "traceperf: more calls per request than ${{ inputs.baseline }}, or the run failed (see the report above)" + exit 1 diff --git a/examples/slow-apps/clean-batched/perf-baseline.json b/examples/slow-apps/clean-batched/perf-baseline.json new file mode 100644 index 0000000..fcdf185 --- /dev/null +++ b/examples/slow-apps/clean-batched/perf-baseline.json @@ -0,0 +1,13 @@ +{ + "version": 1, + "routes": { + "GET /orders": { + "requests": 10, + "calls": { + "SELECT * FROM items WHERE order_id IN (?)": 1, + "SELECT * FROM orders LIMIT 12": 1 + } + } + }, + "problems": [] +} diff --git a/src/__tests__/run.test.ts b/src/__tests__/run.test.ts index dfc2320..42ec458 100644 --- a/src/__tests__/run.test.ts +++ b/src/__tests__/run.test.ts @@ -412,3 +412,21 @@ describe('config file and login', () => { } }); }); + +describe('markdownReport', () => { + const { markdownReport } = jest.requireActual('../run'); + it('leads with the verdict, lists baseline changes, folds the timing findings', () => { + const md = markdownReport({ + trees: 30, findings: [{ kind: 'repeated', root: 'GET /orders', ms: 40, message: 'GET /orders called q 12 times', fix: 'batch it' }], + baseline: { regressions: ['GET /orders: q 1 → 12 per request (an N+1?)'], improvements: [], notes: [] }, baselineFile: 'b.json', + load: { requests: 30, durationMs: 300, statuses: { 200: 30 }, errors: 0, latency: {} }, + }); + expect(md).toMatch(/^### traceperf: ✖ more calls per request than the baseline\n\n- ✖ GET \/orders: q 1 → 12/); + expect(md).toContain('
Why it was slow'); + expect(md).toContain(' - fix: batch it'); + expect(md).toContain('30 requests in 0.3 s (100/s), 30× 200'); + expect(markdownReport({ trees: 5, findings: [], baseline: { regressions: [], improvements: [], notes: [] }, baselineFile: 'b.json' })) + .toMatch(/^### traceperf: ✔ no new queries or calls\n\n- ✔ same calls per request on every route/); + expect(markdownReport({ trees: 5, findings: [] })).toMatch(/^### traceperf: no issues found/); + }); +}); diff --git a/src/run.ts b/src/run.ts index 7068c8c..f22ef47 100644 --- a/src/run.ts +++ b/src/run.ts @@ -34,6 +34,8 @@ export interface RunArgs { /** Request headers, e.g. { authorization: 'Bearer ...' } */ headers?: Record; html?: string; + /** Write a Markdown summary here (pull-request comments, job summaries) */ + markdown?: string; /** Write call counts per route here (a baseline for --baseline) */ saveBaseline?: string; /** Compare call counts per route with this baseline; exit 1 on more calls or new problems */ @@ -65,6 +67,7 @@ export const RUN_USAGE = ` traceperf run [app args] [options] --html out.html write an HTML report --output f.jsonl keep the raw request trees --json print stats and findings as JSON + --markdown f.md write a Markdown summary (for a PR comment or job summary) --save-baseline f.json save calls per request for each route (commit it) --baseline f.json compare with it; exit 1 if a route makes more calls (e.g. an N+1) or a new problem appears. Counts, not @@ -252,6 +255,7 @@ function parseArgv(argv: string[], partial = false, start?: RunArgs): RunArgs | break; } case '--json': args.json = true; break; + case '--markdown': if (!(args.markdown = argv[++i])) return '--markdown expects a file'; break; case '--save-baseline': if (!(args.saveBaseline = argv[++i])) return '--save-baseline expects a file'; break; case '--baseline': if (!(args.baseline = argv[++i])) return '--baseline expects a file'; break; case '--no-profile': args.profile = false; break; @@ -856,6 +860,9 @@ export async function runApp(args: RunArgs, deps: RunDeps): Promise { if (args.html && summary.trees) { fs.writeFileSync(args.html, deps.report(text, { html: true, findings: summary.findings, title: `traceperf run: ${args.command.join(' ')}` })); } + if (args.markdown) { + fs.writeFileSync(args.markdown, markdownReport({ load, findings: summary.findings, baseline: baselineReport, baselineFile: args.baseline, baselineError, trees: summary.trees || 0 })); + } if (args.json) { deps.out(JSON.stringify({ load, trees: summary.trees, findings: summary.findings, ...(baselineReport ? { baseline: baselineReport } : {}) }, null, 2)); } else if (!summary.trees) { @@ -887,6 +894,39 @@ export async function runApp(args: RunArgs, deps: RunDeps): Promise { return !summary.trees && exitCode ? 1 : 0; } +/** A short Markdown summary for a pull-request comment or a CI job summary */ +export function markdownReport(r: { + load?: LoadResult; findings: Finding[]; trees: number; + baseline?: { regressions: string[]; improvements: string[]; notes: string[] }; baselineFile?: string; baselineError?: string; +}): string { + const out: string[] = []; + const failed = !!r.baselineError || !!(r.baseline && r.baseline.regressions.length); + out.push(`### traceperf: ${failed ? '✖ more calls per request than the baseline' : r.baseline ? '✔ no new queries or calls' : `${r.findings.length ? `${r.findings.length} finding${r.findings.length === 1 ? '' : 's'}` : 'no issues found'}`}`, ''); + if (r.baselineError) out.push(r.baselineError, ''); + if (r.baseline) { + const { regressions, improvements, notes } = r.baseline; + for (const x of regressions) out.push(`- ✖ ${x}`); + for (const x of improvements) out.push(`- ✔ ${x}`); + for (const x of notes) out.push(`- ${x}`); + if (!regressions.length && !improvements.length) out.push('- ✔ same calls per request on every route'); + out.push('', `Compared with \`${r.baselineFile}\`: calls per request from one-at-a-time requests, so the result does not depend on the machine.`, ''); + } + if (r.findings.length) { + out.push('
Why it was slow (timings from this CI machine)', ''); + for (const f of r.findings) { + out.push(`- **${f.kind}**: ${f.message.replace(/\n/g, ' ')}`); + if (f.fix) out.push(` - fix: ${f.fix}`); + } + out.push('', '
', ''); + } + if (r.load) { + const rate = r.load.requests / Math.max(r.load.durationMs / 1000, 1e-3); + const statuses = Object.entries(r.load.statuses).map(([s, n]) => `${n}× ${s}`).join(', '); + out.push(`${r.load.requests} requests in ${(r.load.durationMs / 1000).toFixed(1)} s (${rate.toFixed(0)}/s)${statuses ? `, ${statuses}` : ''}; ${r.trees} request trees`); + } + return out.join('\n') + '\n'; +} + function waitForPort(fs: any, file: string, exited: () => boolean, timeoutMs: number): Promise { const deadline = Date.now() + timeoutMs; return new Promise(resolve => {