cli: add instrumentation for side effects #158
Conversation
📝 WalkthroughWalkthroughThis pull request introduces instrumentation support into the apibara package. A new JSON release configuration file marks the change as a prerelease patch with instrumentation side effects. New functions for instrumentation— Changes
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
examples/cli/instrumentation.ts (1)
8-16: Logger factory implementation demonstrates good customizationThe logger implementation correctly demonstrates how to customize the default logger. The example of prefixing the indexer name with "cli-" shows a simple but effective customization pattern.
However, consider adding more documentation about the parameters received by the LoggerFactoryFn (indexer, indexers, preset) to help users understand what values they can expect.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
change/apibara-d61e7e81-39c1-43c0-99cd-cfac39828cbe.json(1 hunks)examples/cli/instrumentation.ts(1 hunks)packages/cli/src/rolldown/config.ts(2 hunks)packages/cli/src/rolldown/plugins/instrumentation.ts(1 hunks)packages/cli/src/runtime/internal/app.ts(2 hunks)packages/cli/src/runtime/start.ts(2 hunks)packages/cli/src/types/config.ts(1 hunks)packages/cli/src/types/virtual/instrumentation.d.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (16)
packages/cli/src/types/virtual/instrumentation.d.ts (1)
1-4: Type declarations established correctly for instrumentation moduleThis is a well-structured declaration file for a virtual module that will provide instrumentation capabilities. The types are correctly imported from "apibara/types" and the exports are properly typed with appropriate fallbacks to undefined.
change/apibara-d61e7e81-39c1-43c0-99cd-cfac39828cbe.json (1)
1-7: Change file correctly configured for prereleaseThe change file is properly formatted and includes all required fields. The feature is accurately described as adding instrumentation for side effects, and the change type is appropriately set as a prerelease with a patch-level dependent change.
packages/cli/src/rolldown/config.ts (2)
15-15: Import statement correctly addedThe instrumentation plugin is properly imported from the relative path.
91-91: Instrumentation plugin correctly integratedThe instrumentation plugin is properly added to the Rolldown configuration. The placement before other plugins is appropriate since instrumentation should typically be set up early in the build process.
examples/cli/instrumentation.ts (1)
1-7: Register function implementation provides good extensibilityThe register function is correctly typed as RegisterFn and implemented as an async function, which is appropriate for potentially asynchronous instrumentation setup tasks. The comment about registering side effects like OpenTelemetry or Sentry provides good guidance for users.
packages/cli/src/runtime/start.ts (2)
5-5: Added instrumentation importThis import brings in the instrumentation registration function which will be used during startup.
38-42: Good implementation of instrumentation registrationThe code correctly checks if the register function exists before attempting to use it, making this implementation optional and preventing potential runtime errors. The logging statements provide good visibility into the registration process.
packages/cli/src/runtime/internal/app.ts (3)
14-14: Added instrumentation logger importThis import brings in the custom logger from the instrumentation module.
60-60: Changed reporter declaration from const to letThis change allows the reporter to be reassigned if a custom logger is provided through instrumentation.
66-79: Well-implemented custom logger integrationThe implementation correctly:
- Checks if a custom logger is provided
- Creates a reporter using the custom logger with appropriate parameters
- Validates that the reporter has a log method before using it
- Falls back to the default logger if needed
This ensures that custom logging can be injected while maintaining backward compatibility.
packages/cli/src/types/config.ts (2)
14-15: Added RegisterFn type definitionThis type properly defines the instrumentation registration function as returning a Promise, ensuring it can handle asynchronous operations.
16-19: Updated logger type definitionThe new LoggerFactoryFn type properly defines the function signature for creating custom loggers with the expected parameters.
packages/cli/src/rolldown/plugins/instrumentation.ts (4)
1-12: Well-structured plugin imports and instrumentation path resolutionThe implementation correctly:
- Imports necessary modules
- Determines the instrumentation file path based on the current working directory
- Handles both TypeScript and JavaScript file extensions
This ensures the instrumentation file can be found regardless of the project's language choice.
13-22: Good handling of missing instrumentation fileThe code gracefully handles the case when no instrumentation file exists by providing undefined values for register and logger functions. This ensures the application works correctly even without custom instrumentation.
24-45: Well-documented approach to avoid rolldown warningsThe comments thoroughly explain the reasoning behind using require instead of import, providing examples of the warnings that would otherwise occur. This helps maintainers understand the implementation decision.
46-68: Robust implementation of instrumentation module loadingThe implementation:
- Creates a virtual module with proper exports
- Uses a try/catch block to handle potential errors during require
- Validates that the imported functions are actually functions before using them
- Silently handles errors to prevent application crashes
This ensures that the instrumentation functionality is robust and fault-tolerant.
fracek
left a comment
fracek
left a comment
There was a problem hiding this comment.
Looks good and I'm surprised it was so little code! Only one tiny detail and it's ready.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/cli/src/types/config.ts (1)
28-104: 💡 Verification agent🧩 Analysis chain
Verify the logger property removal impact.
The removal of the
loggerproperty from bothApibaraConfigandApibaraOptionsinterfaces suggests a restructuring of the logging functionality. Please ensure that existing code using these interfaces is updated appropriately.
🏁 Script executed:
#!/bin/bash # Search for any usage of the removed logger property in the codebase echo "Searching for usages of the 'logger' property in configurations..." rg -t ts "(ApibaraConfig|ApibaraOptions).*\{[^}]*logger[^}]*\}" --glob "!**/node_modules/**" --glob "!**/dist/**" echo "Searching for direct access to the logger property..." rg -t ts "\.(logger)\b" --glob "!**/node_modules/**" --glob "!**/dist/**"Length of output: 2957
Action Required: Update Logger Usage Across the Codebase
The removal of the
loggerproperty from theApibaraConfigandApibaraOptionsinterfaces now leaves several files still invoking methods onapibara.logger. Please update these references to align with the new logging architecture. Notable files with remaining logger usages include:
packages/cli/src/cli/commands/start.ts
- Calls to
apibara.logger.startandapibara.logger.error.packages/rolldown/plugins/instrumentation.ts
- Checks and assignments involving
instrumentation.logger.packages/cli/src/core/scan.ts
apibara.logger.debuginvocations.packages/cli/src/core/build/prod.ts
- Multiple calls:
apibara.logger.start,apibara.logger.success,apibara.logger.info, andapibara.logger.error.packages/cli/src/cli/commands/dev.ts
- Various diagnostics using
apibara.logger.Additional references in files such as
packages/cli/src/core/config/update.tspackages/cli/src/core/build/dev.tspackages/cli/src/core/build/build.tspackages/indexer/src/plugins/logger.tspackages/cli/src/core/build/prepare.tsPlease review and refactor these areas to either remove direct logger calls, use an updated logging mechanism, or ensure the new logger is injected appropriately.
🧹 Nitpick comments (1)
packages/cli/src/types/config.ts (1)
14-26: Consider adding JSDoc comments for the new types.The newly added types would benefit from JSDoc comments explaining their purpose and usage, especially since they're part of a new instrumentation feature.
+/** + * Function type for registering instrumentation side effects + */ export type RegisterFn = () => Promise<void>; +/** + * Function type for creating a custom console reporter based on indexer configuration + */ export type LoggerFactoryFn = ({ indexer, indexers, preset, }: LoggerFactoryArgs) => ConsolaReporter; +/** + * Arguments passed to the logger factory function + */ export type LoggerFactoryArgs = { indexer: string; indexers: string[]; preset?: string; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/cli/src/types/config.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (3)
packages/cli/src/types/config.ts (3)
14-15: New type for instrumentation registration looks good.The added
RegisterFntype properly defines a function signature for registering side effects as mentioned in the PR objectives, returning a Promise.
16-20: Type renaming and structure improvement looks good.The rename from
LoggerFactorytoLoggerFactoryFnbetter indicates its purpose as a function. The type now properly references the newLoggerFactoryArgstype, improving the code organization.
22-26: Properly implemented the suggested type for logger factory arguments.You've addressed fracek's feedback by creating a dedicated
LoggerFactoryArgstype for the parameters, making the code more maintainable and easier to extend in the future.
2 participants
No description provided.