chore(runner): add child logger with runner id #3380
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
Code ReviewOverviewThis PR introduces a cached child logger with runner-specific attributes, improving observability by automatically attaching the Positive Aspects
Issues and Concerns1. Potential Race Condition in Logger Caching (Minor)get log(): Logger | undefined {
if (this.#logCached) return this.#logCached;
const l = logger();
if (l) {
if (this.runnerId) {
this.#logCached = l.child({
runnerId: this.runnerId,
});
return this.#logCached;
} else {
return l;
}
}
return undefined;
}Issue: The cache is set based on whether Scenario:
Actually, on closer inspection, this appears to work correctly because the cache is only set when Recommendation: Consider adding a comment explaining that the cache is only populated after set runnerId(value: string) {
this.#runnerId = value;
this.#logCached = undefined; // Reset cache when runnerId changes
}2. Missing Type Import (Minor)The 3. Test Coverage (Important)The test file (
Recommendation: At minimum, add a unit test that verifies: test('log property creates child logger with runnerId', () => {
const runner = new Runner(config);
// Before runnerId is set
expect(runner.log).toBeDefined();
// Simulate connection and runnerId assignment
runner.runnerId = 'test-runner-123';
// Verify child logger includes runnerId in context
// (This would require inspecting the pino child logger's bindings)
});4. Documentation (Minor)The getter has a brief comment, but consider documenting:
Security ConsiderationsNo security concerns identified. The change only affects logging infrastructure. Performance Considerations✅ Improvement: Caching the child logger prevents recreating it on every log call, which is more efficient than the previous approach of adding Adherence to Repository StandardsBased on CLAUDE.md: ✅ Structured Logging: The PR maintains structured logging practices by using pino's child logger feature instead of formatting the ✅ Code Style: The changes follow existing patterns in the codebase ✅ Naming Conventions: The Suggestions
/**
* Returns a logger instance with runner-specific context.
* Once the runner is connected and has a runnerId, this returns a cached
* child logger that automatically includes the runnerId in all log messages.
* Before connection, returns the base logger without runner context.
*/
get log(): Logger | undefined {
// ...
}
Verdict✅ Approved with minor suggestions This is a solid refactoring that improves code quality and observability. The main suggestions are around edge case handling and documentation. The implementation is correct for the common case and represents a clear improvement over the previous approach. Great work on reducing repetition and improving the logging infrastructure! 🎯 |
NathanFlurry
force-pushed
the
11-05-chore_rivetkit_remove_warnings_for_graceful_runner_shtudown
branch
from
1da12da to
abfe409
Compare
NathanFlurry
force-pushed
the
11-05-chore_runner_add_child_logger_with_runner_id
branch
from
c923a6f to
51912f8
Compare
More templates
@rivetkit/actor
@rivetkit/cloudflare-workers
@rivetkit/core
@rivetkit/db
@rivetkit/framework-base
@rivetkit/next-js
@rivetkit/react
rivetkit
@rivetkit/sql-loader
@rivetkit/engine-runner
@rivetkit/engine-runner-protocol
commit: |
Code ReviewSummaryThis PR introduces a child logger with runner-specific context (runner ID) to improve observability by automatically including the Positive Changes✅ Excellent refactoring - Removes repetitive Issues & Concerns🔴 Critical: Stale Logger Cache BugLocation: The get log(): Logger | undefined {
if (this.#logCached) return this.#logCached; // ❌ Returns stale logger!
const l = logger();
if (l) {
if (this.runnerId) {
this.#logCached = l.child({
runnerId: this.runnerId, // This runnerId could be old
});
return this.#logCached;
} else {
return l;
}
}
return undefined;
}The problem is at if (this.runnerId !== init.runnerId) {
this.runnerId = init.runnerId; // ❌ Cache not cleared!Impact: After a reconnect where Fix: Clear the cache when if (this.runnerId !== init.runnerId) {
this.runnerId = init.runnerId;
this.#logCached = undefined; // Clear stale logger cache
// Clear history if runner id changed
this.#eventHistory.length = 0;
}
|
PR Review: Add child logger with runner IDSummaryThis PR refactors logging throughout the TypeScript runner to use a cached child logger that automatically includes the ✅ Strengths
🔍 Code Quality Observations1. Cache Invalidation Issue
|
NathanFlurry
force-pushed
the
11-05-chore_runner_add_child_logger_with_runner_id
branch
from
51912f8 to
4831d26
Compare
NathanFlurry
force-pushed
the
11-05-chore_rivetkit_remove_warnings_for_graceful_runner_shtudown
branch
from
abfe409 to
5b69155
Compare
PR Review: Add child logger with runner IDSummaryThis PR improves logging consistency by creating a child logger with Code Quality ✅Strengths:
Potential Issues
|
PR Review: Add child logger with runner idSummaryThis PR improves logging observability by creating a child logger with the runner ID automatically included in all log messages. This removes the need to manually add Code Quality ✅Strengths:
Implementation Review ✅The logger getter (lines 142-161 in mod.ts): get log(): Logger | undefined {
if (this.#logCached) return this.#logCached;
const l = logger();
if (l) {
if (this.runnerId) {
this.#logCached = l.child({
runnerId: this.runnerId,
});
return this.#logCached;
} else {
return l;
}
}
return undefined;
}Observations:
Potential Issues
|
NathanFlurry
force-pushed
the
11-05-chore_runner_add_child_logger_with_runner_id
branch
from
4831d26 to
ff5ae6a
Compare
NathanFlurry
force-pushed
the
11-05-chore_rivetkit_remove_warnings_for_graceful_runner_shtudown
branch
from
5b69155 to
c179f06
Compare
PR Review: Add child logger with runner idSummaryThis PR introduces a child logger with runner-specific attributes to improve log traceability. The implementation creates a cached logger getter that automatically includes the ✅ Strengths
🔍 Code Quality ObservationsImplementation PatternThe logger getter implementation is solid: get log(): Logger | undefined {
if (this.#logCached) return this.#logCached;
const l = logger();
if (l) {
if (this.runnerId) {
this.#logCached = l.child({ runnerId: this.runnerId });
return this.#logCached;
} else {
return l;
}
}
return undefined;
}Potential Issue: The cache is set only when 🐛 Potential Issues1. Logger Cache Invalidation (Minor)The cached logger is created when Recommendation: Consider invalidating the cache if the implementation allows runner ID changes: get log(): Logger | undefined {
const l = logger();
if (l) {
if (this.runnerId) {
// Recreate if runnerId changed
if (\!this.#logCached || this.#lastRunnerId \!== this.runnerId) {
this.#logCached = l.child({ runnerId: this.runnerId });
this.#lastRunnerId = this.runnerId;
}
return this.#logCached;
}
return l;
}
return undefined;
}However, if runner IDs are immutable once set, the current implementation is fine. 2. Commented Debug Logs (Cleanup Opportunity)Found several commented log lines:
Recommendation: Remove these commented lines in a follow-up cleanup if they're no longer needed. 🔒 Security ConsiderationsNo security concerns identified. The change is purely internal logging infrastructure. ⚡ Performance ConsiderationsPositive:
Note: The getter is called frequently (50+ times per file). The caching strategy is appropriate. 🧪 Test CoverageThe test file (
Recommendation: Enable and update tests to verify the logging behavior, especially around the connection lifecycle. 📝 Code StyleAdheres to TypeScript conventions. The use of private fields ( ✨ Suggestions for Improvement
🎯 SummaryThis is a clean, well-executed refactoring that improves code maintainability and logging consistency. The changes are low-risk and provide clear benefits. Recommendation: ✅ Approve with minor suggestions The suggestions above are optional improvements, not blockers. The PR is ready to merge as-is. Review generated with assistance from Claude Code |
Merge activity
|
graphite-app
bot
deleted the
11-05-chore_runner_add_child_logger_with_runner_id
branch
No description provided.