Add git-graph aware sorting for benchmark data and comparisons #329
Conversation
Add GitGraphAnalyzer class for finding previous benchmarks based on git ancestry instead of execution time. This fixes release branch comparisons where PRs were comparing against wrong baseline commits. - Add src/gitGraph.ts with git analysis functions - Modify addBenchmarkEntry.ts to use git-graph aware previous benchmark selection - Update default_index_html.ts to sort visualization data by commit timestamp Fixes #3769
Add comprehensive tests for GitGraphAnalyzer and addBenchmarkEntry integration. All 227 tests pass successfully.
Co-authored-by: Stu Hood <[email protected]> Signed-off-by: Ankit <[email protected]>
Ankitml/git graph awareness
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughIntroduces GitGraphAnalyzer to use git ancestry when locating previous benchmarks and computing insertion indices; addBenchmarkEntry now inserts new benchmark at the ancestry-derived position (with existing truncation), and tests for the analyzer and integration were added. Changes
Sequence DiagramsequenceDiagram
participant Action as GitHub Action
participant Add as addBenchmarkEntry
participant Analyzer as GitGraphAnalyzer
participant Git as Git CLI
participant Data as Benchmark Data Store
Action->>Add: invoke addBenchmarkEntry(newBenchmark)
Add->>Analyzer: determine insertion (newCommitSha)
Analyzer->>Git: git log --oneline --topo-order (currentSha)
Git-->>Analyzer: ancestry array (or error)
rect rgb(230,240,250)
Note over Analyzer,Data: Resolve previous benchmark via ancestry
Analyzer->>Data: findPreviousBenchmark(suites, currentSha)
Data-->>Analyzer: previousBenchmark or null
end
rect rgb(240,250,230)
Note over Analyzer,Add: Compute insertion index
Analyzer->>Data: findInsertionIndex(suites, newCommitSha)
Data-->>Analyzer: insertIndex
end
Add->>Data: insert newBenchmark at insertIndex
Data->>Data: truncate oldest entries if > maxItems
Data-->>Action: return/emit updated benchmark data
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI Agents
In @src/gitGraph.ts:
- Around line 88-116: The code in findInsertionIndex relies on getAncestry
returning an array whose first element is the commit itself and does slice(1) to
exclude it; add a defensive check: call getAncestry(newCommitSha) as before,
verify ancestry.length>0 (already done) and then ensure the first element equals
newCommitSha before skipping it — if ancestry[0] === newCommitSha use
ancestry.slice(1) or otherwise build ancestorSet by filtering out newCommitSha
(or other unexpected values) so the commit never appears in ancestorSet; update
the variable ancestorSet creation near findInsertionIndex / getAncestry usage to
use this robust logic (reference: findInsertionIndex, getAncestry, ancestorSet).
- Around line 27-47: getAncestry currently interpolates the ref into a shell
string, enabling command injection; fix it by validating/sanitizing ref and
calling a non-shell exec variant with args instead of string interpolation:
ensure getAncestry checks ref against allowed git-ref patterns (hex SHA,
branch/tag name regex) and then invoke child_process.execFileSync or spawnSync
passing ['log','--oneline','--topo-order', ref] (or equivalent arg array) with
cwd and encoding to avoid a shell, and keep the existing try/catch and warnings
on failure.
In @test/write.spec.ts:
- Around line 119-131: The mock for GitGraphAnalyzer is incorrect: remove
non-existent methods getCurrentBranch and sortByGitOrder, and update the
signatures of existing mocks to match the real class by changing
findPreviousBenchmark(suites) to findPreviousBenchmark(suites, currentSha) and
findInsertionIndex(suites) to findInsertionIndex(suites, newCommitSha); ensure
the mocked implementations use the new parameters (currentSha/newCommitSha) so
tests exercise the same parameter-dependent logic as the real GitGraphAnalyzer.
🧹 Nitpick comments (5)
src/addBenchmarkEntry.ts (1)
15-15: Consider caching or injecting GitGraphAnalyzer to avoid repeatedgit --versionchecks.A new
GitGraphAnalyzerinstance is created on everyaddBenchmarkEntrycall. The constructor runsexecSync('git --version')to detect git availability. For workflows that process multiple benchmarks, this adds unnecessary overhead.🔎 Suggested approaches
Option 1: Module-level singleton
import { GitGraphAnalyzer } from './gitGraph'; + +// Reuse analyzer instance across calls +const gitAnalyzer = new GitGraphAnalyzer(); export function addBenchmarkEntry( benchName: string, benchEntry: Benchmark, entries: BenchmarkSuites, maxItems: number | null, ): { prevBench: Benchmark | null; normalizedCurrentBench: Benchmark } { let prevBench: Benchmark | null = null; let normalizedCurrentBench: Benchmark = benchEntry; - const gitAnalyzer = new GitGraphAnalyzer();Option 2: Dependency injection
export function addBenchmarkEntry( benchName: string, benchEntry: Benchmark, entries: BenchmarkSuites, maxItems: number | null, + gitAnalyzer: GitGraphAnalyzer = new GitGraphAnalyzer(), ): { prevBench: Benchmark | null; normalizedCurrentBench: Benchmark } {test/addBenchmarkEntryGitGraph.test.ts (1)
82-97: Consider usingBenchmarkSuitestype instead ofany.Line 85 uses
anytype forentries. Using the actualBenchmarkSuitestype from../src/writewould provide better type safety.🔎 Suggested fix
+import { BenchmarkSuites } from '../src/write'; + it('should create new benchmark suite when none exists', () => { const benchName = 'test-suite'; const benchEntry = createMockBenchmark('abc123'); - const entries: any = {}; + const entries: BenchmarkSuites = {};test/gitGraphAnalyzer.test.ts (1)
116-129: Consider extracting duplicatecreateMockBenchmarkhelper.The
createMockBenchmarkhelper is defined identically in bothfindPreviousBenchmarkandfindInsertionIndexdescribe blocks (lines 117-129 and 210-222). Consider extracting it to the top level of the describe block to reduce duplication.🔎 Suggested refactor
Move the helper to the top of the
GitGraphAnalyzerdescribe block:describe('GitGraphAnalyzer', () => { let analyzer: GitGraphAnalyzer; const originalEnv = process.env; + + const createMockBenchmark = (id: string, timestamp: string): Benchmark => ({ + commit: { + id, + timestamp, + message: `Commit ${id}`, + url: `https://github.com/test/repo/commit/${id}`, + author: { username: 'testuser' }, + committer: { username: 'testuser' }, + }, + date: Date.now(), + tool: 'cargo', + benches: [], + }); // ... rest of tests using shared helpersrc/gitGraph.ts (2)
8-15: Consider logging git CLI availability during construction.Currently, the constructor silently catches errors when checking git availability. Adding a debug log when git is unavailable could help with troubleshooting, though deferring the warning until actual usage (as done in
getAncestry) is also reasonable.🔎 Optional logging enhancement
constructor() { try { execSync('git --version', { stdio: 'ignore' }); this.gitCliAvailable = true; } catch (e) { this.gitCliAvailable = false; + core.debug('Git CLI not available during initialization'); } }
5-129: Consider caching ancestry results for performance.The
getAncestrymethod executes a git command on every call. If the method is invoked multiple times with the same ref during a single action run, caching the results could improve performance. However, given that GitHub Actions run infrequently and this is likely not a hot path, this optimization can be deferred.🔎 Example caching implementation
export class GitGraphAnalyzer { private readonly gitCliAvailable: boolean; + private ancestryCache: Map<string, string[]> = new Map(); // ... constructor ... getAncestry(ref: string): string[] { if (!this.gitCliAvailable) { core.warning('Git CLI not available, cannot determine ancestry'); return []; } + // Check cache first + if (this.ancestryCache.has(ref)) { + return this.ancestryCache.get(ref)!; + } + try { const output = execSync(`git log --oneline --topo-order ${ref}`, { encoding: 'utf8', cwd: process.env.GITHUB_WORKSPACE ?? process.cwd(), }); - return output + const result = output .split('\n') .filter((line) => line.trim()) .map((line) => line.split(' ')[0]); + + this.ancestryCache.set(ref, result); + return result; } catch (error) { core.warning(`Failed to get ancestry for ref ${ref}: ${error}`); return []; } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/addBenchmarkEntry.tssrc/config.tssrc/default_index_html.tssrc/gitGraph.tssrc/write.tstest/addBenchmarkEntryGitGraph.test.tstest/gitGraphAnalyzer.test.tstest/write.spec.ts
🧰 Additional context used
🧬 Code graph analysis (4)
test/addBenchmarkEntryGitGraph.test.ts (1)
src/addBenchmarkEntry.ts (1)
addBenchmarkEntry(7-50)
test/gitGraphAnalyzer.test.ts (1)
src/gitGraph.ts (1)
GitGraphAnalyzer(5-129)
src/addBenchmarkEntry.ts (2)
src/gitGraph.ts (1)
GitGraphAnalyzer(5-129)src/normalizeBenchmark.ts (1)
normalizeBenchmark(4-20)
src/gitGraph.ts (1)
examples/benchmarkjs/bench.js (1)
suite(2-2)
🔇 Additional comments (13)
src/config.ts (1)
6-7: LGTM!Trivial formatting change - no impact on functionality.
src/write.ts (1)
493-493: LGTM!Destructuring reorder has no functional impact - object property access is order-independent.
src/default_index_html.ts (1)
164-171: LGTM!Good refactor - the local variable improves readability and the comment clarifies that chart data ordering relies on server-side (git-graph) sorting, aligning with the PR's objectives.
src/addBenchmarkEntry.ts (1)
37-47: LGTM - Insertion and truncation logic correctly handles git-graph ordering.The insertion at the computed index maintains topological order, and truncation from the beginning removes entries that are earliest in the git history. This aligns with the PR's objective to order by git ancestry rather than timestamp.
test/addBenchmarkEntryGitGraph.test.ts (2)
1-15: LGTM - Good mock setup pattern.Mocks are correctly defined before imports, ensuring proper module hoisting behavior with Jest.
122-151: Good edge case coverage for maxItems truncation.This test validates that truncation works correctly with git-graph ordering - the oldest entries (by position) are removed, and the newest entry is inserted at the correct position. The assertion on line 147 properly verifies that
old1was removed.test/gitGraphAnalyzer.test.ts (3)
1-16: LGTM - Proper mock isolation for system-level dependencies.The mock setup correctly isolates
@actions/coreandchild_process.execSync, ensuring tests don't make actual git calls. Mocks are defined before imports as required.
18-30: Good environment isolation pattern.Preserving and restoring
process.envprevents test pollution. ThebeforeEach/afterEachpattern ensures each test starts with a clean state.
170-206: Good fallback behavior coverage.These tests verify that when git ancestry fails or returns unexpected results, the analyzer correctly falls back to execution-time ordering. This ensures graceful degradation in edge cases.
src/gitGraph.ts (4)
17-22: LGTM!Simple getter with clear purpose and correct implementation.
49-82: LGTM with good defensive programming.The logic correctly:
- Retrieves ancestry for the current commit
- Verifies the commit exists in ancestry before proceeding
- Iterates through ancestors to find the most recent benchmark
- Falls back gracefully to execution-time ordering when ancestry is unavailable
The multiple fallback paths ensure robustness.
118-128: LGTM!The fallback method correctly iterates through benchmarks in reverse execution order (most recent first) and returns the first benchmark that isn't the current commit. Good use of array spread to avoid mutating the original array.
34-34:--topo-orderis the correct flag for this use case.You're correct that
--topo-orderdiffers from depth-first traversal (which interleaves commits from parallel branches in timestamp order), but for ancestry tracking, topological ordering is appropriate—it ensures parent-child relationships are respected and avoids interleaving commits from different branches. If the PR description mentions "depth-first," the terminology may be imprecise, but the implementation with--topo-orderis sound.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/gitGraph.ts (1)
27-51: Excellent fix: Command injection vulnerability resolved.The method now correctly uses
spawnSyncwith an argument array instead of string interpolation, eliminating the command injection risk. The error handling and fallback behavior are appropriate.Optional: Consider adding ref validation for clearer error messages
While command injection is no longer possible, validating that
refmatches expected git reference patterns (SHA-1 hash, branch name, tag name) before calling git could provide clearer error messages for invalid inputs:/** * Get git ancestry using topological order */ getAncestry(ref: string): string[] { if (!this.gitCliAvailable) { core.warning('Git CLI not available, cannot determine ancestry'); return []; } // Optional: Validate ref format const validRefPattern = /^[a-zA-Z0-9._\-\/]+$/; if (!validRefPattern.test(ref)) { core.warning(`Invalid git ref format: ${ref}`); return []; } try { const result = spawnSync('git', ['log', '--oneline', '--topo-order', ref], { encoding: 'utf8', cwd: process.env.GITHUB_WORKSPACE ?? process.cwd(), }); // ... rest of implementationThis is a nice-to-have improvement, not a requirement.
test/write.spec.ts (1)
119-131: Mock parameters are unused, but this appears intentional for general write tests.The method signatures correctly include
currentShaandnewCommitShaparameters, but the mock implementations ignore them—findPreviousBenchmarkalways returns the last benchmark regardless of the commit SHA, andfindInsertionIndexalways returns the array length.However, this simplified behavior appears intentional:
write.spec.tstests generalwriteBenchmarkfunctionality, not git-graph-specific logic. The realGitGraphAnalyzer.findPreviousBenchmarkimplementation usescurrentShato traverse git ancestry and find related commits, but that behavior is tested in dedicated files (gitGraphAnalyzer.test.tsandaddBenchmarkEntryGitGraph.test.ts), which use proper spy mocks with assertions.Consider adding a comment above the mock definition explaining that it's intentionally simplified for general write tests while git-graph-specific logic is verified in dedicated test files. This would clarify the distinction and prevent confusion.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/gitGraph.tstest/write.spec.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/gitGraph.ts (1)
examples/benchmarkjs/bench.js (1)
suite(2-2)
🔇 Additional comments (5)
src/gitGraph.ts (5)
8-15: LGTM: Git availability check is well-implemented.The constructor safely checks for git CLI availability using
spawnSyncwith array arguments and handles errors gracefully by setting the availability flag.
17-22: LGTM: Clean getter implementation.The method provides appropriate public access to the git availability status.
53-86: LGTM: Well-structured ancestry-based benchmark lookup.The method correctly traverses the git ancestry to find the most recent previous benchmark, with appropriate fallback logic when ancestry information is unavailable. The logging is well-placed and the edge cases are handled properly.
88-122: Excellent implementation: Robust ancestry-based insertion logic.The method correctly determines the insertion index by finding the most recent ancestor in the existing suite. The defensive check on lines 104-106 ensures robustness even if the ancestry array structure is unexpected—it only skips the first element if it matches the commit being inserted. The backward iteration and fallback behavior are both appropriate.
124-134: LGTM: Simple and correct fallback implementation.The private fallback method correctly finds the most recent benchmark by execution time (excluding the current commit) when git ancestry is unavailable. The use of spread operator prevents mutation of the original array.
1 participant
This PR adds git-graph awareness to the benchmark action, addressing a critical issue where benchmark data points were sorted by commit timestamp rather than git topology. This caused incorrect graph visualizations and PR comparisons for release branches.
Problem
Previously, benchmark data was sorted by
commit.author.timestamp, which meant:0.20.x) would insert it at a time-based positionmain), not the most recent commit on the same branchSolution
gitGraph.ts: A new module that analyzes git topology to determine the "mainline" pathChanges
gitGraph.tswith git topology analysis functionsaddBenchmarkEntry.tsto use git-graph aware sortingRelated
Closes paradedb/paradedb#3769