chore: fix logger usage and dependencies #215
Conversation
WalkthroughDependencies and logging were reorganized: root removed pino deps; core added pino and dev pino-pretty; SDK reordered dependencies. SDK and core files switched to structured logging and, in SDK, adopted the core logger, removing the local logger module. No control flow or public API changes, except SDK logger module removal. Changes
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
sampaiodiego
force-pushed
the
fix-logger
branch
from
bce00de to
13b282c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #215 +/- ##
==========================================
- Coverage 80.99% 80.94% -0.06%
==========================================
Files 63 63
Lines 4693 4696 +3
==========================================
Hits 3801 3801
- Misses 892 895 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/federation-sdk/src/services/config.service.ts (1)
50-51: Schema/interface mismatch and unused config: make signingKeyPath optional.Interface marks it optional, schema requires it, and loadSigningKey ignores it. Make optional and actually use it (see below).
- signingKeyPath: z.string(), + signingKeyPath: z.string().optional(),packages/federation-sdk/src/services/invite.service.ts (1)
94-94: Avoid unhandled promise rejections when fire-and-forgetting.
Detached promises should log failures to prevent silent drops.- void federationService.sendEventToAllServersInRoom(inviteEvent); + void federationService + .sendEventToAllServersInRoom(inviteEvent) + .catch((err) => this.logger.error({ err, eventId: inviteEvent.eventId, roomId }, 'Failed to broadcast invite event'));- void federationService.sendEventToAllServersInRoom(inviteEvent); + void federationService + .sendEventToAllServersInRoom(inviteEvent) + .catch((err) => this.logger.error({ err, eventId: inviteEvent.eventId, roomId }, 'Failed to broadcast invite event'));Also applies to: 120-120
🧹 Nitpick comments (19)
packages/federation-sdk/package.json (1)
18-28: Consider alphabetizing dependencies to reduce diff churnOptional: keeping the dependencies list alphabetized makes future diffs smaller and easier to review.
packages/federation-sdk/src/services/config.service.ts (5)
85-88: Log error with proper structure (avoid 'msg' field and include the Error object).Preserve stack/causes and keep the message in the logger’s message slot. Example pino-style:
- this.logger.error({ - msg: 'Configuration validation failed:', - details: error.errors, - }); + this.logger.error( + { err: error, details: error.errors }, + 'Configuration validation failed', + );
142-147: Honor signingKeyPath and use structured info logs.Currently ignores signingKeyPath and interpolates strings. Use provided path when present and structured logging.
- const signingKeyPath = `${CONFIG_FOLDER}/${this.config.serverName}.signing.key`; - this.logger.info(`Loading signing key from ${signingKeyPath}`); + const signingKeyPath = + this.config.signingKeyPath ?? `${CONFIG_FOLDER}/${this.config.serverName}.signing.key`; + this.logger.info({ signingKeyPath }, 'Loading signing key'); const keys = await getKeyPair({ signingKeyPath }); - this.logger.info( - `Successfully loaded signing key for server ${this.config.serverName}`, - ); + this.logger.info({ serverName: this.config.serverName }, 'Successfully loaded signing key');
150-153: Preserve error stack in logs.Avoid stringifying errors; log the Error instance to keep stack and cause.
- const errorMessage = - error instanceof Error ? error.message : 'Unknown error'; - this.logger.error(`Failed to load signing key: ${errorMessage}`); + const err = error instanceof Error ? error : new Error(String(error)); + this.logger.error({ err }, 'Failed to load signing key');
129-133: Unreachable fallback and missing guard for empty keys.Template string is always truthy; default never used. Also guard empty array.
- const signingKeys = await this.getSigningKey(); - const signingKey = signingKeys[0]; - return `${signingKey.algorithm}:${signingKey.version}` || 'ed25519:1'; + const signingKeys = await this.getSigningKey(); + const signingKey = signingKeys?.[0]; + if (!signingKey?.algorithm || !signingKey?.version) { + return 'ed25519:1'; + } + return `${signingKey.algorithm}:${signingKey.version}`;
135-138: Guard against missing privateKey to avoid runtime crash.- const signingKeys = await this.getSigningKey(); - return toUnpaddedBase64(signingKeys[0].privateKey); + const signingKeys = await this.getSigningKey(); + const signingKey = signingKeys?.[0]; + if (!signingKey?.privateKey) { + throw new Error('No signing key available'); + } + return toUnpaddedBase64(signingKey.privateKey);packages/federation-sdk/src/services/event-emitter.service.ts (1)
6-6: Use a scoped logger via createLogger for consistency and contextOther services use createLogger(''). Standardize here and include the service name in logs.
Apply:
-import { logger } from '@rocket.chat/federation-core'; +import { createLogger } from '@rocket.chat/federation-core';- logger.debug({ msg: `Event emitted: ${event}`, event, data }); + this.logger.debug({ event, data }, `Event emitted: ${String(event)}`);Add the scoped logger on the class (outside these ranges):
private readonly logger = createLogger('EventEmitterService');Also applies to: 33-33
packages/federation-sdk/src/services/federation.service.ts (1)
278-281: Good move to structured errors; align remaining catch blocks to the same shapeThese use { msg, err }, which is consistent with pino conventions. Please update earlier catches (e.g., makeJoin, sendJoin, sendTransaction, sendEvent, getEvent, getState, getStateIds, getVersion) that still log strings and stacks to use the same structured shape for uniformity and better parsing.
Also applies to: 315-318
packages/federation-sdk/src/services/event-authorization.service.ts (1)
362-363: Prefererrkey for Error objects to match the rest of the codebaseOther sites use
err; standardize the field name for tooling/serialization consistency.- this.logger.warn({ msg: `Invalid ACL pattern: ${pattern}`, error }); + this.logger.warn({ msg: `Invalid ACL pattern: ${pattern}`, err: error });packages/federation-sdk/src/services/event.service.ts (2)
179-183: Useerrinstead oferrorand minimize heavy payloads in error logs
- Rename
errortoerrfor consistency with other logs/tools.- Optional: logging the entire event can be noisy; consider logging identifiers (eventId/type) unless you need full payloads for triage.
- this.logger.error({ - msg: 'Event validation failed', - event, - error: err, - }); + this.logger.error({ + msg: 'Event validation failed', + event, // consider reducing to { id: generateId(event), type: event.type } + err: err, + });
721-725: LGTM: consistent structured logging across state/auth pathsThese follow the { msg, err } convention and keep messages uniform. Consider rolling this style out to remaining non-structured logs in this file (e.g., EDU processing) for full consistency.
Also applies to: 733-737, 783-787, 799-803, 811-815
packages/federation-sdk/src/queues/base.queue.ts (1)
35-38: Use Pino’s structured parameters for logging errorsReplace the object-with-
msgusage with a separate message and context object:- logger.error({ - msg: `${this.constructor.name}: Error processing item`, - err, - }); + logger.error( + { err, queue: this.constructor.name }, + 'Error processing item', + );packages/federation-sdk/src/services/invite.service.ts (5)
41-41: Prefer structured logging over string interpolation.
Easier to query logs and avoids coupling message formatting to code.- this.logger.debug(`Inviting ${userId} to room ${roomId}`); + this.logger.debug({ userId, roomId, sender, isDirectMessage }, 'Inviting user to room');
181-181: Replace console.error with the centralized logger.
Aligns with the PR goal and preserves structured logging.- console.error(e); + this.logger.warn({ err: e, roomId, eventId }, 'Invite persistence skipped; local room state not present yet');
148-149: Improve error detail for easier debugging.
Include both expected and received IDs.- throw new Error(`Invalid eventId ${eventId}`); + throw new Error(`Invalid eventId: expected ${inviteEvent.eventId}, got ${eventId}`);
102-102: Typo in comment: “room” → “server”.
Minor doc polish.- // invited user from another room + // invited user from another server
77-81: Make server extraction robust to additional colons.
Use first-colon index instead of last-segment .pop().- const invitedServer = inviteEvent.stateKey?.split(':').pop(); + const sk = inviteEvent.stateKey; + const invitedServer = sk && sk.includes(':') ? sk.slice(sk.indexOf(':') + 1) : undefined;- const residentServer = roomId.split(':').pop(); + const residentServer = roomId.includes(':') ? roomId.slice(roomId.indexOf(':') + 1) : undefined;Also applies to: 136-139
packages/core/src/utils/checkSignAndHashes.ts (1)
43-45: Use Pino’s standard signature and add minimal context; avoid logging full hashesPrefer passing the message as the second argument and include origin; trim hashes to keep logs small.
- logger.error({ msg: 'Invalid hash', hash, expectedHash }); + logger.error( + { origin, hash: hash?.slice(0, 12), expectedHash: expectedHash?.slice(0, 12) }, + 'Invalid hash', + );packages/core/package.json (1)
21-22: pino version consistency is confirmed; add Node.js engine constraint
- Only packages/core/package.json includes pino@^9.11.0 — no duplicates.
- Add an
engines.nodefield (e.g.">=18") in package.json (root or relevant packages) to enforce Node.js compatibility with pino v9.
📜 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 ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
package.json(0 hunks)packages/core/package.json(1 hunks)packages/core/src/utils/checkSignAndHashes.ts(1 hunks)packages/core/src/utils/makeRequest.ts(0 hunks)packages/federation-sdk/package.json(1 hunks)packages/federation-sdk/src/queues/base.queue.ts(1 hunks)packages/federation-sdk/src/services/config.service.ts(1 hunks)packages/federation-sdk/src/services/event-authorization.service.ts(2 hunks)packages/federation-sdk/src/services/event-emitter.service.ts(2 hunks)packages/federation-sdk/src/services/event.service.ts(5 hunks)packages/federation-sdk/src/services/federation.service.ts(2 hunks)packages/federation-sdk/src/services/invite.service.ts(1 hunks)packages/federation-sdk/src/services/state.service.ts(3 hunks)packages/federation-sdk/src/utils/logger.ts(0 hunks)
💤 Files with no reviewable changes (3)
- packages/core/src/utils/makeRequest.ts
- packages/federation-sdk/src/utils/logger.ts
- package.json
🔇 Additional comments (8)
packages/federation-sdk/package.json (1)
19-19: Move-only dependency change looks goodReordering
@rocket.chat/emitterwithin dependencies has no functional impact. Safe change.packages/federation-sdk/src/services/config.service.ts (1)
76-83: Approve structured logging and key-loading flow createLogger returns a pino child logger supporting object+message calls, and getKeyPair resolves to a SigningKey array.packages/federation-sdk/src/services/event-authorization.service.ts (1)
238-239: LGTM: structured error with contextConsistent with the new logging pattern.
packages/federation-sdk/src/services/event.service.ts (1)
233-236: LGTM: structured validation error logsGood shape and field names; easy to parse and correlate.
Also applies to: 244-246
packages/federation-sdk/src/services/state.service.ts (3)
1-1: LGTM: centralized logger and signEvent importAligns with the repo-wide logger consolidation.
156-156: LGTM: structured error with contextClear message and machine-parsable fields.
641-643: LGTM: structured debug with explicit msgGood contextualization while keeping fields queryable.
packages/core/package.json (1)
26-26: pino-pretty only in dev is correctKeeping pino-pretty as a devDependency avoids bundling it in production. LGTM.
If other packages still have pino-pretty, consider consolidating it here or at the root, depending on where it’s used. The script above will list all occurrences.
| HttpException, | ||
| HttpStatus, | ||
| } from '@rocket.chat/federation-core'; | ||
| import { EventBase, createLogger } from '@rocket.chat/federation-core'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Centralizing logger import: looks good. Please verify no leftover local logger/console usage.
Great switch to the core logger.
Run to confirm the SDK no longer imports a local logger or uses console/pino directly:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for legacy local logger imports…"
rg -nP -C2 "(from|require\()\\s*['\"][^'\\\"]*utils/logger['\\\"]" --type=ts --type=tsx || true
echo
echo "Searching for direct pino usage…"
rg -nP -C2 "\\bpino\\b" -g "!**/node_modules/**" --type=ts --type=tsx || true
echo
echo "Searching for console.* calls (exclude tests)…"
rg -nP -C1 "\\bconsole\\.(log|info|warn|error|debug)\\s*\\(" --type=ts --type=tsx -g "!**/test/**" || trueLength of output: 208
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for legacy local logger imports…"
rg -nP -C2 "(?:from|require\()\\s*['\"][^'\"]*utils/logger['\"]" -g "*.{ts,tsx}" -g "!node_modules/**" || true
echo
echo "Searching for direct pino usage…"
rg -nP -C2 "\\bpino\\b" -g "*.{ts,tsx}" -g "!node_modules/**" || true
echo
echo "Searching for console.* calls (exclude tests)…"
rg -nP -C1 "\\bconsole\\.(?:log|info|warn|error|debug)\\s*\\(" -g "*.{ts,tsx}" -g "!**/test/**" || trueLength of output: 3839
Eliminate direct pino imports and console calls across the SDK
- packages/federation-sdk/src/server-discovery/discovery.ts:2 – remove
import type { Logger } from 'pino'; use the core-exported logger API. - packages/federation-sdk/src/services/invite.service.ts:181 – replace
console.error(e)withlogger.error(e). - packages/federation-sdk/src/services/room.service.ts:911–912 – replace
console.log(…)with the appropriatelogger.debug(…).
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/invite.service.ts around line 181, there
is a direct console.error(e) call; replace it with the SDK logger: ensure an
instance is created from the core createLogger (or reuse the file's existing
logger) and call logger.error(e) instead of console.error(e); if no logger
instance exists in this module/class, create one via
createLogger('federation:invite-service') at the top and use that instance for
the error call.
3 participants
Summary by CodeRabbit
Refactor
Chores
Note