-
Notifications
You must be signed in to change notification settings - Fork 19
chore: improve error logs #285
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
Conversation
WalkthroughThis PR systematically refactors error logging across 10 federation-sdk service files, converting string-based and flat error messages to structured object payloads containing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes follow a consistent refactoring pattern across multiple files (logging format standardization), but vary in structure and context fields per service. Verification requires checking each file for correctness despite the repetitive nature. Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #285 +/- ##
==========================================
+ Coverage 60.47% 60.49% +0.02%
==========================================
Files 67 67
Lines 6676 6675 -1
==========================================
+ Hits 4037 4038 +1
+ Misses 2639 2637 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/federation-sdk/src/services/state.service.ts (1)
473-477: Consider adding theerrfield.The structured logging at lines 473-477 includes
eventId,authEvents, andmsg, but is missing theerrfield. For consistency with other error logs in this PR and to preserve the error details, consider addingerr: errorto the log object.Apply this diff:
this.logger.error({ eventId: event.eventId, authEvents: event.getAuthEventIds(), msg: 'event failed auth check while saving state, this should not have happened while walking an auth chain, the chain is incorrect', + err: error, });packages/federation-sdk/src/services/room.service.ts (3)
1046-1053: Add error message when throwing.The structured logging looks good, but the thrown error at line 1053 lacks a message, making debugging more difficult.
Apply this diff to add a descriptive error message:
this.logger.warn({ msg: 'not enough servers participating in the room to retry missing events', roomId, branchEventId: context.event.eventId, missing, }); - throw new Error(); + throw new Error('Not enough servers participating in the room to retry missing events');
1059-1065: Consider usinginfolevel instead ofwarn.This logs a retry attempt, which is normal operation rather than a warning condition. The
warnlevel might trigger alerts unnecessarily.Apply this diff to use a more appropriate log level:
- this.logger.warn({ + this.logger.info({ msg: 'attempting to fetch events from participating server', askingServer, roomId, branchEventId: context.event.eventId, missing, });
1079-1086: Add error message when throwing.The structured logging is excellent, but the thrown error at line 1086 lacks a message. This makes it harder to identify the root cause when caught upstream.
Apply this diff to add a descriptive error message:
this.logger.error({ msg: 'server list exhausted, we still have missing events', roomId, branchEventId: context.event.eventId, missing, }); - throw new Error(); + throw new Error('Server list exhausted with missing events');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
packages/federation-sdk/src/services/config.service.ts(1 hunks)packages/federation-sdk/src/services/database-connection.service.ts(2 hunks)packages/federation-sdk/src/services/edu.service.ts(2 hunks)packages/federation-sdk/src/services/event-authorization.service.ts(3 hunks)packages/federation-sdk/src/services/event-fetcher.service.ts(0 hunks)packages/federation-sdk/src/services/event.service.ts(1 hunks)packages/federation-sdk/src/services/federation-request.service.ts(1 hunks)packages/federation-sdk/src/services/missing-event.service.ts(1 hunks)packages/federation-sdk/src/services/room.service.ts(2 hunks)packages/federation-sdk/src/services/state.service.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/federation-sdk/src/services/event-fetcher.service.ts
🔇 Additional comments (11)
packages/federation-sdk/src/services/federation-request.service.ts (1)
116-119: LGTM!The conversion to structured error logging is correct. The
msganderrfields provide clear context, and the error-rethrowing behavior is preserved.packages/federation-sdk/src/services/missing-event.service.ts (1)
58-62: LGTM!The structured logging with additional
eventIdcontext is helpful for debugging. The return behavior is preserved correctly.packages/federation-sdk/src/services/config.service.ts (1)
92-95: LGTM!Standardizing the error field name from
detailstoerrmaintains consistency across the codebase. The error handling behavior is preserved.packages/federation-sdk/src/services/edu.service.ts (1)
46-49: LGTM!Both catch blocks correctly implement structured error logging with
msganderrfields. The error-rethrowing behavior is preserved in both methods.Also applies to: 85-88
packages/federation-sdk/src/services/event.service.ts (1)
297-301: LGTM!The structured logging with the
eduobject provides valuable debugging context. The loop continuation behavior (processing remaining EDUs after an error) is correctly preserved.packages/federation-sdk/src/services/database-connection.service.ts (1)
15-15: LGTM!Both database connection error logs correctly use structured logging with
msganderrfields. The error handling behavior is preserved in both locations.Also applies to: 56-56
packages/federation-sdk/src/services/state.service.ts (1)
791-795: LGTM!The structured logging correctly includes
err,eventId, andmsgfields. The error-throwing behavior is preserved.packages/federation-sdk/src/services/event-authorization.service.ts (3)
177-180: LGTM!The structured error logging with
msganderrfields is correct, and the return behavior is preserved.
429-437: LGTM!The comprehensive structured logging includes all relevant context fields (
err,eventId,authorizationHeader,method,uri,body,msg), which will be valuable for debugging authorization issues. The error handling behavior is correctly preserved.
488-495: LGTM!The structured logging includes appropriate context fields for media access debugging. The error handling behavior is preserved correctly.
packages/federation-sdk/src/services/room.service.ts (1)
1038-1043: LGTM! Structured logging improves observability.The change to structured logging with all relevant context fields (
roomId,branchEventId,missing) is appropriate and aids in debugging federation issues.
correctly show error logs
Summary by CodeRabbit