diff --git a/.github/scripts/__tests__/pr-triage-classify.test.js b/.github/scripts/__tests__/pr-triage-classify.test.js index cb44204bee..53d3f16242 100644 --- a/.github/scripts/__tests__/pr-triage-classify.test.js +++ b/.github/scripts/__tests__/pr-triage-classify.test.js @@ -10,6 +10,7 @@ const assert = require('node:assert/strict'); const { isTestFile, isTrivialFile, isCriticalFile, isSecurityCriticalFile, isCoreCriticalFile, isInfraCriticalFile, + isQueryEngineCriticalFile, isInternalToolingFile, isReleaseArtifactFile, computeSignals, determineTier, buildTierComment, } = require('../pr-triage-classify'); @@ -201,9 +202,34 @@ describe('critical-path bands', () => { } }); - it('isCriticalFile still matches either list (path-level check)', () => { + it('classifies the SQL rendering + execution pipeline as query-engine-critical', () => { + for (const f of [ + 'packages/common-utils/src/core/renderChartConfig.ts', + 'packages/common-utils/src/core/builderToRawSql.ts', + 'packages/common-utils/src/clickhouse/index.ts', + ]) { + assert.ok(isQueryEngineCriticalFile(f), `expected query-engine-critical: ${f}`); + assert.ok(!isSecurityCriticalFile(f), `expected not security-critical: ${f}`); + assert.ok(!isCoreCriticalFile(f), `expected not core-critical: ${f}`); + assert.ok(!isInfraCriticalFile(f), `expected not infra-critical: ${f}`); + } + }); + + it('does not sweep the rest of common-utils into the query-engine band', () => { + for (const f of [ + 'packages/common-utils/src/core/metadata.ts', + 'packages/common-utils/src/core/utils.ts', + 'packages/common-utils/src/queryParser.ts', + 'packages/common-utils/src/types.ts', + ]) { + assert.ok(!isQueryEngineCriticalFile(f), `expected not query-engine-critical: ${f}`); + } + }); + + it('isCriticalFile still matches any band (path-level check)', () => { assert.ok(isCriticalFile('packages/api/src/middleware/auth.ts')); assert.ok(isCriticalFile('docker/hyperdx/Dockerfile')); + assert.ok(isCriticalFile('packages/common-utils/src/core/renderChartConfig.ts')); assert.ok(!isCriticalFile('packages/app/src/App.tsx')); }); }); @@ -533,6 +559,55 @@ describe('determineTier', () => { ]), 4); }); + it('substantial query-engine rewrite escalates (PR #2859 pattern)', () => { + // Multi-series metric merge moved into ClickHouse: 664 prod lines across + // the rendering + execution pipeline. Tests and snapshots don't count. + assert.equal(classify('alice', 'feat/metrics-ch-side-merge', [ + makeFile('packages/common-utils/src/core/renderChartConfig.ts', 313, 36), + makeFile('packages/common-utils/src/clickhouse/index.ts', 19, 289), + makeFile('packages/common-utils/src/core/builderToRawSql.ts', 5, 2), + makeFile('packages/common-utils/src/__tests__/renderChartConfig.test.ts', 174, 0), + makeFile('packages/common-utils/src/__tests__/__snapshots__/renderChartConfig.test.ts.snap', 245, 0), + makeFile('packages/app/src/components/DBEditTimeChartForm/ChartEditorControls.tsx', 6, 6), + makeFile('packages/cli/src/shared/tileQuery.ts', 2, 2), + makeFile('.changeset/metrics-ch-side-merge.md', 6, 0), + ]), 4); + }); + + it('does NOT escalate a routine chart fix touching the query engine (PR #2759 pattern)', () => { + // Series-limit ranking fix: 68 lines in renderChartConfig, well under the + // 150-line bar. Half of all rendering-touching PRs look like this. + assert.equal(classify('alice', 'fix/ratio-series-limit', [ + makeFile('packages/common-utils/src/core/renderChartConfig.ts', 48, 20), + ]), 2); + }); + + it('escalates once query-engine churn reaches the bar', () => { + assert.equal(classify('alice', 'refactor/render-pipeline', [ + makeFile('packages/common-utils/src/core/renderChartConfig.ts', 120, 30), // exactly 150 + ]), 4); + assert.equal(classify('alice', 'refactor/render-pipeline', [ + makeFile('packages/common-utils/src/core/renderChartConfig.ts', 120, 29), // 149 — just under + ]), 2); + }); + + it('aggregates query-engine churn across files', () => { + // No single file clears the bar, but the pipeline change as a whole does + assert.equal(classify('alice', 'refactor/query-exec', [ + makeFile('packages/common-utils/src/core/renderChartConfig.ts', 60, 20), + makeFile('packages/common-utils/src/clickhouse/index.ts', 50, 15), + makeFile('packages/common-utils/src/core/builderToRawSql.ts', 5, 0), + ]), 4); + }); + + it('query-engine test files never count toward the bar', () => { + assert.equal(classify('alice', 'test/render-coverage', [ + makeFile('packages/common-utils/src/__tests__/renderChartConfig.test.ts', 400, 100), + makeFile('packages/common-utils/src/core/__tests__/builderToRawSql.test.ts', 200, 0), + makeFile('packages/common-utils/src/core/renderChartConfig.ts', 10, 5), + ]), 2); + }); + it('docs under a critical path never escalate', () => { assert.equal(classify('alice', 'docs/collector-readme', [ makeFile('packages/otel-collector/README.md', 60, 20), @@ -678,9 +753,11 @@ describe('determineTier', () => { }); it('cross-layer change below the line floor drops to Tier 2 (PR #2707 pattern)', () => { + // renderChartConfig is query-engine-critical, but 8 lines is far under + // the 150-line bar — a small cross-layer fix still tiers on size. assert.equal(classify('alice', 'fix/histogram-grouping', [ makeFile('packages/app/src/Chart.tsx', 10, 4), - makeFile('packages/common-utils/src/renderChartConfig.ts', 6, 2), + makeFile('packages/common-utils/src/core/renderChartConfig.ts', 6, 2), ]), 2); // 22 prod lines }); @@ -739,6 +816,9 @@ describe('buildTierComment', () => { infraCriticalFiles: [], infraCriticalLines: 0, infraCriticalEscalates: false, + queryEngineCriticalFiles: [], + queryEngineCriticalLines: 0, + queryEngineCriticalEscalates: false, internalToolingFiles: [], isBotAuthor: false, allFilesTrivial: false, @@ -809,6 +889,33 @@ describe('buildTierComment', () => { assert.ok(!body.includes('substantially modified')); }); + it('explains a substantial query-engine change as the Tier 4 trigger', () => { + const queryEngineCriticalFiles = [ + makeFile('packages/common-utils/src/core/renderChartConfig.ts', 313, 36), + makeFile('packages/common-utils/src/clickhouse/index.ts', 19, 289), + ]; + const body = buildTierComment(4, makeSignals({ + criticalFiles: queryEngineCriticalFiles, + queryEngineCriticalFiles, + queryEngineCriticalLines: 657, + queryEngineCriticalEscalates: true, + })); + assert.ok(body.includes('Query rendering engine substantially modified')); + assert.ok(body.includes('657 lines')); + assert.ok(body.includes('renderChartConfig.ts')); + assert.ok(!body.includes('Critical-path files'), 'no always-critical files were touched'); + }); + + it('notes a light query-engine touch as context rather than a trigger', () => { + const body = buildTierComment(2, makeSignals({ + queryEngineCriticalFiles: [makeFile('packages/common-utils/src/core/renderChartConfig.ts', 6, 2)], + queryEngineCriticalLines: 8, + queryEngineCriticalEscalates: false, + })); + assert.ok(body.includes('query rendering engine lightly')); + assert.ok(!body.includes('Query rendering engine substantially modified')); + }); + it('explains an automated release PR', () => { const body = buildTierComment(1, makeSignals({ isReleaseArtifactPR: true, diff --git a/.github/scripts/pr-triage-classify.js b/.github/scripts/pr-triage-classify.js index 1d17be9729..f7ee056bd5 100644 --- a/.github/scripts/pr-triage-classify.js +++ b/.github/scripts/pr-triage-classify.js @@ -1,12 +1,15 @@ 'use strict'; // ── File classification patterns ───────────────────────────────────────────── -// Critical paths come in three bands, ordered by how much the size of the diff +// Critical paths come in four bands, ordered by how much the size of the diff // should be allowed to soften them. // -// security — Tier 4 at any size, no escape hatch -// core — Tier 4 unless the PR merely grazes it (see the graze bounds) -// infra — Tier 4 only once substantially modified (INFRA_CRITICAL_MIN_LINES) +// security — Tier 4 at any size, no escape hatch +// core — Tier 4 unless the PR merely grazes it (see the graze bounds) +// infra — Tier 4 only once substantially modified (INFRA_CRITICAL_MIN_LINES) +// query-engine — Tier 4 only once substantially modified +// (QUERY_ENGINE_CRITICAL_MIN_LINES, higher than infra's bar — +// it is a hot path that routine chart fixes touch constantly) // // Without the bands, a three-line Dockerfile tweak carried the same "domain // expert sign-off" weight as an auth change. @@ -43,6 +46,18 @@ const INFRA_CRITICAL_PATTERNS = [ /^\.github\/workflows\/(main|release)\.yml$/, ]; +// The SQL rendering + execution pipeline in common-utils. Every chart, search, +// and alert query flows through renderChartConfig, so a substantial change here +// can silently alter query semantics product-wide (e.g. #2859 moving +// multi-series metric merge computation into ClickHouse). It is also one of the +// hottest files in the repo — routine chart fixes graze it weekly — so it gets +// its own band with a bar high enough that only engine-level rewrites escalate. +const QUERY_ENGINE_CRITICAL_PATTERNS = [ + /^packages\/common-utils\/src\/core\/renderChartConfig\./, + /^packages\/common-utils\/src\/core\/builderToRawSql\./, + /^packages\/common-utils\/src\/clickhouse\//, +]; + // Docs and images carry no functional risk, so they never escalate a PR — not // even under a critical path (e.g. packages/otel-collector/CHANGELOG.md). const DOC_PATTERN = /\.(md|txt|png|jpg|jpeg|gif|svg|ico)$/i; @@ -100,6 +115,14 @@ const TIER4_ESCALATION_LINES = 1000; // Tier 3 → 4 for very large diffs // change spread thinly across several of them still escalates. const INFRA_CRITICAL_MIN_LINES = 30; +// Query-engine paths use the same total-churn mechanism but a higher bar. +// Calibrated against 20 recent merged PRs touching these files: at infra's +// 30-line bar half of them escalated, including routine Tier 2 chart fixes +// (#2759, #2613, #2422); at 150 only substantial engine changes cleared it +// (#2246 Increase aggFn, #2634 builder→raw SQL, #2859 metric merge in CH) — +// exactly the changes that warrant a domain expert. +const QUERY_ENGINE_CRITICAL_MIN_LINES = 150; + // A PR only "grazes" a core-critical path when the touch AND the PR as a whole // are tiny. Both bounds matter: across 600 merged PRs, a <=10-line core-critical // touch inside a larger PR still drew substantive review comments 50% of the @@ -165,7 +188,7 @@ const TIER_INFO = { 4: { emoji: '🔴', headline: 'Tier 4 — Critical', - detail: 'Touches authentication, tenancy data models, the public API or shipped database config — or substantially changes background tasks, the OTel pipeline, image build, or release CI.', + detail: 'Touches authentication, tenancy data models, the public API or shipped database config — or substantially changes the query rendering engine, background tasks, the OTel pipeline, image build, or release CI.', process: 'Deep review from a domain expert. Synchronous walkthrough may be required.', sla: 'Schedule synchronous review within 2 business days.', }, @@ -179,10 +202,12 @@ const isDocFile = f => DOC_PATTERN.test(f); const isSecurityCriticalFile = f => SECURITY_CRITICAL_PATTERNS.some(p => p.test(f)); const isCoreCriticalFile = f => CORE_CRITICAL_PATTERNS.some(p => p.test(f)); const isInfraCriticalFile = f => INFRA_CRITICAL_PATTERNS.some(p => p.test(f)); -// Path-level check only — whether a core- or infra-critical file actually -// escalates the PR depends on diff size, resolved in computeSignals. +const isQueryEngineCriticalFile = f => QUERY_ENGINE_CRITICAL_PATTERNS.some(p => p.test(f)); +// Path-level check only — whether a core-, infra- or query-engine-critical file +// actually escalates the PR depends on diff size, resolved in computeSignals. const isCriticalFile = f => - isSecurityCriticalFile(f) || isCoreCriticalFile(f) || isInfraCriticalFile(f); + isSecurityCriticalFile(f) || isCoreCriticalFile(f) || isInfraCriticalFile(f) || + isQueryEngineCriticalFile(f); const isInternalToolingFile = f => INTERNAL_TOOLING_PATTERNS.some(p => p.test(f)); const isReleaseArtifactFile = f => RELEASE_ARTIFACT_PATTERNS.some(p => p.test(f)); @@ -223,9 +248,16 @@ function computeSignals(pr, filesRes) { !isSecurityCriticalFile(f.filename) && !isCoreCriticalFile(f.filename) ); + const queryEngineCriticalFiles = criticalCandidates.filter(f => + isQueryEngineCriticalFile(f.filename) && + !isSecurityCriticalFile(f.filename) && + !isCoreCriticalFile(f.filename) && + !isInfraCriticalFile(f.filename) + ); const coreCriticalLines = churn(coreCriticalFiles); const infraCriticalLines = churn(infraCriticalFiles); + const queryEngineCriticalLines = churn(queryEngineCriticalFiles); // Everything a reviewer actually has to read. Unlike prodLines this keeps // workflows, Actions scripts and internal-tooling packages in: they are @@ -247,11 +279,14 @@ function computeSignals(pr, filesRes) { reviewableLines <= GRAZE_MAX_PROD_LINES; const infraCriticalEscalates = infraCriticalLines >= INFRA_CRITICAL_MIN_LINES; + const queryEngineCriticalEscalates = + queryEngineCriticalLines >= QUERY_ENGINE_CRITICAL_MIN_LINES; const criticalFiles = [ ...securityCriticalFiles, ...(grazesCoreCritical ? [] : coreCriticalFiles), ...(infraCriticalEscalates ? infraCriticalFiles : []), + ...(queryEngineCriticalEscalates ? queryEngineCriticalFiles : []), ]; const isBotAuthor = BOT_AUTHORS.includes(author); @@ -287,8 +322,10 @@ function computeSignals(pr, filesRes) { author, branchName, prodFiles, prodLines, testLines, criticalFiles, securityCriticalFiles, coreCriticalFiles, infraCriticalFiles, - coreCriticalLines, infraCriticalLines, reviewableLines, - grazesCoreCritical, infraCriticalEscalates, + queryEngineCriticalFiles, + coreCriticalLines, infraCriticalLines, queryEngineCriticalLines, + reviewableLines, + grazesCoreCritical, infraCriticalEscalates, queryEngineCriticalEscalates, internalToolingFiles, isBotAuthor, allFilesTrivial, isReleaseArtifactPR, touchesApiModels, touchesFrontend, touchesBackend, touchesSharedUtils, @@ -339,8 +376,10 @@ function buildTierComment(tier, signals) { isCrossLayer, // Defaulted so older callers (and hand-built fixtures) keep working. securityCriticalFiles = [], coreCriticalFiles = [], infraCriticalFiles = [], - coreCriticalLines = 0, infraCriticalLines = 0, + queryEngineCriticalFiles = [], + coreCriticalLines = 0, infraCriticalLines = 0, queryEngineCriticalLines = 0, grazesCoreCritical = false, infraCriticalEscalates = false, + queryEngineCriticalEscalates = false, internalToolingFiles = [], isReleaseArtifactPR = false, spansLayers = false, } = signals; @@ -391,6 +430,9 @@ function buildTierComment(tier, signals) { if (infraCriticalEscalates && infraCriticalFiles.length > 0) { triggers.push(`**Background tasks or delivery pipeline substantially modified** — ${infraCriticalLines} lines (bar: ${INFRA_CRITICAL_MIN_LINES}):\n${fileList(infraCriticalFiles)}`); } + if (queryEngineCriticalEscalates && queryEngineCriticalFiles.length > 0) { + triggers.push(`**Query rendering engine substantially modified** — ${queryEngineCriticalLines} lines (bar: ${QUERY_ENGINE_CRITICAL_MIN_LINES}). Every chart, search, and alert query flows through this code:\n${fileList(queryEngineCriticalFiles)}`); + } if (tier === 4 && prodLines > TIER4_ESCALATION_LINES && criticalFiles.length === 0) { triggers.push(`**Large diff**: ${prodLines} production lines changed (threshold: ${TIER4_ESCALATION_LINES})`); } @@ -429,6 +471,9 @@ function buildTierComment(tier, signals) { if (infraCriticalFiles.length > 0 && !infraCriticalEscalates) { contextSignals.push(`touches background tasks or the delivery pipeline lightly (${infraCriticalLines} lines, under the ${INFRA_CRITICAL_MIN_LINES}-line bar for Tier 4)`); } + if (queryEngineCriticalFiles.length > 0 && !queryEngineCriticalEscalates) { + contextSignals.push(`touches the query rendering engine lightly (${queryEngineCriticalLines} lines, under the ${QUERY_ENGINE_CRITICAL_MIN_LINES}-line bar for Tier 4)`); + } if (spansLayers && !isCrossLayer) { contextSignals.push(`spans packages but only ${prodLines} prod lines — under the ${CROSS_LAYER_MIN_LINES}-line cross-layer bar`); } @@ -445,9 +490,11 @@ module.exports = { // Thresholds (exported for tests and documentation) TIER2_MAX_LINES, INFRA_CRITICAL_MIN_LINES, CROSS_LAYER_MIN_LINES, TIER4_ESCALATION_LINES, CORE_CRITICAL_GRAZE_LINES, GRAZE_MAX_PROD_LINES, + QUERY_ENGINE_CRITICAL_MIN_LINES, // Pure functions isTestFile, isTrivialFile, isDocFile, isCriticalFile, isSecurityCriticalFile, isCoreCriticalFile, isInfraCriticalFile, + isQueryEngineCriticalFile, isInternalToolingFile, isReleaseArtifactFile, computeSignals, determineTier, buildTierComment, };