-
Notifications
You must be signed in to change notification settings - Fork 129
chore(rivetkit): log runner id with actor logs #3360
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(rivetkit): log runner id with actor logs #3360
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
Code ReviewThanks for this PR! The change adds runner ID logging to actor logs, which will be valuable for debugging and tracing. Here's my feedback: Positive Observations ✓
Concerns & Suggestions1. Potential Race ConditionIssue: The Looking at the code flow:
Recommendation: This appears intentional based on the fallback to
2. Type SafetyIssue: The return type Recommendation: getExtraActorLogParams(): Record<string, string> {
return { runnerId: String(this.#runner.runnerId ?? "-") };
}This ensures type safety even if 3. Missing TestsIssue: No test coverage for the new functionality. Recommendation: Consider adding a test in
4. DocumentationMinor: The JSDoc comment is brief. Recommendation: Expand the documentation: /**
* Extra properties to add to logs for each actor.
* Called during actor initialization to augment log context.
* Values may be placeholders if not yet available (e.g., before runner connection).
*/
getExtraActorLogParams?(): Record<string, string>;Questions
VerdictThe core implementation is solid and follows good patterns. The main concern is ensuring users understand that runnerId might initially be "-" in logs. Consider adding clarifying comments or documentation. Approval Status: ✅ Approved with minor suggestions Review generated by Claude Code |
4b3360a to
b33ce24
Compare
6f6affa to
7e5b57a
Compare
Code ReviewThis PR adds runner ID logging to actor logs, which is valuable for debugging and tracing in distributed environments. Overall, the implementation is clean and follows good patterns. Here are my observations: Positives ✓
Suggestions for Improvement
Security & Performance
Code StyleThe code follows TypeScript conventions and matches the existing codebase style. Final AssessmentThis is a solid, straightforward change that adds valuable debugging information. The main risk is the timing issue - ensure Approve pending verification of the timing/initialization concern. |
Pull Request ReviewThis PR adds runner ID logging to actor instances by introducing a new optional method ✅ Strengths
🔍 Observations & Suggestions1. Type Safety ConsiderationThe return type is
2. Consistency Across DriversI noticed there are 3
Question: Should the other drivers also implement
Since the method is optional, this is not blocking, but it might be worth considering for consistency. 3. Potential Race ConditionIn getExtraActorLogParams(): Record<string, string> {
return { runnerId: this.#runner.runnerId ?? "-" };
}This is called during actor initialization (line 331 of instance.ts). At that point:
The fallback
Suggestion: If you want the actual runnerId in logs, you might need to:
This may not be an issue if runnerId is always set synchronously, but worth clarifying. 4. TestingNo test changes were included. Consider:
The driver-test-suite at 📝 Minor Style Notes
🎯 SummaryThis is a solid, focused PR that adds valuable observability. The main considerations are:
The implementation is clean and the optional method pattern is good design. Nice work! 🚀 Recommendation: ✅ Approve with minor suggestions for follow-up |
7e5b57a to
60e4821
Compare
b33ce24 to
2b8185b
Compare
Code Review: PR #3360 - Log runner id with actor logsSummaryThis PR adds runner ID logging to actor instances by introducing an optional Positive Aspects✅ Clean interface design - Using an optional method ( ✅ Proper null handling - The fallback to ✅ Minimal surface area - The change is focused and doesn't introduce unnecessary complexity. Issues & Recommendations1. Type Safety Concern - Return type could be more specificFile: The return type // Current
getExtraActorLogParams?(): Record<string, string>;
// Suggested - more type-safe
getExtraActorLogParams?(): Record<string, string | number | boolean>;This would allow numeric runner IDs or other typed metadata in the future. 2. Inconsistent Implementation - Missing implementation in Cloudflare Workers driverFile: The Recommendation: Consider adding a similar implementation for Cloudflare Workers if runner/DO identification would be useful there: getExtraActorLogParams(): Record<string, string> {
// Could log Durable Object ID or other CF-specific context
return { durableObjectId: this.#globalState./* ... */ };
}3. Missing Test CoverageThere are no tests verifying:
Recommendation: Add a test case in the driver test suite to verify this functionality. 4. Documentation MissingThe JSDoc comment for /** Extra properties to add to logs for each actor. */Recommendation: Expand documentation to clarify:
Example: /**
* Provides extra properties to include in structured logs for all log statements
* made by this actor instance.
*
* Called once during actor initialization. The returned properties are merged
* into the log context and will appear in all subsequent log entries for this actor.
*
* @returns A record of string key-value pairs to include in log context.
* @example
* ```typescript
* getExtraActorLogParams() {
* return { runnerId: this.#runner.runnerId ?? "-" };
* }
* ```
*/
getExtraActorLogParams?(): Record<string, string>;5. Minor: Log Key Naming ConventionFile: The log key uses camelCase ( Security & Performance✅ No security concerns identified Overall AssessmentThis is a solid, focused change that improves observability. The main areas for improvement are:
Recommendation: Approve with suggestions for test coverage and documentation improvements. |
2b8185b to
78f7198
Compare
60e4821 to
2cd2955
Compare
Code ReviewSummaryThis PR adds runner ID tracking to actor logs by introducing an optional ✅ Positive Aspects
🔍 Observations & QuestionsTiming Consideration:
However, I notice that Question: Can
📝 Minor Suggestions
getExtraActorLogParams(): Record<string, string | number | boolean> {
return { runnerId: this.#runner.runnerId ?? "-" };
}This would allow for non-string metadata in the future.
/**
* Extra properties to add to logs for each actor.
* Called during ActorInstance.start() before actor initialization.
*/
getExtraActorLogParams?(): Record<string, string>;🎯 Test CoverageNo test changes are included. Consider adding:
Security & Performance✅ No security concerns identified ConclusionThis is a solid, minimal change that adds useful debugging information. The optional interface design is appropriate. My main question is about the timing guarantees around when Recommendation: Approve with optional follow-up to clarify timing guarantees and add tests. |
Code Review: Add Runner ID to Actor LogsSummaryThis PR adds runner ID logging to actor instances by introducing an optional Positive Observations✅ Clean Interface Design: The use of an optional method ( ✅ Consistent Implementation: The extra log params are applied to both the main logger ( ✅ Fallback Handling: The code properly handles the case where Issues & Recommendations1. Type Safety Concern (Minor)Location: The return type Current code: getExtraActorLogParams?(): Record<string, string>;In implementation: return { runnerId: this.#runner.runnerId ?? "-" };Recommendation: Consider using a more flexible type to allow for getExtraActorLogParams?(): Record<string, string | number | undefined>;Or keep the current type but add a JSDoc comment clarifying implementations must ensure string values. 2. Missing Implementations (Medium Priority)Location: Other Two other drivers implement
Impact: This is fine for optional methods, but these drivers might benefit from similar contextual logging. For example, the Cloudflare driver could log the Durable Object ID. Recommendation: Consider adding implementations to other drivers in follow-up work, or document why only 3. Potential Timing Issue (Low Priority)Location: The Current code: getExtraActorLogParams(): Record<string, string> {
return { runnerId: this.#runner.runnerId ?? "-" };
}Question: Is Recommendation: Verify that 4. Test Coverage (Medium Priority)No tests were added to verify:
Recommendation: Add a test case in the existing driver tests (e.g., Security Considerations✅ No security concerns identified. The runner ID is internal infrastructure metadata. Performance Considerations✅ Minimal performance impact. The method is called once during actor initialization, and the object creation is trivial. Code StyleThe code follows TypeScript conventions and is consistent with the existing codebase style. Overall AssessmentThis is a clean, focused change that adds useful debugging information. The main suggestions are:
Status: ✅ Approved - This can be merged as-is, with the recommendations addressed in follow-up work if needed. |
2cd2955 to
c74dcd8
Compare
78f7198 to
95ffd87
Compare
95ffd87 to
b462d64
Compare
c74dcd8 to
a904843
Compare
Code ReviewSummaryThis PR adds logging of runner IDs with actor logs by introducing an optional Code Quality ✅Positives:
Potential Issues 🔍1. Timing Consideration (Minor) getExtraActorLogParams(): Record<string, string> {
return { runnerId: this.#runner.runnerId ?? "-" };
}This method is called during Question: Is this intentional? Should we consider:
2. Type Safety (Minor) Missing Implementation (Optional Driver Interface)The Consideration: Should the file system driver also include any relevant context in logs? If not, this is fine as-is. Test Coverage 📋No tests were added. Given this is a logging enhancement, consider:
Performance ✅Negligible impact - single object creation per actor instance initialization. Security ✅No security concerns. Logging runner IDs is appropriate for debugging and observability. Documentation 📝Consider adding JSDoc to explain:
Example: /**
* Extra properties to add to logs for each actor.
* Called during actor instance construction, so some values may not be available yet.
*/
getExtraActorLogParams?(): Record<string, string>;SuggestionsLow Priority:
Conclusion ✅This is a clean, focused PR that improves observability. The implementation is solid with only minor considerations around timing and documentation. Approved with minor suggestions for enhancement. 🤖 Generated by Claude Code |
Code ReviewThis PR adds runner ID logging to actor logs by introducing an optional ✅ Strengths
💡 Suggestions1. Type Safety ImprovementConsider using a more specific return type to ensure consistency: // Instead of Record<string, string>
getExtraActorLogParams?(): Record<string, unknown>;This allows logging non-string values (numbers, booleans) which structured logging systems like Pino handle well. 2. Null Safety in ImplementationIn getExtraActorLogParams(): Record<string, string> {
// Only include runnerId if it exists
return this.#runner.runnerId ? { runnerId: this.#runner.runnerId } : {};
}This avoids logging 3. DocumentationAdd JSDoc to explain when/why extra params would be used: /**
* Extra properties to add to logs for each actor.
* Useful for adding driver-specific identifiers (e.g., runner IDs, container IDs)
* that help correlate actor logs with infrastructure logs.
*/
getExtraActorLogParams?(): Record<string, string>;4. Other DriversConsider whether
Not necessary for this PR, but worth considering for consistency. 🔍 Questions
✅ No Issues Found
SummaryApprove with minor suggestions. The core implementation is solid. Consider the type safety and null handling improvements for better maintainability. Generated by Claude Code |
Merge activity
|

No description provided.