cli: use default logger that includes indexer name #120
Conversation
📝 WalkthroughWalkthroughThis pull request introduces several changes to the Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@fracek has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
CodeRabbit Configuration File (
|
|
Looks like this for now |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
packages/cli/src/rollup/plugins/config.ts (1)
7-9: Consider build-time implicationsThe change from static JSON to dynamic import might affect build-time optimizations. Ensure that the configuration is properly tree-shaken and doesn't include unnecessary code in the final bundle.
Consider documenting the following aspects:
- When the configuration is evaluated (build-time vs runtime)
- How circular dependencies are prevented
- Impact on tree-shaking and bundle size
examples/cli/indexers/1-evm.indexer.ts (1)
16-17: Consider these improvements to the logging implementationWhile the implementation works, here are some suggestions to enhance it:
- Consider moving the logger initialization outside the transform function to avoid recreating it on every call.
- Use template literals for better readability and performance.
- Add more structured context to the log message.
Here's a suggested improvement:
+const logger = useLogger(); + export default defineIndexer(EvmStream)({ streamUrl: "https://ethereum.preview.apibara.org", finality: "accepted", startingCursor: { orderKey: 10_000_000n, }, filter: { header: "always", transactions: [{}], }, async transform({ endCursor }) { - const logger = useLogger(); - logger.info("Transforming block ", endCursor?.orderKey); + logger.info(`Transforming block`, { + blockNumber: endCursor?.orderKey, + finality: "accepted" + }); }, });packages/cli/src/runtime/internal/logger.ts (2)
11-19: Consider enhancing color differentiation for multiple indexersWith only 6 colors, multiple indexers could get assigned the same color, potentially causing confusion in environments with many indexers running simultaneously.
Consider:
- Expanding the color palette using RGB combinations
- Adding a brightness/saturation variation when colors repeat
const INDEXER_COLOR_MAP = [ colors.red, colors.green, colors.yellow, colors.blue, colors.magenta, colors.cyan, + colors.white, + // Add more variations + (text: string) => colors.red(colors.bold(text)), + (text: string) => colors.green(colors.bold(text)), ];
20-32: Consider mapping all possible log levelsThe current implementation only maps levels 0 and 1, but consola supports more log levels (0-5).
Consider mapping all levels for consistent coloring:
export const LEVEL_COLOR_MAP: { [k in LogLevel]?: string } = { + 2: "blue", // info + 3: "gray", // debug + 4: "gray", // trace + 5: "gray", // verbose };packages/cli/src/runtime/internal/app.ts (1)
45-47: Consider adding validation for custom logger configurationThe custom logger configuration could benefit from validation to ensure the returned reporter meets the required interface.
if (config.logger) { - reporter = config.logger({ indexer: indexerName, preset }); + const customReporter = config.logger({ indexer: indexerName, preset }); + if (!customReporter || typeof customReporter.log !== 'function') { + throw new Error('Custom logger must implement the ConsolaReporter interface'); + } + reporter = customReporter; }packages/cli/src/types/config.ts (1)
79-80: LGTM! Consider adding JSDoc commentsThe logger configuration is well-placed in the options interface. Consider adding JSDoc comments to document the expected behavior and default logger implementation.
+ /** + * Custom logger factory function. + * If not provided, a default logger that includes the indexer name will be used. + */ logger?: LoggerFactory;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
change/apibara-17aa4aa8-e6bb-419c-9c4a-65bee8c749cd.json(1 hunks)examples/cli/indexers/1-evm.indexer.ts(2 hunks)packages/cli/src/core/apibara.ts(0 hunks)packages/cli/src/rollup/plugins/config.ts(1 hunks)packages/cli/src/runtime/index.ts(1 hunks)packages/cli/src/runtime/internal/app.ts(2 hunks)packages/cli/src/runtime/internal/logger.ts(1 hunks)packages/cli/src/types/config.ts(4 hunks)
💤 Files with no reviewable changes (1)
- packages/cli/src/core/apibara.ts
✅ Files skipped from review due to trivial changes (1)
- change/apibara-17aa4aa8-e6bb-419c-9c4a-65bee8c749cd.json
🔇 Additional comments (12)
packages/cli/src/runtime/index.ts (1)
2-2: LGTM! Verify logger implementation.
The export statement is well-structured and aligns with the PR objective to implement default logging capabilities.
Let's verify the logger implementation exists:
✅ Verification successful
Export of createLogger is properly implemented
The verification confirms that:
- The
createLoggerfunction is correctly implemented inpackages/cli/src/runtime/internal/logger.ts - No duplicate implementations exist in the codebase
- The export path in the index file correctly points to the implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the logger implementation exists and exports createLogger
# Check if the logger file exists and exports createLogger
ast-grep --pattern 'export function createLogger' packages/cli/src/runtime/internal/logger.ts
# Check for any potential duplicate logger implementations
rg -g '!node_modules' -g '*.ts' 'export.*createLogger'
Length of output: 345
packages/cli/src/rollup/plugins/config.ts (2)
7-9: Consider type safety for imported configuration
The imported configuration should match the expected type structure for the logger configuration.
7-9: Verify config file existence and error handling
While the dynamic import of configuration is a good architectural choice, we should ensure proper error handling for missing or invalid configuration files.
Let's verify the config file handling:
✅ Verification successful
Let me gather more context about the configuration handling:
Let me check one more aspect about the configuration loading and error handling:
Based on the gathered information, I can now provide the final response:
Configuration handling is properly implemented
The dynamic import of configuration is safe and well-architected:
- Configuration is loaded through
c12library which provides robust config loading and watching capabilities - Error handling is handled at the config loader level through
c12 - The virtual module is part of the build process and the config file path is guaranteed to exist when the code runs
- The system supports both static loading and watch mode for development
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how config file paths are resolved and validate error handling
# Check for config file path resolution
rg -A 5 "_c12.configFile" --type ts
# Look for error handling around config loading
ast-grep --pattern 'try {
$$$
import * as projectConfig from $path
$$$
} catch'
Length of output: 513
Script:
#!/bin/bash
# Check the broader context of config file handling
# Look for the plugin implementation and surrounding code
rg -B 5 -A 10 "virtual.*config" --type ts
# Check how c12 config is initialized
rg -B 3 -A 3 "_c12" --type ts
# Look for error handling patterns around configuration
ast-grep --pattern 'function $name($_) {
$$$
apibara.options._c12
$$$
}'
Length of output: 4013
Script:
#!/bin/bash
# Check configuration loading and validation
# Look for configuration loading and validation
rg -B 5 -A 10 "loadConfig|createConfig" --type ts
# Check for error handling in the loader
ast-grep --pattern 'try {
$$$
loadConfig
$$$
} catch'
# Look for configuration type definitions
rg -A 10 "interface ApibaraConfig" --type ts
Length of output: 3055
examples/cli/indexers/1-evm.indexer.ts (2)
3-3: LGTM: Clean import statement
The import statement is properly placed and follows the project's conventions.
16-17: Verify consistent logger usage across indexers
Let's ensure the logger implementation is consistent across other indexers and properly handles errors.
✅ Verification successful
Let me gather more information about the logger implementation and usage patterns.
Let me check the complete logger implementation to ensure proper error handling.
Logger implementation is consistent and properly handles errors
The verification shows that:
- Both indexers (EVM and Starknet) use the logger consistently for block transformation logging
- The logger plugin is properly implemented with:
- Error handling for missing logger context
- Configurable reporter support
- Proper initialization in the plugin lifecycle
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check logger usage patterns across indexers
# Check for other indexer files and their logger usage
echo "Checking logger usage patterns in indexers:"
rg -l "useLogger" examples/ --type ts
# Check for error handling in logger implementation
echo "Checking logger implementation:"
ast-grep --pattern 'useLogger = () => {
$$$
}'
Length of output: 341
Script:
#!/bin/bash
# Check actual logger usage in both indexer files
echo "Checking logger usage in EVM indexer:"
rg "logger\." -A 2 examples/cli/indexers/1-evm.indexer.ts
echo "\nChecking logger usage in Starknet indexer:"
rg "logger\." -A 2 examples/cli/indexers/2-starknet.indexer.ts
# Look for logger implementation
echo "\nSearching for logger implementation:"
rg "export.*useLogger" -A 5
Length of output: 1000
Script:
#!/bin/bash
# Get the complete logger plugin implementation
echo "Complete logger plugin implementation:"
cat packages/indexer/src/plugins/logger.ts
# Check if there are any other logger-related files
echo "\nChecking for other logger-related files:"
fd logger packages/indexer/src/
Length of output: 1159
packages/cli/src/runtime/internal/logger.ts (1)
1-10: LGTM: Imports are well-organized and necessary
All imported types and utilities are used appropriately in the implementation.
packages/cli/src/runtime/internal/app.ts (4)
2-2: LGTM: Import statements are well-organized
The new imports properly separate the external logger plugin from the local logger creation utility.
Also applies to: 7-7
44-47: LGTM: Logger configuration implements required functionality
The implementation correctly sets up a default logger with indexer context while maintaining flexibility for custom logging configurations.
49-52: LGTM: Plugin configuration properly integrates logger
The implementation correctly preserves existing plugins while adding the logger plugin with proper configuration.
44-52: Verify logger usage across the codebase
Let's ensure the new logging setup is compatible with existing usage patterns.
✅ Verification successful
Let me gather more specific information about the logger usage.
Let me check the logger type definition and any other related configurations.
Let me check one final thing about the logger plugin implementation.
Changes to logger configuration are safe and maintain compatibility
The verification shows that the changes are safe because:
- The logger configuration follows the existing type contract (
LoggerFactory) - Example indexers use the logger through
useLogger()hook, which is unaffected by these changes - The logger plugin integration maintains backward compatibility by accepting both default and custom loggers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing custom logger configurations
rg -l "logger.*config" --type ts
# Check for createLogger usage
ast-grep --pattern 'createLogger($_)'
Length of output: 113
Script:
#!/bin/bash
# Check the content of the config file that uses logger configuration
cat packages/cli/src/core/config/update.ts
# Search for logger plugin usage
rg "logger\(" -A 2 --type ts
# Look for logger type definitions and interfaces
rg "interface.*Logger|type.*Logger" --type ts
Length of output: 823
Script:
#!/bin/bash
# Check the logger type definition
cat packages/cli/src/types/config.ts
# Look for createLogger implementation
fd createLogger --type f --exec cat {}
# Check for any other logger-related imports or configurations
rg "import.*logger|from.*logger" --type ts
Length of output: 2864
Script:
#!/bin/bash
# Check the logger plugin implementation
rg -A 5 "export.*createLogger" --type ts
# Check the logger plugin usage in examples
cat examples/cli/indexers/1-evm.indexer.ts
cat examples/cli/indexers/2-starknet.indexer.ts
Length of output: 2560
packages/cli/src/types/config.ts (2)
15-18: Well-designed type definition for logger factory!
The LoggerFactory type is well-structured with:
- Required
indexerparameter ensuring indexer name is always available - Optional
presetparameter providing flexibility for different logging configurations - Clear return type using
ConsolaReporterfor structured logging
34-34: Verify logger configuration inheritance with presets
The optional logger configuration looks good, but let's verify how it interacts with the preset system.
✅ Verification successful
Logger configuration works correctly with presets
The verification shows that logger configuration properly integrates with the preset system:
- The preset resolver (
preset.resolver.ts) usesdefuto merge preset options with base options, which ensures proper inheritance of logger configuration - The runtime correctly handles logger configuration with presets by passing both indexer name and preset to the logger factory
- The preset's logger configuration can be overridden by the base config as expected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any preset configurations that might conflict with or override the logger configuration
# Search for preset configurations that include logger settings
rg -l "preset.*logger" --type ts
# Search for any existing logger configurations in preset files
ast-grep --pattern 'logger[?]: $_'
Length of output: 71
Script:
#!/bin/bash
# Let's try a more comprehensive search strategy
# Search for preset-related files and configurations
fd "preset" --type f --extension ts
# Search for logger configuration patterns
rg "logger" --type ts -B 2 -A 2
# Look for preset configuration interfaces or types
ast-grep --pattern 'interface $_ {
$$$
}'
# Search for configuration inheritance patterns
ast-grep --pattern 'extends $_Config'
Length of output: 102160
Script:
#!/bin/bash
# Let's search for preset and config inheritance patterns more specifically
# Search for preset-related configuration code
rg "preset.*config" --type ts -B 2 -A 2
# Look for preset configuration handling
ast-grep --pattern 'preset?: $_'
# Search for logger configuration in preset files
rg "logger.*preset" --type ts -B 2 -A 2
# Check preset resolver implementation
cat packages/cli/src/core/config/resolvers/preset.resolver.ts
Length of output: 1188
| export function createLogger(indexer: string, preset?: string) { | ||
| return new DefaultReporter(indexer, preset); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation to factory function
The factory function should validate its inputs to prevent runtime errors.
export function createLogger(indexer: string, preset?: string) {
+ if (!indexer || typeof indexer !== 'string') {
+ throw new Error('Indexer name must be a non-empty string');
+ }
+ if (preset !== undefined && typeof preset !== 'string') {
+ throw new Error('Preset must be a string when provided');
+ }
return new DefaultReporter(indexer, preset);
}📝 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.
| export function createLogger(indexer: string, preset?: string) { | |
| return new DefaultReporter(indexer, preset); | |
| } | |
| export function createLogger(indexer: string, preset?: string) { | |
| if (!indexer || typeof indexer !== 'string') { | |
| throw new Error('Indexer name must be a non-empty string'); | |
| } | |
| if (preset !== undefined && typeof preset !== 'string') { | |
| throw new Error('Preset must be a string when provided'); | |
| } | |
| return new DefaultReporter(indexer, preset); | |
| } |
| class DefaultReporter implements ConsolaReporter { | ||
| private tag: string; | ||
|
|
||
| constructor(indexer: string, preset?: string) { | ||
| const color = | ||
| INDEXER_COLOR_MAP[murmurHash(indexer) % INDEXER_COLOR_MAP.length]; | ||
|
|
||
| const tag = `[${indexer}${preset ? `/${preset}` : ""}]`; | ||
| this.tag = color(tag); | ||
| } | ||
|
|
||
| log(logObj: LogObject, ctx: { options: ConsolaOptions }) { | ||
| const { args } = logObj; | ||
| const typeColor = | ||
| TYPE_COLOR_MAP[logObj.type] || LEVEL_COLOR_MAP[logObj.level] || "gray"; | ||
|
|
||
| const type = getColor(typeColor as ColorName, "white")(logObj.type); | ||
| console.log(`${this.tag} ${type}`, ...args); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and environment flexibility
The current implementation has a few areas that could be improved for robustness:
- Add error handling for invalid log objects
- Consider environment-specific logging (e.g., CI, production)
log(logObj: LogObject, ctx: { options: ConsolaOptions }) {
+ if (!logObj || typeof logObj !== 'object') {
+ console.error('Invalid log object received');
+ return;
+ }
+
const { args } = logObj;
const typeColor =
TYPE_COLOR_MAP[logObj.type] || LEVEL_COLOR_MAP[logObj.level] || "gray";
const type = getColor(typeColor as ColorName, "white")(logObj.type);
- console.log(`${this.tag} ${type}`, ...args);
+ const output = `${this.tag} ${type}`;
+
+ // Allow for different output methods based on environment
+ if (process.env.CI) {
+ // CI-specific formatting
+ console.log(`::debug::${output}`, ...args);
+ } else {
+ console.log(output, ...args);
+ }
}📝 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.
| class DefaultReporter implements ConsolaReporter { | |
| private tag: string; | |
| constructor(indexer: string, preset?: string) { | |
| const color = | |
| INDEXER_COLOR_MAP[murmurHash(indexer) % INDEXER_COLOR_MAP.length]; | |
| const tag = `[${indexer}${preset ? `/${preset}` : ""}]`; | |
| this.tag = color(tag); | |
| } | |
| log(logObj: LogObject, ctx: { options: ConsolaOptions }) { | |
| const { args } = logObj; | |
| const typeColor = | |
| TYPE_COLOR_MAP[logObj.type] || LEVEL_COLOR_MAP[logObj.level] || "gray"; | |
| const type = getColor(typeColor as ColorName, "white")(logObj.type); | |
| console.log(`${this.tag} ${type}`, ...args); | |
| } | |
| } | |
| class DefaultReporter implements ConsolaReporter { | |
| private tag: string; | |
| constructor(indexer: string, preset?: string) { | |
| const color = | |
| INDEXER_COLOR_MAP[murmurHash(indexer) % INDEXER_COLOR_MAP.length]; | |
| const tag = `[${indexer}${preset ? `/${preset}` : ""}]`; | |
| this.tag = color(tag); | |
| } | |
| log(logObj: LogObject, ctx: { options: ConsolaOptions }) { | |
| if (!logObj || typeof logObj !== 'object') { | |
| console.error('Invalid log object received'); | |
| return; | |
| } | |
| const { args } = logObj; | |
| const typeColor = | |
| TYPE_COLOR_MAP[logObj.type] || LEVEL_COLOR_MAP[logObj.level] || "gray"; | |
| const type = getColor(typeColor as ColorName, "white")(logObj.type); | |
| const output = `${this.tag} ${type}`; | |
| // Allow for different output methods based on environment | |
| if (process.env.CI) { | |
| // CI-specific formatting | |
| console.log(`::debug::${output}`, ...args); | |
| } else { | |
| console.log(output, ...args); | |
| } | |
| } | |
| } |
|
Updated to look like this. Notice how the shorter indexer name is padded to align nicely
|
fracek
force-pushed
the
indexer-logger
branch
from
a662bfd to
cb99387
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
examples/cli/indexers/2-starknet.indexer.ts (2)
Line range hint
39-42: Clean up or implement the commented code.The commented writer code appears to be a placeholder. Either implement the block data insertion or remove the commented code if it's no longer needed.
Would you like me to help implement the block data insertion logic or create an issue to track this task?
Line range hint
36-43: Consider adding error handling.The transform function should include error handling to gracefully handle potential issues with block data or database operations.
Consider wrapping the transform logic in a try-catch block:
async transform({ endCursor, block: { header }, context }) { const logger = useLogger(); + try { logger.info("Transforming block ", endCursor?.orderKey); const { writer } = useSink({ context }); // writer.insert([{ // number: header?.blockNumber.toString(), // hash: header?.blockHash, // }]) + } catch (error) { + logger.error("Failed to transform block", { error, blockNumber: header?.blockNumber }); + throw error; + } },packages/cli/src/runtime/internal/app.ts (1)
44-55: Consider adding error handling and reducing parameter duplicationThe logger configuration looks good overall, but could benefit from some improvements:
- Add error handling for custom logger initialization
- Reduce parameter duplication using an object
Consider refactoring to:
+ const loggerParams = { + indexer: indexerName, + preset, + indexers: availableIndexers, + }; + let reporter: ConsolaReporter = createLogger({ - indexer: indexerName, - preset, - indexers: availableIndexers, + ...loggerParams }); if (config.logger) { + try { reporter = config.logger({ - indexer: indexerName, - preset, - indexers: availableIndexers, + ...loggerParams }); + } catch (error) { + console.warn('Custom logger initialization failed, falling back to default logger', error); + } }packages/cli/src/runtime/internal/logger.ts (2)
11-34: Enhance type safety for color mappingsWhile the color mappings are well-organized, we can improve type safety to prevent potential runtime errors.
Consider applying these changes:
-const INDEXER_COLOR_MAP = [ +const INDEXER_COLOR_MAP: ((str: string) => string)[] = [ colors.red, colors.green, colors.yellow, colors.blue, colors.magenta, colors.cyan, ]; -const TYPE_COLOR_MAP: { [k in LogType]?: string } = { +const TYPE_COLOR_MAP: { [k in LogType]?: ColorName } = { info: "cyan", fail: "red", success: "green", ready: "green", start: "magenta", }; -const LEVEL_COLOR_MAP: { [k in LogLevel]?: string } = { +const LEVEL_COLOR_MAP: { [k in LogLevel]?: ColorName } = { };
65-71: Add JSDoc documentation to the factory functionAdding documentation would improve the API's usability.
Consider adding this documentation:
+/** + * Creates a new logger instance with the specified configuration. + * @param {Object} config - The logger configuration + * @param {string} config.indexer - The name of the current indexer + * @param {string[]} config.indexers - List of all indexer names for padding calculation + * @param {string} [config.preset] - Optional preset name to be included in the log tag + * @returns {ConsolaReporter} A configured logger instance + */ export function createLogger({ indexer, indexers, preset, }: { indexer: string; indexers: string[]; preset?: string }) { return new DefaultReporter(indexer, indexers, preset); }packages/cli/src/types/config.ts (1)
15-18: LGTM! Consider adding JSDoc documentation.The LoggerFactory type definition is well-structured and aligns with the PR objectives. The inclusion of both individual indexer name and the full indexers array provides good context for logging.
Consider adding JSDoc documentation to explain the purpose and parameters:
+/** + * Factory function type for creating a logger instance + * @param options.indexer - Name of the current indexer + * @param options.indexers - Array of all available indexer names + * @param options.preset - Optional logging preset configuration + * @returns A Consola reporter instance + */ export type LoggerFactory = ({ indexer, preset, }: { indexer: string; indexers: string[]; preset?: string }) => ConsolaReporter;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
change/apibara-17aa4aa8-e6bb-419c-9c4a-65bee8c749cd.json(1 hunks)examples/cli/indexers/1-evm.indexer.ts(2 hunks)examples/cli/indexers/2-starknet.indexer.ts(1 hunks)packages/cli/src/core/apibara.ts(0 hunks)packages/cli/src/rollup/plugins/config.ts(1 hunks)packages/cli/src/runtime/index.ts(1 hunks)packages/cli/src/runtime/internal/app.ts(2 hunks)packages/cli/src/runtime/internal/logger.ts(1 hunks)packages/cli/src/types/config.ts(4 hunks)
💤 Files with no reviewable changes (1)
- packages/cli/src/core/apibara.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- examples/cli/indexers/1-evm.indexer.ts
- packages/cli/src/runtime/index.ts
- packages/cli/src/rollup/plugins/config.ts
- change/apibara-17aa4aa8-e6bb-419c-9c4a-65bee8c749cd.json
🔇 Additional comments (7)
packages/cli/src/runtime/internal/app.ts (3)
2-2: LGTM: Import statements are properly organized
The new imports for logger functionality are correctly structured, with type imports appropriately separated.
Also applies to: 7-7
57-60: LGTM: Plugin configuration is well-structured
The plugin configuration correctly preserves existing plugins while adding the logger plugin. The null coalescing operator handles the case where no plugins were previously defined.
44-60: Verify logger output format across different indexers
The implementation aligns with the PR objectives to include indexer names in logs. Let's verify the logger output format across different indexers.
✅ Verification successful
Logger implementation is correctly integrated across the codebase
The implementation is properly structured and consistent:
- The logger plugin is correctly exported from
@apibara/indexer/plugins/logger - The
ConsolaReportertype is properly shared across packages - The custom logger configuration is properly handled in
app.ts - Example indexers demonstrate correct usage of the logger through
useLogger - The
DefaultReporterimplementation inlogger.tscorrectly includes indexer name in the tag
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for logger usage and configuration patterns across the codebase
# Check for other logger configurations that might need similar updates
rg -A 5 "createLogger|ConsolaReporter" --type typescript
# Check for existing logger plugin usage patterns
ast-grep --pattern 'logger({ $$ })'
Length of output: 131
Script:
#!/bin/bash
# Let's try with different file extensions and patterns
# Check for logger configurations
rg -A 5 "createLogger|ConsolaReporter" --type ts
# Look for logger plugin usage
rg -A 5 "logger\(\{" --type ts
# Search for logger-related imports
rg "import.*logger" --type ts
# Check for logger type definitions
rg "type.*ConsolaReporter" --type ts
Length of output: 6365
packages/cli/src/runtime/internal/logger.ts (2)
1-10: LGTM! Well-structured imports with proper type definitions.
The imports are clean, properly typed, and include all necessary dependencies for the logging implementation.
55-62: Previous suggestion for enhanced error handling still applies
The log method could benefit from improved error handling and environment-specific logging as previously suggested.
packages/cli/src/types/config.ts (2)
34-34: LGTM! Logger configuration properly integrated.
The logger property is correctly added as an optional field, maintaining backward compatibility while enabling configuration through config files.
79-80: LGTM! Verify logger usage across the codebase.
The logger property is properly added to ApibaraOptions with clear sectioning. Let's verify its consistent usage across the codebase.
✅ Verification successful
Let me try different commands to gather more information about logger usage.
Let me check the logger implementation to complete the verification.
Logger implementation and usage are consistent across the codebase
The logger property is properly integrated with:
- Correct type definition of
LoggerFactoryin config.ts - Consistent implementation in the logger plugin (packages/indexer/src/plugins/logger.ts)
- Proper usage in runtime setup (packages/cli/src/runtime/internal/app.ts)
- Well-defined default reporter implementation (packages/cli/src/runtime/internal/logger.ts)
- Consistent usage patterns in example indexers using
useLogger()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent logger usage patterns across the codebase
# Check for logger property usage
echo "Checking logger property usage patterns:"
rg -A 2 "logger\?:" --type typescript
# Check for LoggerFactory type imports
echo "Checking LoggerFactory type imports:"
rg "import.*LoggerFactory.*from.*config" --type typescript
# Check for createLogger function implementations
echo "Checking logger creation patterns:"
ast-grep --pattern 'createLogger($$$)'
Length of output: 810
Script:
#!/bin/bash
# Search for logger-related patterns with ripgrep
echo "Searching for logger-related patterns:"
rg "logger" -t ts -A 2
# Search for LoggerFactory type definition and usage
echo "Searching for LoggerFactory type:"
rg "LoggerFactory" -t ts -A 2
# Find files that might contain logger configuration
echo "Finding logger configuration files:"
fd -e ts -e js logger
Length of output: 8884
Script:
#!/bin/bash
# Check logger implementation
echo "Checking logger implementation:"
cat packages/cli/src/runtime/internal/logger.ts
# Check logger plugin implementation
echo "Checking logger plugin implementation:"
cat packages/indexer/src/plugins/logger.ts
# Check LoggerFactory type definition
echo "Checking LoggerFactory type definition:"
rg -A 10 "export type LoggerFactory"
Length of output: 3607
| async transform({ endCursor, block: { header }, context }) { | ||
| const logger = useLogger(); | ||
| logger.info("Transforming block ", endCursor); | ||
| logger.info("Transforming block ", endCursor?.orderKey); |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent logging patterns found across indexers
The verification reveals inconsistencies in block logging patterns:
examples/cli/indexers/*: UsesendCursor?.orderKeyexamples/starknet-indexer/src/indexer.tsandexamples/indexer/src/indexer.ts: Usesheader?.blockNumber
Additionally, there are minor format inconsistencies:
- Some use a space after "block" (
,) - Others don't use a space (
,)
🔗 Analysis chain
LGTM! Verify logging format consistency.
The change to log only the orderKey instead of the entire endCursor object makes the logs more concise and focused.
Let's verify the logging format consistency across other indexers:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check logging patterns in other indexer files
# Expected: Similar logging patterns using orderKey
# Search for similar logging patterns in other indexer files
rg -A 2 "Transforming block" --type ts
Length of output: 889
| constructor(indexer: string, indexers: string[], preset?: string) { | ||
| const color = | ||
| INDEXER_COLOR_MAP[murmurHash(indexer) % INDEXER_COLOR_MAP.length]; | ||
|
|
||
| const longestIndexerName = Math.max( | ||
| ...indexers.map((i) => i.length), | ||
| indexer.length, | ||
| ); | ||
|
|
||
| const paddedIndexer = indexer | ||
| .padEnd(longestIndexerName, " ") | ||
| .slice(0, Math.min(longestIndexerName, MAX_INDEXER_NAME_LENGTH)); | ||
|
|
||
| const tag = `${paddedIndexer}${preset ? `/${preset}` : ""} |`; | ||
| this.tag = color(tag); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and error handling to constructor
The constructor should validate its inputs to prevent potential runtime issues.
Consider applying these changes:
constructor(indexer: string, indexers: string[], preset?: string) {
+ if (!indexer || typeof indexer !== 'string') {
+ throw new Error('Indexer name must be a non-empty string');
+ }
+ if (!Array.isArray(indexers)) {
+ throw new Error('Indexers must be an array');
+ }
+ if (preset !== undefined && typeof preset !== 'string') {
+ throw new Error('Preset must be a string when provided');
+ }
+
const color =
INDEXER_COLOR_MAP[murmurHash(indexer) % INDEXER_COLOR_MAP.length];📝 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.
| constructor(indexer: string, indexers: string[], preset?: string) { | |
| const color = | |
| INDEXER_COLOR_MAP[murmurHash(indexer) % INDEXER_COLOR_MAP.length]; | |
| const longestIndexerName = Math.max( | |
| ...indexers.map((i) => i.length), | |
| indexer.length, | |
| ); | |
| const paddedIndexer = indexer | |
| .padEnd(longestIndexerName, " ") | |
| .slice(0, Math.min(longestIndexerName, MAX_INDEXER_NAME_LENGTH)); | |
| const tag = `${paddedIndexer}${preset ? `/${preset}` : ""} |`; | |
| this.tag = color(tag); | |
| } | |
| constructor(indexer: string, indexers: string[], preset?: string) { | |
| if (!indexer || typeof indexer !== 'string') { | |
| throw new Error('Indexer name must be a non-empty string'); | |
| } | |
| if (!Array.isArray(indexers)) { | |
| throw new Error('Indexers must be an array'); | |
| } | |
| if (preset !== undefined && typeof preset !== 'string') { | |
| throw new Error('Preset must be a string when provided'); | |
| } | |
| const color = | |
| INDEXER_COLOR_MAP[murmurHash(indexer) % INDEXER_COLOR_MAP.length]; | |
| const longestIndexerName = Math.max( | |
| ...indexers.map((i) => i.length), | |
| indexer.length, | |
| ); | |
| const paddedIndexer = indexer | |
| .padEnd(longestIndexerName, " ") | |
| .slice(0, Math.min(longestIndexerName, MAX_INDEXER_NAME_LENGTH)); | |
| const tag = `${paddedIndexer}${preset ? `/${preset}` : ""} |`; | |
| this.tag = color(tag); | |
| } |
fracek
force-pushed
the
indexer-logger
branch
from
cb99387 to
dd272cc
Compare
fracek
force-pushed
the
indexer-logger
branch
from
dd272cc to
c8ddd39
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/cli/src/runtime/internal/logger.ts (3)
11-31: Consider enhancing constant definitionsThe color mappings are well-structured, but could benefit from additional safeguards and documentation.
Consider applying these improvements:
+/** Colors used for different indexers, cycled using murmur hash */ -const INDEXER_COLOR_MAP = [ +const INDEXER_COLOR_MAP = Object.freeze([ colors.red, colors.green, colors.yellow, colors.blue, colors.magenta, colors.cyan, -]; +]); +/** Maps log types to their respective colors for visual distinction */ -const TYPE_COLOR_MAP: { [k in LogType]?: string } = { +const TYPE_COLOR_MAP: Readonly<{ [k in LogType]?: string }> = { info: "cyan", fail: "red", success: "green", ready: "green", start: "magenta", };
33-33: Document the rationale for maximum indexer name lengthThe magic number 20 for MAX_INDEXER_NAME_LENGTH would benefit from documentation explaining why this specific limit was chosen.
Consider adding a comment:
+// Maximum length for indexer names to maintain consistent log formatting +// and prevent excessive padding while ensuring readability const MAX_INDEXER_NAME_LENGTH = 20;
38-52: Extract padding logic to improve readabilityThe constructor contains complex padding logic that could be extracted into a separate method for better maintainability.
Consider refactoring like this:
+ private formatIndexerName(indexer: string, indexers: string[], preset?: string): string { + const presetLength = preset ? preset.length : 0; + const longestIndexerName = + Math.max(...indexers.map((i) => i.length), indexer.length) + presetLength; + + return `${indexer}${preset ? `:${preset} ` : ""}` + .padEnd(longestIndexerName, " ") + .slice(0, Math.min(longestIndexerName, MAX_INDEXER_NAME_LENGTH)); + } constructor(indexer: string, indexers: string[], preset?: string) { const color = INDEXER_COLOR_MAP[murmurHash(indexer) % INDEXER_COLOR_MAP.length]; - const presetLength = preset ? preset.length : 0; - const longestIndexerName = - Math.max(...indexers.map((i) => i.length), indexer.length) + presetLength; - const paddedIndexer = `${indexer}${preset ? `:${preset} ` : ""}` - .padEnd(longestIndexerName, " ") - .slice(0, Math.min(longestIndexerName, MAX_INDEXER_NAME_LENGTH)); + const paddedIndexer = this.formatIndexerName(indexer, indexers, preset); this.tag = color(`${paddedIndexer} |`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
change/apibara-17aa4aa8-e6bb-419c-9c4a-65bee8c749cd.json(1 hunks)examples/cli/indexers/1-evm.indexer.ts(2 hunks)examples/cli/indexers/2-starknet.indexer.ts(1 hunks)packages/cli/src/core/apibara.ts(0 hunks)packages/cli/src/rollup/plugins/config.ts(1 hunks)packages/cli/src/runtime/index.ts(1 hunks)packages/cli/src/runtime/internal/app.ts(2 hunks)packages/cli/src/runtime/internal/logger.ts(1 hunks)packages/cli/src/types/config.ts(4 hunks)
💤 Files with no reviewable changes (1)
- packages/cli/src/core/apibara.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/cli/src/runtime/index.ts
- examples/cli/indexers/1-evm.indexer.ts
- examples/cli/indexers/2-starknet.indexer.ts
- packages/cli/src/types/config.ts
- packages/cli/src/rollup/plugins/config.ts
- packages/cli/src/runtime/internal/app.ts
- change/apibara-17aa4aa8-e6bb-419c-9c4a-65bee8c749cd.json
🔇 Additional comments (3)
packages/cli/src/runtime/internal/logger.ts (3)
1-10: LGTM! Dependencies and types are well-defined.
The imports are clean and properly typed, with clear separation between type imports and runtime dependencies.
54-61: Add error handling for log arguments
The log method should handle potential edge cases with the log arguments.
The previous review comment about error handling is still applicable here. Consider implementing the suggested changes for robust error handling and environment-specific logging.
64-70: Add input validation to factory function
The previous review comment about adding input validation to the factory function is still applicable here. Consider implementing the suggested validation checks.
jaipaljadeja
left a comment
jaipaljadeja
left a comment
There was a problem hiding this comment.
This is so good!! I liked this whole logging system.
Thanks a lot.
2 participants
No description provided.