fix(rivetkit): fix not pino bindings #3334
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
Code Review - PR #3334: fix(rivetkit): fix not pino bindingsSummaryThis PR fixes a bug where child logger bindings (contextual fields added via Positive Findings✅ Correct Bug Fix: The change properly addresses the issue by:
✅ Consistent Implementation: Bindings are correctly added to all 4 code paths in the ✅ Proper Ordering: Bindings are spread first, then merged with log data, allowing log-specific fields to override bindings if needed Issues & Recommendations🔴 Critical: Type Safety ConcernIssue: The code uses rivetkit-typescript/packages/rivetkit/src/common/log.ts:167 const bindings = (this as any).bindings?.() || {};Recommendation: Consider adding a proper type definition for the Pino logger context: interface PinoLoggerContext {
bindings(): Record<string, any>;
}
// Then use:
const bindings = (this as PinoLoggerContext).bindings?.() || {};This would provide better type safety while still being explicit about the expected interface.
|
NathanFlurry
force-pushed
the
11-04-fix_rivetkit_fix_not_pino_bindings
branch
from
084c5e3 to
b5e9f49
Compare
Code Review: fix(rivetkit): fix not pino bindingsSummaryThis PR fixes a bug where Pino logger bindings (child logger fields) were not being included in log output. The fix retrieves bindings from the logger instance and spreads them into the log output object. Positive Aspects ✅
Issues & Concerns 🔍1. Type Safety (Medium Priority)The code uses const bindings = (this as any).bindings?.() || {};Recommendation: Consider adding a proper type definition or interface for the Pino logger context to maintain type safety. If this is necessary due to Pino's internal API, add a comment explaining why the 2. Missing Tests (High Priority)There are no tests for the logging functionality, which means:
Recommendation: Add tests that verify:
Example test case: test('child logger bindings are included in output', () => {
const logger = getLogger('test').child({ requestId: '123' });
// Assert that logs contain requestId field
});3. Performance Consideration (Low Priority)The bindings function is called on every log statement. While Pino is designed for this, consider:
Note: This is likely fine as-is since Pino's 4. Documentation (Low Priority)The existing TODO comment mentions performance improvements with a custom transport target: // TODO: This is a hack to not implement our own transport target. We can get better perf if we have our own transport target.Recommendation: Consider updating this TODO or the inline comment to mention that bindings support was added and should be maintained in any future refactoring. Code Quality 📝Strengths:
Minor Suggestions:
Security Concerns 🔒No security issues identified. The change doesn't introduce any new attack vectors. Testing Recommendations 🧪Given the lack of existing tests for this module, I recommend:
Example test structure: describe('Logger bindings', () => {
it('includes child logger bindings in output', () => { /* ... */ });
it('merges bindings with explicit log fields', () => { /* ... */ });
it('handles nested child loggers', () => { /* ... */ });
});Verdict ✨Approval with recommendations: This is a solid bug fix that addresses a real issue. The implementation is correct and consistent. However, the lack of tests is concerning for a logging library that's central to observability. Suggested action items:
References
|
NathanFlurry
force-pushed
the
11-03-chore_rivetkit_fmt
branch
from
19cd542 to
4f8383d
Compare
NathanFlurry
force-pushed
the
11-04-fix_rivetkit_fix_not_pino_bindings
branch
from
b5e9f49 to
3cedf91
Compare
Code ReviewSummaryThis PR fixes a bug where child logger bindings (fields added via Positive Changes ✅
Code Quality ObservationsType Safety Concern const bindings = (this as any).bindings?.() || {};Using
Repetitive Code 📝 const mergeWithBindings = (data: any) => ({ ...bindings, ...data });
// Then use:
customWrite(levelName, mergeWithBindings({ ...objOrMsg, msg, time }));Empty Object Fallback 💭 const bindings = (this as any).bindings?.() || {};Good defensive programming - ensures bindings is always an object even if the method doesn't exist. Testing Concerns 🧪No test files were found for the logging module. Consider adding tests to verify:
Performance Considerations ⚡
Security ✓No security concerns identified. The bindings are treated the same as other log data. Recommendations
Overall Assessment✅ Approved - This is a solid bug fix that addresses the issue correctly. The implementation is safe and follows the existing code patterns. The concerns raised are minor improvements that can be addressed in follow-up work if desired. The fix properly handles the Pino child logger API and ensures structured logging context is preserved, which is important for log aggregation and debugging. |
Code Review: fix(rivetkit): fix pino bindingsSummaryThis PR fixes a bug where child logger bindings (fields added via Positive Aspects ✅
Observations & Suggestions 🔍1. Type Safety Concern (Minor)const bindings = (this as any).bindings?.() || {};Issue: Using Suggestion: Consider creating a typed interface: interface PinoLoggerInternal {
bindings?: () => Record<string, unknown>;
}
const bindings = ((this as unknown) as PinoLoggerInternal).bindings?.() || {};This provides minimal type documentation while maintaining flexibility. 2. Missing Unit TestsObservation: This fix addresses a critical bug in logging functionality, but I don't see corresponding test coverage being added. Recommendation: Add tests that verify:
Example test case structure: test('child logger bindings appear in output', () => {
const logger = getBaseLogger().child({ actorId: '123', target: 'test' });
logger.info({ msg: 'test message' });
// Assert output contains both actorId and target
});3. Unused Parameter Issue (Fixed)Good catch: Changing 4. Code Duplication (Pre-existing)Observation: The four branches for handling different input argument patterns have significant duplication. The only differences are:
Suggestion for future PR: Consider refactoring to reduce duplication: const logObject = { ...bindings, time };
if (inputArgs.length >= 2) {
const [objOrMsg, msg] = inputArgs;
if (typeof objOrMsg === "object" && objOrMsg !== null) {
Object.assign(logObject, objOrMsg, { msg });
} else {
logObject.msg = String(objOrMsg);
}
} else if (inputArgs.length === 1) {
const [objOrMsg] = inputArgs;
if (typeof objOrMsg === "object" && objOrMsg !== null) {
Object.assign(logObject, objOrMsg);
} else {
logObject.msg = String(objOrMsg);
}
}
customWrite(levelName, logObject);This is purely a style suggestion and not blocking for this PR. Performance Considerations ⚡The changes have minimal performance impact:
Security Considerations 🔒No security concerns identified. The code:
Testing Recommendations 🧪
ConclusionVerdict: ✅ LGTM with minor suggestions This is a solid bug fix that addresses the core issue correctly. The implementation follows Pino's patterns and maintains backward compatibility. The suggestions above are for improving code quality and test coverage but are not blocking issues. The main actionable item is adding test coverage to prevent regression of this bug in the future. Reviewed by: Claude Code |
Merge activity
|
NathanFlurry
force-pushed
the
11-03-chore_rivetkit_fmt
branch
from
4f8383d to
d91f7e7
Compare
NathanFlurry
force-pushed
the
11-04-fix_rivetkit_fix_not_pino_bindings
branch
from
3cedf91 to
c98b4a2
Compare
Code ReviewSummaryThis PR fixes a bug where Pino logger bindings (child logger fields) were not being included in the log output. The fix extracts bindings from the logger instance and spreads them into all log entries. Positive Aspects ✅
Code Quality 📝Type Safety Concern
Object Spread Order customWrite(levelName, {
...bindings,
...objOrMsg,
msg,
time,
});This is correct because it allows explicit log properties to override bindings if there's a conflict, which is the expected behavior. Performance Considerations ⚡
Test Coverage 🧪Missing: No tests were added for this functionality. Consider adding tests that verify:
Example test case: const childLogger = logger.child({ target: 'test-target' });
childLogger.info({ custom: 'value' }, 'test message');
// Verify output contains both 'target' and 'custom' fieldsSecurity Concerns 🔒No security issues identified. The bindings are user-controlled (set via Recommendations 💡
VerdictApproved ✓ - This is a solid bug fix that correctly implements Pino bindings support. The implementation is consistent and safe. Adding tests would strengthen confidence in this change, but the code quality is good. |
Code ReviewSummaryThis PR fixes a critical bug where Pino child logger bindings (fields added via Code Quality & Best Practices ✅Strengths:
Minor considerations:
Potential Issues 🔍Low Risk:
Testing Recommendations 🧪The PR lacks test coverage for the fixed functionality. Recommend adding tests such as: import { describe, it, expect, beforeEach, afterEach } from 'vitest';
import { configureDefaultLogger, getBaseLogger } from './log';
describe('child logger bindings', () => {
let logOutput: string[] = [];
const originalLog = console.log;
beforeEach(() => {
logOutput = [];
console.log = (msg: string) => logOutput.push(msg);
});
afterEach(() => {
console.log = originalLog;
});
it('should include child logger bindings in log output', async () => {
await configureDefaultLogger('info');
const logger = getBaseLogger();
const child = logger.child({ requestId: '123', userId: 'abc' });
child.info({ action: 'test' }, 'test message');
const output = logOutput[0];
expect(output).toContain('requestId=123');
expect(output).toContain('userId=abc');
expect(output).toContain('action=test');
expect(output).toContain('msg="test message"');
});
it('should handle nested child loggers', async () => {
await configureDefaultLogger('info');
const logger = getBaseLogger();
const child1 = logger.child({ layer: 'api' });
const child2 = child1.child({ endpoint: '/users' });
child2.info('processing request');
const output = logOutput[0];
expect(output).toContain('layer=api');
expect(output).toContain('endpoint=/users');
});
});Security Considerations ✅No security concerns identified. The change doesn't introduce any new attack surfaces. Performance Considerations ⚡
Impact AssessmentFiles affected: 1 file, core logging infrastructure This is a well-implemented bug fix that improves structured logging by ensuring child logger context is properly propagated. The implementation is clean and consistent. Recommendation: ✅ Approve with suggestion to add tests The code is ready to merge, but adding test coverage would prevent regression and document the expected behavior for future maintainers. |
No description provided.