Skip to content

Conversation

@sampaiodiego
Copy link
Member

@sampaiodiego sampaiodiego commented Oct 6, 2025

Summary by CodeRabbit

  • New Features

    • None.
  • Refactor

    • Adopted structured logging across services to standardize info/debug/error messages with contextual fields for clearer diagnostics.
    • Improved consistency of logs in error and success paths without changing behavior.
  • Chores

    • Replaced ad-hoc console output with centralized structured logs to aid troubleshooting and monitoring.
    • Enhanced log payloads to include relevant identifiers and context, improving traceability during operations.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 6, 2025

Walkthrough

Logging across three federation SDK services was refactored from string/positional arguments to structured objects. Error, info, and debug calls now pass contextual fields (e.g., msg, err, identifiers). No control-flow, API surface, or exception behavior changes; functions continue to return/throw as before.

Changes

Cohort / File(s) Summary
Structured logging migration (services)
packages/federation-sdk/src/services/event-fetcher.service.ts, packages/federation-sdk/src/services/federation.service.ts, packages/federation-sdk/src/services/room.service.ts
Replaced string-based logger calls with structured objects ({ msg, err, ...context }). Updated debug/info payloads to include identifiers (eventIds, targetServerName, transaction, roomId, eventId). Control flow and public APIs unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • ggazzo
  • rodrigok

Poem

In logs I hop, with keys in tow,
From stringy thickets to fields that show.
msg and err now neatly aligned,
A carrot of context, crisply defined.
I thump the ground—no flow askew—
Just tidier tracks the servers view. 🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title “chore: improve logs” succinctly captures the main objective of the pull request, which is to enhance logging by replacing string-based messages with structured log objects across multiple services. It is concise, clear, and directly related to the changes introduced.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore-improve-logs

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.84%. Comparing base (14b0c72) to head (4568d39).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #250   +/-   ##
=======================================
  Coverage   81.84%   81.84%           
=======================================
  Files          63       63           
  Lines        4705     4705           
=======================================
  Hits         3851     3851           
  Misses        854      854           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 (2)
packages/federation-sdk/src/services/federation.service.ts (1)

264-267: LGTM! Consider transaction log verbosity.

The structured log now includes the full transaction object, which provides complete context but may be verbose in production logs if transactions contain large PDU arrays.

If transaction logs become too verbose, consider logging a summary instead:

-			this.logger.info({
-				transaction: txn,
-				msg: `Sending event ${event.eventId} to server: ${server}`,
-			});
+			this.logger.info({
+				msg: `Sending event ${event.eventId} to server: ${server}`,
+				eventId: event.eventId,
+				server,
+				pduCount: txn.pdus.length,
+			});
packages/federation-sdk/src/services/room.service.ts (1)

912-916: Consider log verbosity in joinUser event persistence.

The structured logs now include full event objects at each persistence step, which provides comprehensive debugging context but may generate significant log volume for complex room joins with many state events.

If log volume becomes excessive, consider logging event summaries in production:

logger.info({
  msg: 'Persisting create event',
  eventId: event.eventId,
  eventType: event.event.type,
  roomId: event.roomId,
});

Keep the full event logging for development/debug levels only.

Also applies to: 943-947, 959-963, 977-981, 992-996

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 14b0c72 and 4568d39.

📒 Files selected for processing (3)
  • packages/federation-sdk/src/services/event-fetcher.service.ts (1 hunks)
  • packages/federation-sdk/src/services/federation.service.ts (11 hunks)
  • packages/federation-sdk/src/services/room.service.ts (8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/federation.service.ts (1)
packages/room/src/manager/event-wrapper.ts (1)
  • event (106-115)
🔇 Additional comments (6)
packages/federation-sdk/src/services/event-fetcher.service.ts (1)

123-131: LGTM! Structured logging improves observability.

The shift to structured logging with contextual fields (msg, err, eventIds, targetServerName) enhances log searchability and analysis. The error-handling logic remains unchanged.

packages/federation-sdk/src/services/federation.service.ts (2)

61-61: LGTM! Consistent structured error logging.

All error logs now use structured objects with msg and err fields, improving log parsing and observability across federation operations.

Also applies to: 98-98, 120-120, 141-141, 154-154, 173-173, 186-186, 201-201


85-85: LGTM! Enhanced debug logging with event context.

The structured debug logs now include the full event object, providing complete context for invalid parameter scenarios.

Also applies to: 214-214

packages/federation-sdk/src/services/room.service.ts (3)

214-214: LGTM! Consistent structured logging for room operations.

The upsertRoom method now uses structured logs with msg, roomId, and err fields, improving traceability and error diagnosis.

Also applies to: 216-216


1000-1003: LGTM! State visibility before join event.

The structured log provides a clear snapshot of room state keys before persisting the join event, useful for debugging state resolution issues.


1129-1133: LGTM! Consistent structured error logging.

Error logs now include relevant identifiers (roomId, userId1, userId2) along with msg and err, improving error traceability.

Also applies to: 1429-1434

@ggazzo ggazzo merged commit 6913a67 into main Oct 6, 2025
3 checks passed
@ggazzo ggazzo deleted the chore-improve-logs branch October 6, 2025 14:10
This was referenced Oct 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants