Some logging improvements #148
Conversation
Useful for debugging, to see the room feature and the address of the underlying ably-cocoa channel (for matching with ably-cocoa logs).
Useful for debugging.
WalkthroughThe pull request introduces enhancements to the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (4)
Sources/AblyChat/DefaultRoomLifecycleContributor.swift (1)
35-35: Consider enhancing the debug descriptionWhile forwarding to
underlyingChannel's description works, consider providing more context about this wrapper class itself.internal var debugDescription: String { - "\(underlyingChannel)" + "DefaultRoomLifecycleContributorChannel(channel: \(underlyingChannel))" }Also applies to: 67-69
Tests/AblyChatTests/IntegrationTests.swift (2)
10-20: LGTM: Well-structured logger implementationsBoth logger implementations (AblyCocoaLogger and ChatLogger) are well-designed:
- Clean separation of concerns
- Consistent labeling approach
- Proper use of inheritance/protocol implementation
Consider adding documentation comments to explain the purpose and usage of these loggers, especially since they're part of a debugging enhancement.
Also applies to: 22-33
Line range hint
1-300: Consider documenting the logging structureWhile the implementation is solid, consider adding documentation that explains:
- The overall logging strategy
- When and how to enable logging (TestLogger.loggingEnabled)
- The meaning of different log labels
- How to interpret the logs when debugging issues
This would help other developers effectively use these logging improvements when investigating issues.
Sources/AblyChat/RoomLifecycleManager.swift (1)
Line range hint
796-838: Consider adding more detailed logging for attachment cycle state transitions.While the error handling is thorough, consider adding more detailed logging to help with debugging:
- Log the start of the attachment cycle
- Log when transitioning between different states in the error handling
- Log the completion of the attachment cycle
This would provide a more complete picture of the attachment cycle's progress and help with debugging issue #121.
Example additions:
private func performAttachmentCycle() async throws(ARTErrorInfo) { + logger.log(message: "Starting attachment cycle", level: .info) // CHA-RL1f for contributor in contributors { do { logger.log(message: "Attaching contributor \(contributor)", level: .info) try await contributor.channel.attach() logger.log(message: "Successfully attached contributor \(contributor)", level: .info) } catch let contributorAttachError { let contributorState = await contributor.channel.state logger.log(message: "Failed to attach contributor \(contributor), which is now in state \(contributorState), error \(contributorAttachError)", level: .info) switch contributorState { case .suspended: + logger.log(message: "Handling suspended state for contributor \(contributor)", level: .info) // CHA-RL1h2 let error = ARTErrorInfo(chatError: .attachmentFailed(feature: contributor.feature, underlyingError: contributorAttachError)) // CHA-RL1h3 let retryOperationTask = scheduleAnOperation( kind: .retry( triggeringContributor: contributor, errorForSuspendedStatus: error ) ) changeStatus(to: .suspendedAwaitingStartOfRetryOperation(retryOperationTask: retryOperationTask, error: error)) throw error case .failed: + logger.log(message: "Handling failed state for contributor \(contributor)", level: .info) // CHA-RL1h4 let error = ARTErrorInfo(chatError: .attachmentFailed(feature: contributor.feature, underlyingError: contributorAttachError)) changeStatus(to: .failed(error: error)) // CHA-RL1h5 await detachNonFailedContributors() throw error default: preconditionFailure("Attach failure left contributor in unexpected state \(contributorState)") } } } + logger.log(message: "Completed attachment cycle successfully", level: .info) // CHA-RL1g3 clearTransientDisconnectTimeouts()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Sources/AblyChat/DefaultRoomLifecycleContributor.swift(3 hunks)Sources/AblyChat/RoomLifecycleManager.swift(1 hunks)Tests/AblyChatTests/IntegrationTests.swift(2 hunks)
🧰 Additional context used
📓 Learnings (1)
Sources/AblyChat/RoomLifecycleManager.swift (1)
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#54
File: Sources/AblyChat/RoomLifecycleManager.swift:0-0
Timestamp: 2024-11-12T15:07:31.866Z
Learning: In `RoomLifecycleManager`, handling of unexpected contributor states after attach failure will be addressed in issue #49.
🔇 Additional comments (5)
Sources/AblyChat/DefaultRoomLifecycleContributor.swift (1)
26-26: 💡 Codebase verification
Based on the search results, I can now provide a final response. The codebase shows multiple instances of subscription arrays being stored without cleanup, and there's a TODO comment in Subscription.swift specifically mentioning the need to revisit unsubscribe functionality. The issue is well-documented across the codebase.
Consider implementing subscription cleanup using the existing finish() method
The codebase already has a finish() method in the Subscription class that can be used for cleanup. This method should be called when subscriptions are no longer needed. Specific areas needing attention:
DefaultRoomLifecycleContributor.swift: Clean updiscontinuitySubscriptionswhen no longer neededRoomLifecycleManager.swift: Implement cleanup for multiple subscription arraysDefaultMessages.swift: Add cleanup forsubscriptionPoints
🔗 Analysis chain
Consider addressing subscription cleanup TODO
Both classes have TODO comments about cleaning up old subscriptions (issue #36). Since we're improving logging and debugging capabilities, this might be a good time to address these potential memory leaks as well.
Consider implementing a subscription management system that:
- Tracks subscription lifetimes
- Automatically removes stale subscriptions
- Provides cleanup hooks when subscribers are deallocated
Also applies to: 57-57
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any existing implementations of subscription cleanup in the codebase
rg -A 5 "func cleanup.*Subscription"
# Check for any similar patterns in other files
rg -A 5 "discontinuitySubscriptions.*remove"
Length of output: 85
Script:
#!/bin/bash
# Let's search for TODO comments related to subscriptions and cleanup
rg -i "todo.*subscription" -A 3
# Search for any subscription-related code patterns
rg -i "subscription|subscribe" -A 3
# Look for any cleanup or removal patterns
ast-grep --pattern 'func $FUNC_NAME($$$) {
$$$
remove($$$)
$$$
}'
Length of output: 127377
⛔ Skipped due to learnings
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#120
File: Sources/AblyChat/RoomLifecycleManager.swift:322-324
Timestamp: 2024-11-19T13:22:50.638Z
Learning: In `Sources/AblyChat/RoomLifecycleManager.swift`, the `onRoomStatusChange(bufferingPolicy:)` method already has a TODO to manage subscriptions properly. Avoid flagging this issue in future reviews.
Tests/AblyChatTests/IntegrationTests.swift (3)
2-2: LGTM: Appropriate use of @testable import
The @testable import allows access to internal components for testing purposes.
62-63: LGTM: Clear and descriptive logging labels
The use of "tx" and "rx" as logging labels provides clear distinction between transmitting and receiving clients in the logs.
35-52: LGTM: Clean implementation of logging configuration
The utility methods have been updated appropriately to support labeled logging:
- Consistent parameter usage
- Clean conditional logging setup
- Good separation of concerns between Ably and Chat logging
Let's verify that TestLogger.loggingEnabled is properly defined:
✅ Verification successful
TestLogger.loggingEnabled is properly defined but disabled by default
The implementation is correct, but it's worth noting that TestLogger.loggingEnabled is set to false by default, which means the logging configuration in the utility methods won't take effect unless explicitly enabled. This is intentional as per the comment: "By default, we don't log in tests to keep the test logs easy to read."
The utility methods handle this gracefully:
- When
loggingEnabledis false, no logging handlers are configured - When
loggingEnabledis true, proper logging is set up with the provided labels
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for TestLogger definition and usage
rg -A 3 "class TestLogger"
Length of output: 28
Script:
#!/bin/bash
# Let's try different search patterns to locate TestLogger
rg -l "TestLogger"
Length of output: 351
Script:
#!/bin/bash
# Search for any logging-related configuration or setup
rg "loggingEnabled" -A 3
Length of output: 1344
Script:
#!/bin/bash
# Search for test configuration files that might contain logger setup
fd -e swift "Test.*Config"
Length of output: 28
Script:
#!/bin/bash
# Search for files containing both "Test" and "Logger" in their names
fd ".*Test.*Logger.*" -e swift
Length of output: 77
Script:
#!/bin/bash
# Let's examine the TestLogger implementation
cat Tests/AblyChatTests/Helpers/TestLogger.swift
Length of output: 663
Sources/AblyChat/RoomLifecycleManager.swift (1)
809-809: LGTM! Enhanced logging for successful contributor attachment.
The added logging statement improves debugging capabilities by providing clear feedback when a contributor is successfully attached. This complements the existing error logging and helps track the progress of contributor attachments.
Note: This PR is based on top of #147; please review that one first.
A few things I added to help me whilst investigating #121.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests