chore: Adapt logs to object format - 3#38111
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughThis PR systematically converts string-based logger calls to structured object logs across authentication, custom OAuth, IRC bridge, Slack bridge, apps communication, orchestrator, and apps-engine modules. Changes maintain existing control flow and error handling while standardizing logging payloads to include contextual fields like Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 @@
## develop #38111 +/- ##
===========================================
- Coverage 70.66% 70.66% -0.01%
===========================================
Files 3148 3148
Lines 108850 108850
Branches 19628 19559 -69
===========================================
- Hits 76924 76914 -10
- Misses 29922 29928 +6
- Partials 2004 2008 +4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/apps-engine/deno-runtime/handlers/slashcommand-handler.ts (1)
33-48: Avoid logging rawparams(PII/size), and guarderror.messageagainst non-Errorthrows.
Line 33 can dump user-provided content; Lines 45-48 can throw iferrorisn’t anError.Proposed fix
- app.getLogger().debug({ msg: `Command is being executed...`, commandName, method, params }); + app.getLogger().debug({ + msg: 'Command is being executed...', + commandName, + method, + // Avoid dumping arbitrary user input; log shape/keys only. + paramsType: Array.isArray(params) ? 'array' : typeof params, + }); try { if (method === 'executor' || method === 'previewer') { result = await handleExecutor({ AppAccessorsInstance }, command, method, params); } else if (method === 'executePreviewItem') { result = await handlePreviewItem({ AppAccessorsInstance }, command, params); } else { return new JsonRpcError(`Method ${method} not found on slashcommand ${commandName}`, -32000); } app.getLogger().debug({ msg: `Command was successfully executed.`, commandName, method }); - } catch (error) { - app.getLogger().debug({ msg: `Command was unsuccessful.`, commandName, method, err: error }); - - return new JsonRpcError(error.message, -32000); + } catch (error: unknown) { + const err = error instanceof Error ? error : new Error(String(error)); + app.getLogger().debug({ msg: `Command was unsuccessful.`, commandName, method, err }); + + return new JsonRpcError(err.message, -32000); }apps/meteor/app/slackbridge/server/RocketAdapter.js (1)
68-72: Don't log full message objects; log only essential identifiers.The
rocketMessageDeletedobject passed to logging at line 68-71 may contain message content (.msg), file references, attachments, and user metadata that should not be written to logs. ThepostDeleteMessage()method only needs the room ID and timestamp to function; extract and log only those identifiers instead.Proposed fix
- rocketLogger.debug({ msg: 'onRocketMessageDelete', rocketMessageDeleted }); + rocketLogger.debug({ + msg: 'onRocketMessageDelete', + rid: rocketMessageDeleted?.rid, + _id: rocketMessageDeleted?._id, + ts: rocketMessageDeleted?.ts, + });Same pattern exists at line 140 (
onMessagelogs fullrocketMessage).
🤖 Fix all issues with AI agents
In @apps/meteor/app/irc/server/servers/RFC2813/index.js:
- Around line 126-128: The current logging call logs the raw IRC command buffer
and can leak plaintext passwords (from this.config.passwords.local) when a PASS
command is sent; change the code to redact the password for PASS commands by
creating a loggedBuffer derived from buffer (do not mutate buffer), detect PASS
(e.g., check if buffer starts with "PASS " or matches /^PASS\s+/i), replace the
password portion with a placeholder like "REDACTED" in loggedBuffer, call
this.log({ msg: 'Sending Command', buffer: loggedBuffer }), and continue to
write the original buffer to this.socket.write(`${buffer}\r\n`) so only logs are
sanitized while the actual command is sent unchanged.
In @apps/meteor/app/slackbridge/server/RocketAdapter.js:
- Around line 140-141: The debug call currently logs the entire rocketMessage
payload (rocketLogger.debug({ msg: 'onRocketMessage', rocketMessage })),
exposing sensitive data; update the logging in both the onRocketMessage and
onRocketMessageDelete locations to log only essential metadata by constructing
an object with messageId (rocketMessage._id or id), roomId (rocketMessage.rid),
timestamp (rocketMessage.ts or rcpt time), userId (rocketMessage.u &&
rocketMessage.u._id), hasFiles (Boolean(rocketMessage.files &&
rocketMessage.files.length)), and isEdited (Boolean(rocketMessage.editedAt ||
rocketMessage edited flag)), and pass that object to rocketLogger.debug with a
clear msg string instead of the full rocketMessage payload.
In @apps/meteor/ee/server/apps/communication/rest.ts:
- Around line 544-548: The code currently logs the entire marketplace response
body (await request.json()) in orchestrator.getRocketChatLogger().error which
risks PII and large logs; change the error logging in the failure branch of the
bundle-apps fetch to log only the HTTP status (request.status) and a safe error
string extracted from the JSON (e.g., response.errorMsg || response.error ||
response.message) instead of the full body, and apply the same change pattern in
fetchMarketplaceApps.ts and fetchMarketplaceCategories.ts so no full response
JSON is emitted to logs while still returning API.v1.failure().
In @packages/apps-engine/deno-runtime/handlers/app/handler.ts:
- Around line 57-69: The success branch currently logs the entire result object
via app?.getLogger().debug({ msg: `'${appMethod}' was successfully called! The
result is:`, appMethod, result }); — remove the verbose result payload and
instead log only the success message and appMethod (e.g.,
app?.getLogger().debug({ msg: `'${appMethod}' was successfully called!`,
appMethod })); apply the same change for the other occurrence mentioned for
UIKit interactions (the earlier debug call around lines 43–47) so success logs
do not include the full result object.
- Around line 36-48: Fix the double-space typo in the success log message and
stop logging the full result object; in the success branch of
app?.getLogger().debug (where appMethod and result are used) change the message
to "Method was successfully called" (single space) and replace the full result
with a bounded summary (e.g., a truncated string, type+key list or size-limited
JSON) so sensitive fields aren't emitted; keep references to appMethod and
errorMessage as before and ensure the error branch still logs result.message but
not the raw result object.
📜 Review details
Configuration used: Organization 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 (12)
apps/meteor/app/authentication/server/lib/restrictLoginAttempts.tsapps/meteor/app/authentication/server/startup/index.jsapps/meteor/app/custom-oauth/server/custom_oauth_server.jsapps/meteor/app/irc/server/irc-bridge/index.jsapps/meteor/app/irc/server/servers/RFC2813/index.jsapps/meteor/app/slackbridge/server/RocketAdapter.jsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/ee/server/apps/orchestrator.jsapps/meteor/server/services/apps-engine/service.tspackages/apps-engine/deno-runtime/handlers/app/handler.tspackages/apps-engine/deno-runtime/handlers/scheduler-handler.tspackages/apps-engine/deno-runtime/handlers/slashcommand-handler.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/ee/server/apps/orchestrator.jsapps/meteor/app/authentication/server/lib/restrictLoginAttempts.tsapps/meteor/app/custom-oauth/server/custom_oauth_server.jsapps/meteor/app/irc/server/irc-bridge/index.jspackages/apps-engine/deno-runtime/handlers/scheduler-handler.tspackages/apps-engine/deno-runtime/handlers/app/handler.tsapps/meteor/server/services/apps-engine/service.tsapps/meteor/app/irc/server/servers/RFC2813/index.jsapps/meteor/app/slackbridge/server/RocketAdapter.jsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/app/authentication/server/startup/index.jspackages/apps-engine/deno-runtime/handlers/slashcommand-handler.ts
🧠 Learnings (10)
📚 Learning: 2026-01-08T15:07:47.836Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38068
File: packages/apps-engine/deno-runtime/handlers/lib/assertions.ts:8-12
Timestamp: 2026-01-08T15:07:47.836Z
Learning: In packages/apps-engine/deno-runtime, App objects are never serialized; they are instantiated and kept in the runtime context. Therefore, runtime assertions can rely on duck-typing checks of App methods (including protected ones like extendConfiguration) as reliable. When reviewing code in this area, prefer duck-typing checks for App behavior in assertions and avoid assuming serialization/deserialization will preserve App state. This guideline applies to files under this directory.
Applied to files:
packages/apps-engine/deno-runtime/handlers/scheduler-handler.tspackages/apps-engine/deno-runtime/handlers/app/handler.tspackages/apps-engine/deno-runtime/handlers/slashcommand-handler.ts
📚 Learning: 2025-11-05T20:53:57.761Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: apps/meteor/ee/server/startup/federation.ts:39-74
Timestamp: 2025-11-05T20:53:57.761Z
Learning: In Rocket.Chat (apps/meteor/app/settings/server/CachedSettings.ts), the settings.watchMultiple() method immediately invokes its callback with current values if all requested settings exist in the store, then continues watching for subsequent changes. It does not wait for a setting to change before the first invocation.
Applied to files:
apps/meteor/server/services/apps-engine/service.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/server/services/apps-engine/service.tsapps/meteor/app/slackbridge/server/RocketAdapter.jsapps/meteor/ee/server/apps/communication/rest.ts
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
apps/meteor/app/slackbridge/server/RocketAdapter.jsapps/meteor/ee/server/apps/communication/rest.ts
📚 Learning: 2026-01-08T15:03:45.071Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38068
File: apps/meteor/tests/data/apps/app-packages/README.md:14-16
Timestamp: 2026-01-08T15:03:45.071Z
Learning: For the RocketChat/Rocket.Chat repository, do not analyze or report formatting issues (such as hard tabs vs spaces, line breaks, etc.). The project relies on automated linting tools to enforce formatting standards.
Applied to files:
apps/meteor/app/slackbridge/server/RocketAdapter.js
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
apps/meteor/app/slackbridge/server/RocketAdapter.jsapps/meteor/ee/server/apps/communication/rest.ts
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
apps/meteor/app/slackbridge/server/RocketAdapter.js
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/app/slackbridge/server/RocketAdapter.jsapps/meteor/app/authentication/server/startup/index.js
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
apps/meteor/app/authentication/server/startup/index.js
📚 Learning: 2025-12-02T22:23:49.593Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37654
File: apps/meteor/client/hooks/useAppSlashCommands.ts:32-38
Timestamp: 2025-12-02T22:23:49.593Z
Learning: In apps/meteor/client/hooks/useAppSlashCommands.ts, the `data?.forEach((command) => slashCommands.add(command))` call during render is intentional. The query is configured with `structuralSharing: false` to prevent React Query from keeping stable data references, and `slashCommands.add` is idempotent, so executing on every render is acceptable and ensures the command registry stays current.
Applied to files:
packages/apps-engine/deno-runtime/handlers/slashcommand-handler.ts
🧬 Code graph analysis (2)
apps/meteor/app/custom-oauth/server/custom_oauth_server.js (2)
apps/meteor/app/irc/server/irc-bridge/index.js (1)
logger(16-16)apps/meteor/app/irc/server/servers/RFC2813/index.js (1)
logger(11-11)
apps/meteor/app/irc/server/servers/RFC2813/index.js (2)
apps/meteor/app/custom-oauth/server/custom_oauth_server.js (1)
logger(21-21)apps/meteor/app/irc/server/irc-bridge/index.js (1)
logger(16-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (19)
apps/meteor/app/authentication/server/startup/index.js (1)
383-385: LGTM: Structured logging improves observability.The conversion to object-based logging with explicit
msganderrfields enhances log parsing and error tracking.packages/apps-engine/deno-runtime/handlers/scheduler-handler.ts (1)
31-47: LGTM: Enhanced scheduler observability.The structured logging format with
processorIdenables effective tracing of job processor execution lifecycle across success and error paths.apps/meteor/app/authentication/server/lib/restrictLoginAttempts.ts (1)
25-25: LGTM: Improved error context and grammar.The structured format with
channelToNotifyhelps diagnose misconfiguration, and the grammar correction ("does not exist") is a nice touch.apps/meteor/app/irc/server/irc-bridge/index.js (1)
138-138: LGTM: Structured queue logging.The object format with
commandandfromfields facilitates filtering and debugging of IRC bridge message flow.apps/meteor/app/custom-oauth/server/custom_oauth_server.js (2)
28-28: LGTM: Structured initialization logging.The structured format aids OAuth provider debugging. The
optionsobject contains configuration metadata that's appropriate for debug-level logs.
310-310: LGTM: Improved avatar debugging context.The structured log with
avatarFieldanddatahelps troubleshoot OAuth avatar configuration issues during development.apps/meteor/ee/server/apps/communication/rest.ts (2)
394-399: Structured warn log looks good; consider minimizingerrpayload if it can include secrets.
519-523: Good conversion to{ msg, err }; ensureeis safe to serialize/log in production.apps/meteor/app/slackbridge/server/RocketAdapter.js (2)
263-264: Nice: structured log with a small, non-sensitive field.
299-305: Structured retry/error logs look good; verifyerrwon’t include sensitive Slack tokens.apps/meteor/app/irc/server/servers/RFC2813/index.js (5)
42-45: Socket event logs converted cleanly to structured payloads.
52-60:log()wrapper is good; consider also wrapping non-object / missing-msginputs for consistency.
[uggest_recommended_refactor]Proposed tweak
log(message) { // TODO logger: debug? if (typeof message === 'string') { logger.info({ msg: message }); return; } - - logger.info(message); + if (!message || typeof message !== 'object' || !('msg' in message)) { + logger.info({ msg: String(message) }); + return; + } + logger.info(message); }
66-70: Connecting log now carries host/port context—nice.
160-170: Added “unhandled peer message” observability is a net win.
184-189: Added “unhandled local command” observability is a net win.packages/apps-engine/deno-runtime/handlers/app/handler.ts (2)
31-32: Entry log format is consistent and low-risk.
112-116: Final structured “success” log is consistent; consider summarizingresultto keep logs bounded.apps/meteor/ee/server/apps/orchestrator.js (1)
189-193: LGTM! Structured logging migration is well-executed.The conversion from string-based to structured object logs is consistent and improves observability. All log statements properly include contextual fields (
msg,appName,appCount,targetVersion,err, etc.), making debugging and monitoring more effective.Also applies to: 201-204, 244-248, 252-256, 317-322, 324-327, 341-346
apps/meteor/server/services/apps-engine/service.ts (1)
34-37: LGTM! Event handler logging is now properly structured.The migration to structured logging across all event handlers (
apps.added,apps.removed,apps.updated,apps.statusUpdate,apps.settingUpdated) is consistent and complete. Each handler now includes appropriate contextual fields (appId,status,setting), significantly improving traceability and debugging capabilities.Also applies to: 42-45, 53-56, 59-62, 70-73, 76-79, 97-101, 104-108, 113-117, 129-133, 143-147
https://rocketchat.atlassian.net/browse/CORE-1715
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.