chore: Adapt logs to object format (final!) #38289
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughStandardizes server logging by renaming catch variables to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 #38289 +/- ##
===========================================
+ Coverage 70.77% 70.79% +0.02%
===========================================
Files 3158 3158
Lines 109363 109355 -8
Branches 19674 19634 -40
===========================================
+ Hits 77401 77422 +21
+ Misses 29931 29904 -27
+ Partials 2031 2029 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
📦 Docker Image Size Report➡️ Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "01/22 18:09 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.00]
line "omnichannel-transcript-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "queue-worker-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "rocketchat" [0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.00]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Thu, 22 Jan 2026 18:09:02 GMT |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/meteor/app/slackbridge/server/slackbridge.js (1)
152-159: Consider masking API token in debug logs.Similar to the bot/app tokens above,
SlackBridge_APITokenis also sensitive and should not be logged in plain text.Proposed fix
settings.watch('SlackBridge_APIToken', (value) => { if (value !== this.apiTokens) { this.apiTokens = value; this.debouncedReconnectIfEnabled(); } - classLogger.debug({ msg: 'Setting: SlackBridge_APIToken', value }); + classLogger.debug({ msg: 'Setting: SlackBridge_APIToken', value: value ? '[REDACTED]' : '' }); });apps/meteor/app/slackbridge/server/SlackAdapter.js (1)
226-233: Fix misleading event label for member join.The log message says
OnSlackEvent-CHANNEL_LEFTfor amember_joined_channelevent, which is confusing during troubleshooting.Suggested fix
- slackLogger.debug({ msg: 'OnSlackEvent-CHANNEL_LEFT', event }); + slackLogger.debug({ msg: 'OnSlackEvent-MEMBER_JOINED_CHANNEL', event });apps/meteor/app/meteor-accounts-saml/server/lib/parsers/Response.ts (1)
420-441: Redact attribute payloads in logs to avoid leaking user data.
Logging raw attribute statements and values can expose PII (email, name, identifiers). Prefer logging counts or attribute names only.🔐 Suggested redaction
- SAMLUtils.log({ msg: 'Attribute Statement found, mapping attributes to profile.', attributeStatement }); - const attributes = attributeStatement.getElementsByTagNameNS('urn:oasis:names:tc:SAML:2.0:assertion', 'Attribute'); - SAMLUtils.log({ msg: 'Attributes will be processed', count: attributes.length }); + const attributes = attributeStatement.getElementsByTagNameNS('urn:oasis:names:tc:SAML:2.0:assertion', 'Attribute'); + SAMLUtils.log({ msg: 'Attribute Statement found, mapping attributes to profile.', attributeCount: attributes.length }); ... - SAMLUtils.log({ msg: 'Mapping attribute to profile', attribute: key, value }); + SAMLUtils.log({ + msg: 'Mapping attribute to profile', + attribute: key, + valueType: Array.isArray(value) ? 'array' : 'string', + valueCount: Array.isArray(value) ? value.length : value ? 1 : 0, + });
🤖 Fix all issues with AI agents
In `@apps/meteor/app/meteor-accounts-saml/server/lib/parsers/Response.ts`:
- Around line 20-23: The current validate method in Response.ts calls
SAMLUtils.log with the full SAML response XML (SAMLUtils.log({ msg: 'Validating
SAML Response', xml })), which can leak PII; update the validate function to
stop emitting raw xml and instead log a safe summary (e.g., length,
RelayState/correlation id, and a hash like SHA256 of the xml) or a redacted
snippet that removes assertion/attribute elements; replace the SAMLUtils.log
call to include only these safe fields and any existing relayState extraction so
callers of validate and the SAMLUtils.log usage are updated accordingly.
In `@apps/meteor/app/slackbridge/server/slackbridge.js`:
- Around line 132-149: The debug logs currently print raw secret values in the
settings.watch callbacks for SlackBridge_BotToken, SlackBridge_AppToken, and
SlackBridge_SigningSecret; update those log statements in the callbacks (the
handlers that set this.botTokens, this.appTokens, this.signingSecrets and call
this.debouncedReconnectIfEnabled()) to avoid printing the raw value — either
omit the value entirely or log a masked placeholder (e.g., show only last 4
chars or “(redacted)”) and keep the same descriptive message so you still know
which setting changed without exposing the secret in classLogger.debug.
🧹 Nitpick comments (11)
apps/meteor/server/services/omnichannel-integrations/providers/twilio.ts (1)
263-265: Keep structured logging for consistency.This log is now a plain string while the PR standardizes structured payloads. Consider logging an object (e.g.,
{ msg, authTokenConfigured, siteUrlConfigured }) to preserve consistency and add context.ee/packages/media-calls/src/server/CallDirector.ts (1)
242-244: Add context fields to the structured error log.The new structured log is fine, but it drops key context (e.g., call id / agent role). Including them will make the error actionable without changing behavior.
♻️ Suggested update
- logger.error({ msg: 'Unable to transfer calls without a reference to the opposite agent.' }); + logger.error({ + msg: 'Unable to transfer calls without a reference to the opposite agent.', + callId: call._id, + agentRole: agent.role, + agentType: agent.actor.type, + });apps/meteor/server/services/nps/getAndCreateNpsSurvey.ts (1)
65-66: Consider adding amsgproperty for consistency.Line 42 includes a descriptive
msgin the structured log. Adding one here would improve log searchability and debugging clarity.♻️ Suggested improvement
} catch (err) { - SystemLogger.error({ err }); + SystemLogger.error({ msg: 'Failed to get or create NPS survey', err }); return false; }apps/meteor/app/livechat/server/lib/rooms.ts (1)
254-259: Add room context to the error log.
Logging only{ err }makes triage harder; includeroomId/departmentId(and optionally a staticmsg) for correlation.♻️ Suggested tweak
- livechatLogger.error({ err }); + livechatLogger.error({ + msg: 'Failed to return room as inquiry', + roomId: room._id, + departmentId, + err, + });apps/meteor/app/webdav/server/methods/uploadFileToWebdav.ts (1)
42-42: Consider simplifying the redundant type guard.The
typeof err === 'object'check is redundant sinceinstanceof Erroralready safely returnsfalsefor primitives andnullwithout throwing.Suggested simplification
- if (typeof err === 'object' && err instanceof Error && err.name === 'error-invalid-account') { + if (err instanceof Error && err.name === 'error-invalid-account') {apps/meteor/ee/server/apps/communication/rest.ts (1)
1218-1220: Minor inconsistency: catch variable not renamed.Other catch blocks in this PR rename
etoerrfor consistency. Here the catch variable remainsewhile the log useserr: e. Consider renaming for uniformity.Suggested change
- } catch (e) { - orchestrator.getRocketChatLogger().warn({ msg: 'Could not fetch cluster status for app', appId: app.getID(), err: e }); + } catch (err) { + orchestrator.getRocketChatLogger().warn({ msg: 'Could not fetch cluster status for app', appId: app.getID(), err }); }apps/meteor/app/importer-pending-files/server/PendingFileImporter.ts (1)
160-168: Inconsistent error variable naming.The outer catch block at line 160 still uses
errorwhile other catch blocks in this file useerr. This is inconsistent with the PR's goal of standardizing toerr.Proposed fix
- } catch (error) { + } catch (err) { // If the cursor expired, restart the method - if (this.isCursorNotFoundError(error)) { + if (this.isCursorNotFoundError(err)) { this.logger.info('CursorNotFound'); return this.startImport(importSelection); } await super.updateProgress(ProgressStep.ERROR); - throw error; + throw err; }apps/meteor/app/slackbridge/server/slackbridge.js (1)
111-113: Inconsistent error variable naming.The catch block uses
errorbut logs it aserr: error. For consistency with other files in this PR, consider renaming the catch variable toerr.Proposed fix
- } catch (error) { - connLogger.error({ msg: 'An error occurred during disconnection', err: error }); + } catch (err) { + connLogger.error({ msg: 'An error occurred during disconnection', err }); }ee/packages/omnichannel-services/src/OmnichannelTranscript.ts (1)
164-257: Consider structuring the MAX_PAYLOAD_EXCEEDED log for consistency.The rest of this block uses structured logs; keeping this branch consistent will improve log parsing.
Suggested tweak
- this.log.error( - `File is too big to be processed by NATS. See NATS config for allowing bigger messages to be sent between services`, - ); + this.log.error({ + msg: 'File too large for NATS payload', + errorCode: 'MAX_PAYLOAD_EXCEEDED', + });apps/meteor/server/lib/ldap/Connection.ts (1)
420-423: Consider structuring this debug log for consistency.This is a minor alignment with the rest of the object-based logging in this file.
Suggested tweak
- searchLogger.debug('LDAP Group Filter is enabled but no group member format is set.'); + searchLogger.debug({ msg: 'LDAP Group Filter is enabled but no group member format is set.' });apps/meteor/app/meteor-accounts-saml/server/lib/Utils.ts (1)
53-56: Consider redacting sensitive fields inproviderListdebug logs.
Structured logs will serialize the full object; if it includes certs/keys or other secrets, debug logs could leak them. Prefer logging only safe fields (e.g., provider names/count) or explicitly redacting sensitive properties.
KevLehman
added
the
stat: QA assured
dionisio-bot
bot
added
stat: ready to merge
KevLehman
changed the title
dionisio-bot
bot
added
stat: ready to merge
dionisio-bot
bot
removed
stat: ready to merge
dionisio-bot
bot
added
stat: ready to merge
dionisio-bot
bot
removed
stat: ready to merge
KevLehman
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
Issue(s)
https://rocketchat.atlassian.net/browse/CORE-1715
Steps to test or reproduce
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.