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 <stuhood@gmail.com> Signed-off-by: Ankit <573824+ankitml@users.noreply.github.com>
Ankitml/git graph awareness
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a GitGraphAnalyzer that uses git ancestry to find previous benchmarks and compute insertion indices; addBenchmarkEntry now inserts new benchmark at the ancestry-derived position (then truncates by maxItems). New unit and integration tests validate analyzer behavior and integration. 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: findInsertionIndex(newCommitSha)
Analyzer->>Git: git log --oneline --topo-order (currentSha)
Git-->>Analyzer: ancestry array or error
rect rgba(230,240,250,0.5)
Analyzer->>Data: findPreviousBenchmark(suites, currentSha)
Data-->>Analyzer: previousBenchmark or null
end
rect rgba(240,250,230,0.5)
Analyzer->>Data: return insertIndex
Add->>Data: insert newBenchmark at insertIndex
Data->>Data: truncate oldest entries if > maxItems
Data-->>Action: return updated benchmark data
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
ktrz
left a comment
ktrz
left a comment
There was a problem hiding this comment.
Thank you @ankitml, @philippemnoel for this contribution. I've added some comments.
My main concerns are:
- unit tests are failing - please have a look what might be the issue
- I would prefer this to be an opt-in behavior until we can verify that it's working correctly for existing users. And possibly create some kind of migration script that reorders the current benchmarks ordering to match the new pattern
- with the current visualization logic do we need to provide any additional changes so that the resulting graph makes sense? I feel like with this ordering it still isn't gonna show it properly on the one graph (if there is possible branching happening
| // Prepare data points for charts (uses server-side ordering) | ||
| return Object.keys(data.entries).map(name => { | ||
| const entries = data.entries[name]; | ||
| return { | ||
| name, | ||
| dataSet: collectBenchesPerTestCase(entries), | ||
| }; | ||
| }); |
There was a problem hiding this comment.
This change seems to be just stylistic, right? Let's revert as it's not relevant to this change
| // Find previous benchmark using git ancestry | ||
| core.debug(`Finding previous benchmark for commit: ${benchEntry.commit.id}`); | ||
|
|
||
| prevBench = gitAnalyzer.findPreviousBenchmark(suites, benchEntry.commit.id); |
There was a problem hiding this comment.
This is an interesting approach. I think it is a good one, but I'm not sure that we should make it the new default just yet. I'd prefer to add a config option to the action so that it's an opt-in feature unless we can comprehensively test it so that we are sure that it's backwards compatible.
|
|
||
| if (ancestry.length === 0) { | ||
| core.warning(`No ancestry found for commit ${currentSha}, falling back to execution time ordering`); | ||
| return this.findPreviousByExecutionTime(suites, currentSha); |
There was a problem hiding this comment.
I would suggest not intermingling the git graph logic with the previous logic, and instead return null here and move the fallback handling to the caller
| const currentIndex = ancestry.indexOf(currentSha); | ||
| if (currentIndex === -1) { | ||
| core.warning(`Current commit ${currentSha} not found in ancestry, falling back to execution time ordering`); | ||
| return this.findPreviousByExecutionTime(suites, currentSha); |
There was a problem hiding this comment.
| const ancestry = this.getAncestry(newCommitSha); | ||
| if (ancestry.length === 0) { | ||
| core.debug('No ancestry found, appending to end'); | ||
| return suites.length; |
There was a problem hiding this comment.
similar to the logic used to find the previous benchmark. It think it's cleaner to separate the git graph logic from the "inserting at the end" as a fallback. If we return null or -1 here we could move the fallback logic to the addBenchmarkEntry
test/write.spec.ts
Outdated
| jest.mock('../src/gitGraph', () => ({ | ||
| GitGraphAnalyzer: jest.fn().mockImplementation(() => ({ | ||
| isGitAvailable: () => true, | ||
| getAncestry: (ref: string) => [], |
| return result.stdout | ||
| .split('\n') | ||
| .filter((line) => line.trim()) | ||
| .map((line) => line.split(' ')[0]); // Extract SHA from "sha message" |
There was a problem hiding this comment.
worth noting that for large repos, this might be huge. Something to keep an eye on. That's another reason why I'd like to have this feature opt-in until we verify it's not causing issues for current users
There was a problem hiding this comment.
Alternatively, we could add a reasonable limit to the git command so that we don't query for all the commits until the beginning of the repo
| * Find the insertion index for a new benchmark entry based on git ancestry. | ||
| * Inserts after the most recent ancestor in the existing suites. | ||
| */ | ||
| findInsertionIndex(suites: Benchmark[], newCommitSha: string): number { |
There was a problem hiding this comment.
Just a thought. findInsertionIndex and findPreviousBenchmark feel like they are conceptually very similar. Couldn't we refactor so that we can reuse more logic?
something like
function findPreviousBenchmarkIndex(/*params*/) {
// finding the previous bench idx
}
function findPreviousBenchmark(...) {
// simplified implementation
return benches[findPreviousBenchmarkIndex()]
}
function findInsertionIndex() {
// simplified
return findPreviousBenchmark() + 1
}
|
Working on these comments in our internal fork and testing them. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/addBenchmarkEntry.ts (1)
37-47: Newly inserted benchmark may be immediately truncated.When
findInsertionIndexreturns a low index (e.g., the new commit is a child of an older ancestor), the entry is spliced into the middle of the array. The subsequent truncation removes entries from the beginning, which could remove the just-inserted benchmark.Example scenario:
suites = [A, B, C, D, E](5 entries),maxItems = 4- New commit
Fis a child ofA, inserted at index 1:[A, F, B, C, D, E]- Truncation:
splice(0, 6-4)removes first 2 entries →[B, C, D, E]- Result:
Fwas just inserted and immediately discardedSuggested fix: Ensure newly inserted entry is preserved
suites.splice(insertionIndex, 0, normalizedCurrentBench); if (maxItems !== null && suites.length > maxItems) { - suites.splice(0, suites.length - maxItems); + // Calculate how many entries to remove, but ensure we don't remove the newly inserted entry + const entriesToRemove = suites.length - maxItems; + // After insertion, the new entry is at insertionIndex. Only remove entries before it. + const safeRemoveCount = Math.min(entriesToRemove, insertionIndex); + if (safeRemoveCount > 0) { + suites.splice(0, safeRemoveCount); + } core.debug( `Number of data items for '${benchName}' was truncated to ${maxItems} due to max-items-in-charts`, ); }Alternatively, consider whether the truncation strategy should be revisited entirely for git-graph aware ordering—perhaps keeping the
maxItemsentries that are most relevant based on ancestry rather than array position.
🧹 Nitpick comments (1)
test/gitGraphAnalyzer.test.ts (1)
263-315: Consider adding test for insertion at a low index with truncation.The current tests verify insertion index calculation but don't cover the interaction with
maxItemstruncation inaddBenchmarkEntry. Given the potential bug where a newly inserted entry could be immediately truncated, adding a test case intest/addBenchmarkEntryGitGraph.test.tswould help catch regressions:it('should not truncate newly inserted benchmark when inserted at low index', () => { // Setup: 5 existing suites, maxItems = 4 // New commit is child of first entry, inserted at index 1 // Verify the new entry is preserved after truncation });
2 participants
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