DO NOT MERGE: Performance audit #121
Conversation
📝 WalkthroughWalkthroughThis set of changes introduces comprehensive performance measurement and logging across various components of the application, primarily focused on event handling, donation processing, and application/round statistics updates. The Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
packages/data-flow/src/orchestrator.ts (1)
101-108:⚠️ Potential issue
slowEventThresholdMsset to 0.5 ms – likely a typoWith network & DB I/O it’s impossible to process an event in under a microsecond scale.
The threshold was previously 500 ms; consider restoring a realistic value (e.g. 200–500 ms) to avoid every event being flagged as “slow”.-private readonly slowEventThresholdMs: number = 0.5; +private readonly slowEventThresholdMs: number = 500;
🧹 Nitpick comments (5)
packages/shared/src/utils/performance-logger.ts (1)
45-69: Consider adding error handling for file operationsThe file initialization logic correctly creates the necessary directory and file with headers. However, there's no error handling for file system operations.
Consider adding try/catch blocks to handle potential file system errors:
private initializeCsvFile(): void { - if (!fs.existsSync(this.csvPath)) { - const header = - [ - "timestamp", - "eventType", - "operation", - "duration", - "totalTime", - "blockNumber", - "transactionHash", - "chainId", - "roundId", - "applicationId", - "donorAddress", - "recipientAddress", - "amount", - "amountInUsd", - "uniqueDonorsCount", - "totalDonationsCount", - "details", - ].join(",") + "\n"; - fs.writeFileSync(this.csvPath, header); - } + try { + if (!fs.existsSync(this.csvPath)) { + const header = + [ + "timestamp", + "eventType", + "operation", + "duration", + "totalTime", + "blockNumber", + "transactionHash", + "chainId", + "roundId", + "applicationId", + "donorAddress", + "recipientAddress", + "amount", + "amountInUsd", + "uniqueDonorsCount", + "totalDonationsCount", + "details", + ].join(",") + "\n"; + fs.writeFileSync(this.csvPath, header); + } + } catch (error) { + console.error(`Failed to initialize CSV file: ${error instanceof Error ? error.message : String(error)}`); + } }packages/processors/src/processors/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/allocated.handler.ts (1)
47-132: Consider using the shared performance logger for consistencyWhile the console logging is useful, consider also using the shared
performanceLoggerfor consistency with other parts of the application and to benefit from the CSV logging capabilities.Consider adding the shared performance logger to record these metrics consistently:
+import { performanceLogger } from "@grants-stack-indexer/shared"; ... async handle(): Promise<Changeset[]> { const startTime = performance.now(); ... const totalTime = performance.now() - startTime; console.log(`[AllocatedWithOrigin] Total processing time: ${totalTime.toFixed(2)}ms`); + + performanceLogger.logMetric({ + timestamp: new Date().toISOString(), + eventType: "Strategy", + operation: "AllocatedWithOrigin", + duration: totalTime, + totalTime: totalTime, + blockNumber: this.event.blockNumber, + transactionHash: this.event.transactionFields.hash, + chainId: this.chainId, + roundId: round.id, + applicationId: application.id, + donorAddress: getAddress(this.event.params.origin), + recipientAddress: getAddress(parsedMetadata.application.recipient), + amount: amount.toString(), + amountInUsd: amountInUsd.toString(), + details: { + roundLookupTime, + appLookupTime, + tokenValidationTime, + priceCalcTime, + metadataTime + } + }); return [ ...packages/data-flow/src/data-loader/handlers/donation.handlers.ts (1)
50-73: Heavy aggregation withreduceconverts to Number.reduce((sum, d) => sum + Number(d.amountInUsd), 0)For batches with many donations this risks precision loss and
Infinityfor huge totals.
Prefer aBigInt/Decimalhelper or accumulate as strings/big.js.packages/data-flow/src/orchestrator.ts (2)
232-304: Possible race when multiple orchestrators write the same CSV
updatePerformanceDatareads the whole file, mutates a map, then rewrites it.
If several orchestrators (different chains) run in parallel, last‑writer‑wins will drop data.At minimum, include the
chainIdin the filename, e.g.:-this.performanceCsvPath = path.join(process.cwd(), "performance.csv"); +this.performanceCsvPath = path.join(process.cwd(), `performance-${this.chainId}.csv`);
603-655:console.timein production logs
console.time/timeEndwrites directly to stdout and bypasses the structuredlogger, producing noisy output when running under PM2/CloudWatch.
Rely entirely onperformance.now()and your structured logger instead.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/indexer/config.yaml(2 hunks)packages/data-flow/src/data-loader/handlers/application.handlers.ts(2 hunks)packages/data-flow/src/data-loader/handlers/donation.handlers.ts(2 hunks)packages/data-flow/src/data-loader/handlers/round.handlers.ts(3 hunks)packages/data-flow/src/orchestrator.ts(5 hunks)packages/processors/src/processors/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/allocated.handler.ts(3 hunks)packages/shared/src/index.ts(1 hunks)packages/shared/src/utils/performance-logger.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.ts`:
**/*.ts:
packages/shared/src/index.tspackages/data-flow/src/data-loader/handlers/round.handlers.tspackages/processors/src/processors/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/allocated.handler.tspackages/data-flow/src/data-loader/handlers/application.handlers.tspackages/shared/src/utils/performance-logger.tspackages/data-flow/src/data-loader/handlers/donation.handlers.tspackages/data-flow/src/orchestrator.ts
🧬 Code Graph Analysis (2)
packages/data-flow/src/data-loader/handlers/round.handlers.ts (1)
packages/shared/src/utils/performance-logger.ts (1)
performanceLogger(99-99)
packages/data-flow/src/data-loader/handlers/application.handlers.ts (1)
packages/shared/src/utils/performance-logger.ts (1)
performanceLogger(99-99)
🪛 ESLint
packages/shared/src/utils/performance-logger.ts
[error] 21-21: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🪛 GitHub Check: lint / Run Linters
packages/shared/src/utils/performance-logger.ts
[failure] 21-21:
Unexpected any. Specify a different type
🪛 GitHub Actions: Main Workflow
packages/shared/src/utils/performance-logger.ts
[error] 21-21: ESLint: Unexpected any. Specify a different type (@typescript-eslint/no-explicit-any)
🔇 Additional comments (12)
packages/shared/src/utils/performance-logger.ts (2)
24-43: Singleton implementation looks goodThe singleton pattern implementation for the
PerformanceLoggerclass is well-structured. The threshold of 500ms is reasonable for identifying significant performance issues.
97-99: Singleton export looks goodThe export of the singleton instance is correctly implemented.
packages/shared/src/index.ts (1)
2-2: Appropriate export of the performance loggerThe performance logger is correctly exported from the shared package, making it accessible to other modules.
packages/processors/src/processors/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/allocated.handler.ts (6)
47-47: Effective overall performance measurementThe addition of overall performance measurement at the beginning and end of the handler is well-implemented, providing valuable insight into the total processing time.
Also applies to: 130-132
55-62: Good implementation of round lookup timingThe timing for the round lookup operation is correctly implemented and provides useful performance data.
64-72: Good implementation of application lookup timingThe timing for the application lookup operation is correctly implemented and provides useful performance data.
76-83: Good implementation of token validation timingThe timing for the token validation operation is correctly implemented and provides useful performance data.
85-106: Good implementation of price calculation timingThe timing for the price calculation operation is correctly implemented and provides useful performance data.
108-112: Good implementation of metadata parsing timingThe timing for the metadata parsing operation is correctly implemented and provides useful performance data.
packages/data-flow/src/data-loader/handlers/round.handlers.ts (2)
2-2: Good import of the shared performance loggerThe import of the performance logger from the shared package is correctly implemented.
56-87:✅ Verification successful
Comprehensive performance logging for the round donation stats increment
The addition of performance logging to the
IncrementRoundDonationStatshandler is well-implemented, capturing start and end times, calculating duration, and logging detailed metrics.However, if the
logMetricmethod is updated to be asynchronous as suggested earlier, this code would need to be updated to handle the promise.Run the following script to check whether the performance logger is also used in similar handlers across the codebase:
🏁 Script executed:
#!/bin/bash # Check for consistent usage of performanceLogger across handlers rg -A 3 'performanceLogger.logMetric\(' --type tsLength of output: 1789
To verify whether
logMetricis async (and thus needsawait), let’s locate whereperformanceLoggeris defined and see itslogMetricsignature. Please run:
🏁 Script executed:
#!/bin/bash # 1. Find where performanceLogger is imported to get its source file rg -n "import .*performanceLogger" --type ts # 2. Search for its class (or factory) definition rg -n "class PerformanceLogger" --type ts rg -n "performanceLogger = new" --type tsLength of output: 617
I need to verify whether
logMetricis declared async. Please run:
🏁 Script executed:
#!/bin/bash # Show logMetric declaration and nearby lines rg -C3 "logMetric" -n packages/shared/src/utils/performance-logger.tsLength of output: 258
No await needed for performanceLogger.logMetric
We’ve confirmed that
performanceLogger.logMetricis declared as a synchronous method (voidreturn), so noawaitis required in this or any of the other handlers. Code changes can be approved as-is.apps/indexer/config.yaml (1)
138-150: Confirm side‑effects of disabling the additional networksAll MAINNET entries are now commented out, leaving only Arbitrum active.
This significantly narrows the indexer’s coverage. Please double‑check:
- Any downstream dashboards / lambdas that still expect data from the removed chains.
- CI/CD environments that may rely on those
ENVIO_*variables – unused variables often trigger IaC linter warnings.- PR objectives: since this is labelled Performance audit, consider re‑enabling at least one low‑traffic testnet to keep functional paths exercised.
| public logMetric(metric: PerformanceMetric): void { | ||
| if (metric.totalTime >= this.THRESHOLD_MS) { | ||
| const row = | ||
| [ | ||
| metric.timestamp, | ||
| metric.eventType, | ||
| metric.operation, | ||
| metric.duration.toFixed(2), | ||
| metric.totalTime.toFixed(2), | ||
| metric.blockNumber || "", | ||
| metric.transactionHash || "", | ||
| metric.chainId || "", | ||
| metric.roundId || "", | ||
| metric.applicationId || "", | ||
| metric.donorAddress || "", | ||
| metric.recipientAddress || "", | ||
| metric.amount || "", | ||
| metric.amountInUsd || "", | ||
| metric.uniqueDonorsCount || "", | ||
| metric.totalDonationsCount || "", | ||
| JSON.stringify(metric.details || {}), | ||
| ].join(",") + "\n"; | ||
|
|
||
| fs.appendFileSync(this.csvPath, row); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use asynchronous file operations for better performance
The logMetric method uses synchronous file operations (fs.appendFileSync), which could potentially block the event loop during high-load situations.
Consider using asynchronous file operations for better performance:
-public logMetric(metric: PerformanceMetric): void {
+public async logMetric(metric: PerformanceMetric): Promise<void> {
if (metric.totalTime >= this.THRESHOLD_MS) {
const row =
[
metric.timestamp,
metric.eventType,
metric.operation,
metric.duration.toFixed(2),
metric.totalTime.toFixed(2),
metric.blockNumber || "",
metric.transactionHash || "",
metric.chainId || "",
metric.roundId || "",
metric.applicationId || "",
metric.donorAddress || "",
metric.recipientAddress || "",
metric.amount || "",
metric.amountInUsd || "",
metric.uniqueDonorsCount || "",
metric.totalDonationsCount || "",
JSON.stringify(metric.details || {}),
].join(",") + "\n";
- fs.appendFileSync(this.csvPath, row);
+ try {
+ await fs.promises.appendFile(this.csvPath, row);
+ } catch (error) {
+ console.error(`Failed to log metric: ${error instanceof Error ? error.message : String(error)}`);
+ }
}
}Note that this change would require updating all calling code to handle the Promise return type.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public logMetric(metric: PerformanceMetric): void { | |
| if (metric.totalTime >= this.THRESHOLD_MS) { | |
| const row = | |
| [ | |
| metric.timestamp, | |
| metric.eventType, | |
| metric.operation, | |
| metric.duration.toFixed(2), | |
| metric.totalTime.toFixed(2), | |
| metric.blockNumber || "", | |
| metric.transactionHash || "", | |
| metric.chainId || "", | |
| metric.roundId || "", | |
| metric.applicationId || "", | |
| metric.donorAddress || "", | |
| metric.recipientAddress || "", | |
| metric.amount || "", | |
| metric.amountInUsd || "", | |
| metric.uniqueDonorsCount || "", | |
| metric.totalDonationsCount || "", | |
| JSON.stringify(metric.details || {}), | |
| ].join(",") + "\n"; | |
| fs.appendFileSync(this.csvPath, row); | |
| } | |
| } | |
| public async logMetric(metric: PerformanceMetric): Promise<void> { | |
| if (metric.totalTime >= this.THRESHOLD_MS) { | |
| const row = | |
| [ | |
| metric.timestamp, | |
| metric.eventType, | |
| metric.operation, | |
| metric.duration.toFixed(2), | |
| metric.totalTime.toFixed(2), | |
| metric.blockNumber || "", | |
| metric.transactionHash || "", | |
| metric.chainId || "", | |
| metric.roundId || "", | |
| metric.applicationId || "", | |
| metric.donorAddress || "", | |
| metric.recipientAddress || "", | |
| metric.amount || "", | |
| metric.amountInUsd || "", | |
| metric.uniqueDonorsCount || "", | |
| metric.totalDonationsCount || "", | |
| JSON.stringify(metric.details || {}), | |
| ].join(",") + "\n"; | |
| try { | |
| await fs.promises.appendFile(this.csvPath, row); | |
| } catch (error) { | |
| console.error( | |
| `Failed to log metric: ${ | |
| error instanceof Error ? error.message : String(error) | |
| }` | |
| ); | |
| } | |
| } | |
| } |
| interface PerformanceMetric { | ||
| timestamp: string; | ||
| eventType: string; | ||
| operation: string; | ||
| duration: number; | ||
| totalTime: number; | ||
| blockNumber?: number; | ||
| transactionHash?: string; | ||
| chainId?: number; | ||
| roundId?: string; | ||
| applicationId?: string; | ||
| donorAddress?: string; | ||
| recipientAddress?: string; | ||
| amount?: string; | ||
| amountInUsd?: string; | ||
| uniqueDonorsCount?: number; | ||
| totalDonationsCount?: number; | ||
| details?: Record<string, any>; | ||
| } |
There was a problem hiding this comment.
The interface structure looks good, but avoid any type
The PerformanceMetric interface provides comprehensive fields for tracking various performance metrics. However, there's an ESLint error related to the use of any type in the details field.
Fix the ESLint error by replacing any with a more specific type or using unknown:
- details?: Record<string, any>;
+ details?: Record<string, unknown>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| interface PerformanceMetric { | |
| timestamp: string; | |
| eventType: string; | |
| operation: string; | |
| duration: number; | |
| totalTime: number; | |
| blockNumber?: number; | |
| transactionHash?: string; | |
| chainId?: number; | |
| roundId?: string; | |
| applicationId?: string; | |
| donorAddress?: string; | |
| recipientAddress?: string; | |
| amount?: string; | |
| amountInUsd?: string; | |
| uniqueDonorsCount?: number; | |
| totalDonationsCount?: number; | |
| details?: Record<string, any>; | |
| } | |
| interface PerformanceMetric { | |
| timestamp: string; | |
| eventType: string; | |
| operation: string; | |
| duration: number; | |
| totalTime: number; | |
| blockNumber?: number; | |
| transactionHash?: string; | |
| chainId?: number; | |
| roundId?: string; | |
| applicationId?: string; | |
| donorAddress?: string; | |
| recipientAddress?: string; | |
| amount?: string; | |
| amountInUsd?: string; | |
| uniqueDonorsCount?: number; | |
| totalDonationsCount?: number; | |
| details?: Record<string, unknown>; | |
| } |
🧰 Tools
🪛 ESLint
[error] 21-21: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🪛 GitHub Check: lint / Run Linters
[failure] 21-21:
Unexpected any. Specify a different type
🪛 GitHub Actions: Main Workflow
[error] 21-21: ESLint: Unexpected any. Specify a different type (@typescript-eslint/no-explicit-any)
| // Get current application stats for logging | ||
| const application = await repository.getApplicationById(applicationId, chainId, roundId); | ||
|
|
||
| performanceLogger.logMetric({ | ||
| timestamp: new Date().toISOString(), | ||
| eventType: "Application", | ||
| operation: "IncrementApplicationDonationStats", | ||
| duration, | ||
| totalTime: duration, | ||
| chainId, | ||
| roundId, | ||
| applicationId, | ||
| amountInUsd, | ||
| uniqueDonorsCount: application?.uniqueDonorsCount, | ||
| totalDonationsCount: application?.totalDonationsCount, | ||
| details: { | ||
| totalAmountDonatedInUsd: application?.totalAmountDonatedInUsd, | ||
| status: application?.status, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extra DB round‑trip could negate the performance gain
Immediately after incrementApplicationDonationStats you fetch the application again to log stats.
That is an additional SELECT per donation:
const application = await repository.getApplicationById(...);If the repository method that increments stats can return the updated row (e.g. UPDATE … RETURNING * in SQL), you avoid the second query entirely.
| const startTime = performance.now(); | ||
| const { chainId, roundId, applicationId, amountInUsd } = changeset.args; | ||
| await repository.incrementApplicationDonationStats( | ||
| { chainId, roundId, id: applicationId }, | ||
| amountInUsd, | ||
| txConnection, | ||
| ); | ||
| const endTime = performance.now(); | ||
| const duration = endTime - startTime; | ||
|
|
There was a problem hiding this comment.
Import Node’s high‑resolution timer explicitly
performance.now() is used, but no import { performance } from "node:perf_hooks" (or "perf_hooks") is present.
Relying on the global may break type‑checking in strict projects and older Node versions.
+import { performance } from "node:perf_hooks";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const startTime = performance.now(); | |
| const { chainId, roundId, applicationId, amountInUsd } = changeset.args; | |
| await repository.incrementApplicationDonationStats( | |
| { chainId, roundId, id: applicationId }, | |
| amountInUsd, | |
| txConnection, | |
| ); | |
| const endTime = performance.now(); | |
| const duration = endTime - startTime; | |
| +import { performance } from "node:perf_hooks"; | |
| const startTime = performance.now(); | |
| const { chainId, roundId, applicationId, amountInUsd } = changeset.args; | |
| await repository.incrementApplicationDonationStats( | |
| { chainId, roundId, id: applicationId }, | |
| amountInUsd, | |
| txConnection, | |
| ); | |
| const endTime = performance.now(); | |
| const duration = endTime - startTime; |
| const startTime = performance.now(); | ||
| await repository.insertDonation(changeset.args.donation, txConnection); | ||
| const endTime = performance.now(); | ||
| const duration = endTime - startTime; | ||
|
|
||
| performanceLogger.logMetric({ | ||
| timestamp: new Date().toISOString(), | ||
| eventType: "Donation", | ||
| operation: "InsertDonation", | ||
| duration, | ||
| totalTime: duration, | ||
| blockNumber: Number(changeset.args.donation.blockNumber), |
There was a problem hiding this comment.
Same timer import issue as above
Add an explicit import of performance from "node:perf_hooks" here as well.
| amount: changeset.args.donation.amount.toString(), | ||
| amountInUsd: changeset.args.donation.amountInUsd, | ||
| details: { | ||
| tokenAddress: changeset.args.donation.tokenAddress, | ||
| amountInRoundMatchToken: changeset.args.donation.amountInRoundMatchToken.toString(), | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential loss of precision when logging amount
donation.amount is converted with .toString() ✔️,
but amountInUsd is coerced to number in later code. Large values or high‑precision decimals may be truncated:
-amountInUsd: changeset.args.donation.amountInUsd,
+amountInUsd: changeset.args.donation.amountInUsd?.toString(),The same applies in InsertManyDonations where a Number() cast is used inside reduce.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| amount: changeset.args.donation.amount.toString(), | |
| amountInUsd: changeset.args.donation.amountInUsd, | |
| details: { | |
| tokenAddress: changeset.args.donation.tokenAddress, | |
| amountInRoundMatchToken: changeset.args.donation.amountInRoundMatchToken.toString(), | |
| }, | |
| }); | |
| // … other code … | |
| someLogger.log({ | |
| amount: changeset.args.donation.amount.toString(), | |
| - amountInUsd: changeset.args.donation.amountInUsd, | |
| + amountInUsd: changeset.args.donation.amountInUsd?.toString(), | |
| details: { | |
| tokenAddress: changeset.args.donation.tokenAddress, | |
| amountInRoundMatchToken: changeset.args.donation.amountInRoundMatchToken.toString(), | |
| }, | |
| }); | |
| // … other code … |
| // Set up performance tracking | ||
| this.performanceCsvPath = path.join(process.cwd(), "performance.csv"); | ||
| this.initializePerformanceCsv(); | ||
| } | ||
|
|
||
| /** | ||
| * Initialize the performance CSV file with headers if it doesn't exist | ||
| */ | ||
| private initializePerformanceCsv(): void { | ||
| if (!fs.existsSync(this.performanceCsvPath)) { | ||
| const headers = | ||
| "timestamp,eventName,slowCount,avgDuration,maxDuration,minDuration,chainId\n"; | ||
| fs.writeFileSync(this.performanceCsvPath, headers); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Blocking FS I/O on the main event loop
initializePerformanceCsv, readPerformanceCsv, and writePerformanceCsv use synchronous fs.*Sync calls inside the hot path.
Under high throughput this can pause the entire orchestrator.
Consider:
- Switching to
fs.promiseswithawait. - Buffering summaries in memory and flushing on interval / graceful shutdown.
🤖 Linear
Closes PAR-XXX
Description
Checklist before requesting a review
Summary by CodeRabbit