[ECO-4965] Implement logging #39
Conversation
WalkthroughThe changes introduce a comprehensive logging system to the Changes
Assessment against linked issues
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 (
|
lawrence-forooghian
force-pushed
the
38-implement-logging
branch
from
548468d
to
20518a5
Compare
lawrence-forooghian
force-pushed
the
38-implement-logging
branch
from
20518a5
to
ead9e26
Compare
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (13)
- Sources/AblyChat/ChatClient.swift (1 hunks)
- Sources/AblyChat/Logging.swift (1 hunks)
- Sources/AblyChat/Room.swift (2 hunks)
- Sources/AblyChat/RoomStatus.swift (2 hunks)
- Sources/AblyChat/Rooms.swift (2 hunks)
- Tests/AblyChatTests/DefaultInternalLoggerTests.swift (1 hunks)
- Tests/AblyChatTests/DefaultRoomStatusTests.swift (1 hunks)
- Tests/AblyChatTests/DefaultRoomTests.swift (4 hunks)
- Tests/AblyChatTests/DefaultRoomsTests.swift (3 hunks)
- Tests/AblyChatTests/Helpers/TestLogger.swift (1 hunks)
- Tests/AblyChatTests/InternalLoggerTests.swift (1 hunks)
- Tests/AblyChatTests/Mocks/MockInternalLogger.swift (1 hunks)
- Tests/AblyChatTests/Mocks/MockLogHandler.swift (1 hunks)
Files skipped from review due to trivial changes (1)
- Tests/AblyChatTests/Helpers/TestLogger.swift
Additional comments not posted (29)
Tests/AblyChatTests/Mocks/MockLogHandler.swift (2)
3-3: Clarify the use of@SynchronizedAccessand@unchecked Sendable.Please confirm if
@SynchronizedAccessis a custom attribute or part of Swift's concurrency model. Additionally, consider the implications of using@unchecked Sendablefor bypassing concurrency checks.
6-8: Mock implementation oflogfunction is correct.The
logfunction correctly captures the log message, level, and context, which is suitable for a mock implementation.Tests/AblyChatTests/Mocks/MockInternalLogger.swift (2)
3-3: Clarify the use of@SynchronizedAccessand@unchecked Sendable.Please confirm if
@SynchronizedAccessis a custom attribute or part of Swift's concurrency model. Additionally, consider the implications of using@unchecked Sendablefor bypassing concurrency checks.
6-8: Mock implementation oflogfunction is correct.The
logfunction correctly captures the log message, level, and code location, which is suitable for a mock implementation.Sources/AblyChat/Rooms.swift (1)
14-14: Logging integration inDefaultRoomsis correctly implemented.The addition of the
loggerproperty and its usage in the constructor andgetfunction are correctly implemented. This enhances the logging capabilities of theDefaultRoomsactor, aligning with the PR objectives.Please ensure that the logger is properly integrated and utilized throughout the rest of the codebase.
Also applies to: 19-22, 36-36
Tests/AblyChatTests/InternalLoggerTests.swift (1)
4-31: Thread-safe property wrapper implementation looks good.The
SynchronizedAccessproperty wrapper usesNSLockto ensure thread-safe access to its wrapped value, which is a robust and standard approach in Swift.Tests/AblyChatTests/DefaultRoomStatusTests.swift (3)
6-6: Correct integration of logging in test setup.The addition of the
TestLoggerinstance in theDefaultRoomStatusconstructor is well-aligned with the PR objectives to enhance logging capabilities. The test method correctly verifies the initial state of thecurrentproperty.
12-12: Proper use of logging in test for initial state.The integration of the
TestLoggerin theDefaultRoomStatusconstructor facilitates the verification of the initial state of theerrorproperty and aligns with the PR's focus on enhancing logging capabilities.
19-19: Effective testing of state transitions with logging.The
test_transitionmethod effectively tests the state transition functionality ofDefaultRoomStatus, and the use ofTestLoggerenhances the logging capabilities during the test, which is in line with the PR objectives.Sources/AblyChat/ChatClient.swift (1)
17-23: Proper integration of logging inDefaultChatClient.The introduction of the
loggerproperty and its initialization usingclientOptionsenhance the logging capabilities of the chat client. The use of theloggerin the initialization of theroomsproperty ensures that logging is integrated throughout the chat client's operations, aligning with the PR objectives.Tests/AblyChatTests/DefaultInternalLoggerTests.swift (3)
4-11: Review oftest_defaultsfunction: Well-structured and clear test.This test function checks the default behavior of the
DefaultInternalLoggerwhen no specific log handler or log level is provided. It correctly asserts that the default log handler is an instance ofDefaultLogHandlerand the default log level is.info. The test is concise and covers the expected behavior effectively.
12-29: Review oftest_logfunction: Comprehensive and correctly implemented.This test function verifies the behavior of the
logmethod inDefaultInternalLogger. It checks that the log message, level, and code location are correctly passed to the underlying log handler. The use ofXCTUnwrapto ensure thatlogArgumentsis not nil before proceeding with assertions is a good practice, ensuring the test fails cleanly if the preconditions are not met. The interpolation of the code location into the message is correctly verified.
31-48: Review oftest_log_whenLogLevelArgumentIsLessSevereThanLogLevelProperty_itDoesNotLogfunction: Correctly tests log level filtering.This test function evaluates the logger's behavior when the log level of a message is less severe than the logger's configured log level. It correctly asserts that no logging should occur in this scenario by checking that
logArgumentsremains nil. This test is crucial for ensuring that the log level filtering works as intended, preventing unnecessary log messages from being processed.Sources/AblyChat/RoomStatus.swift (2)
40-44: Review of logger initialization inDefaultRoomStatus: Properly implemented.The
loggerproperty is correctly declared as private and is initialized in the actor's constructor. This encapsulation ensures that the logger can only be modified internally within the actor, which is a good practice for maintaining control over the logging behavior.
57-57: Review of logging intransition(to:)method: Effective use of logging.The addition of a logging statement within the
transition(to:)method is a good practice. It logs the new state to which the actor is transitioning, providing valuable debugging information. The log level.debugis appropriately chosen for this kind of operational detail, which is typically more verbose and intended for debugging rather than regular operation monitoring.Tests/AblyChatTests/DefaultRoomsTests.swift (3)
9-9: Review of logger addition intest_get_returnsRoomWithGivenID: Correctly integrated.The addition of the
loggerparameter in the instantiation ofDefaultRoomswithin the test setup is correctly implemented. This change allows the test to utilize the enhanced logging capabilities of theDefaultRoomsclass, potentially aiding in debugging and verifying the behavior of room retrieval based on ID.
27-27: Review of logger addition intest_get_returnsExistingRoomWithGivenID: Consistent with other test setups.Similar to the previous test, the addition of the
loggerparameter here is consistent and correctly implemented. It ensures that the logging functionality is available during the test, which can be crucial for tracing and debugging the retrieval of existing rooms.
44-44: Review of logger addition intest_get_throwsErrorWhenOptionsDoNotMatch: Enhances test diagnostics.The inclusion of the
loggerparameter in this test setup is particularly useful, as it enhances the diagnostic capabilities when testing error conditions. This is important for understanding the behavior of theDefaultRoomsclass when room options do not match, and it helps ensure that the error handling is robust and well-documented through logs.Sources/AblyChat/Room.swift (4)
27-28: Addition of logger and explicit typing of _status.The addition of the
loggerproperty and the explicit typing of_statusare well-implemented. These changes enhance the clarity and maintainability of the code by making dependencies explicit and improving error logging capabilities.
30-35: Updated initializer to include logger.The initializer now correctly takes a
loggerparameter and uses it to initialize both theloggerproperty and the_statusinstance. This change is consistent with the PR objectives to enhance logging throughout the library.
75-80: Enhanced error handling in attach method.The addition of error logging in the
attachmethod improves the robustness of the method by providing detailed logs in case of failures. This is a crucial enhancement for debugging and monitoring the library's behavior.
87-92: Enhanced error handling in detach method.Similar to the
attachmethod, thedetachmethod now logs errors when channel operations fail. This consistent approach in error handling and logging is beneficial for maintaining the quality and reliability of the library.Sources/AblyChat/Logging.swift (6)
9-9: Enhancement to LogLevel enum.Making
LogLevelcomparable is a useful enhancement that allows for easier comparison and filtering of log levels, aligning with best practices in logging frameworks.
18-24: Addition of CodeLocation struct.The
CodeLocationstruct is a well-thought-out addition that enhances the contextual information available during logging by capturing the file ID and line number.
26-36: Definition of InternalLogger protocol.The
InternalLoggerprotocol is appropriately defined to include alogmethod that requires aCodeLocation, ensuring that log messages are detailed and traceable.
38-44: Convenience method for logging.The extension to
InternalLoggeradds a convenience method for logging that simplifies the process for developers by automatically capturing the file ID and line number. This is a practical enhancement that reduces boilerplate code.
46-63: Implementation of DefaultInternalLogger.The
DefaultInternalLoggerclass is well-implemented, providing a concrete logging mechanism that respects the log level settings. The use of a guard clause to check the log level before logging is a good practice that enhances performance by avoiding unnecessary logging operations.
66-76: Update to DefaultLogHandler.The update to
DefaultLogHandlerto utilize Swift'sLoggertype and handle log messages based on their severity is a robust implementation that aligns with modern logging practices.Tests/AblyChatTests/DefaultRoomTests.swift (1)
19-19: Updated DefaultRoom instantiation in tests.The instantiation of
DefaultRoomin various test cases has been correctly updated to include theloggerparameter. This change is necessary to align with the modifications in the main library code and ensures that the logging functionality is integrated into the testing framework.Also applies to: 56-56, 85-85, 122-122
lawrence-forooghian
force-pushed
the
38-implement-logging
branch
from
ead9e26
to
a318839
Compare
lawrence-forooghian
force-pushed
the
38-implement-logging
branch
from
a318839
to
888f2ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- Sources/AblyChat/ChatClient.swift (1 hunks)
- Sources/AblyChat/Logging.swift (1 hunks)
- Sources/AblyChat/Room.swift (2 hunks)
- Sources/AblyChat/RoomStatus.swift (2 hunks)
- Sources/AblyChat/Rooms.swift (2 hunks)
- Tests/AblyChatTests/DefaultInternalLoggerTests.swift (1 hunks)
- Tests/AblyChatTests/DefaultRoomStatusTests.swift (1 hunks)
- Tests/AblyChatTests/DefaultRoomTests.swift (4 hunks)
- Tests/AblyChatTests/DefaultRoomsTests.swift (3 hunks)
- Tests/AblyChatTests/Helpers/SynchronizedAccess.swift (1 hunks)
- Tests/AblyChatTests/Helpers/TestLogger.swift (1 hunks)
- Tests/AblyChatTests/InternalLoggerTests.swift (1 hunks)
- Tests/AblyChatTests/Mocks/MockInternalLogger.swift (1 hunks)
- Tests/AblyChatTests/Mocks/MockLogHandler.swift (1 hunks)
Files skipped from review due to trivial changes (2)
- Tests/AblyChatTests/Helpers/TestLogger.swift
- Tests/AblyChatTests/Mocks/MockInternalLogger.swift
Files skipped from review as they are similar to previous changes (8)
- Sources/AblyChat/Room.swift
- Sources/AblyChat/RoomStatus.swift
- Sources/AblyChat/Rooms.swift
- Tests/AblyChatTests/DefaultInternalLoggerTests.swift
- Tests/AblyChatTests/DefaultRoomStatusTests.swift
- Tests/AblyChatTests/DefaultRoomTests.swift
- Tests/AblyChatTests/DefaultRoomsTests.swift
- Tests/AblyChatTests/Mocks/MockLogHandler.swift
Additional comments not posted (9)
Tests/AblyChatTests/InternalLoggerTests.swift (1)
4-16: Well-implemented test for logging functionality.The test method
test_protocolExtension_logMessage_defaultArguments_populatesFileIDAndLineis well-structured and correctly verifies the log message, level, and code location. It uses good practices such asXCTUnwrapfor optional values and dynamic code location checks with#lineand#fileID.Tests/AblyChatTests/Helpers/SynchronizedAccess.swift (1)
1-30: Well-implemented property wrapper for thread-safe access.The
SynchronizedAccessproperty wrapper is correctly implemented to provide thread-safe access to properties usingNSLock. The detailed comment at the beginning of the file is informative and helps clarify the intended use and limitations of the property wrapper.Sources/AblyChat/ChatClient.swift (1)
17-23: Well-integrated logging functionality inDefaultChatClient.The addition of the
loggerproperty and its initialization usingclientOptionsare well-implemented. The modification to include theloggerin theroomsproperty initialization ensures that logging is integrated throughout the chat client's operations.Sources/AblyChat/Logging.swift (6)
9-9: Enhancement: LogLevel now supports comparisons.The addition of
ComparabletoLogLevelis a significant enhancement, allowing for easier comparison and filtering of log levels. This change supports more sophisticated log handling scenarios, such as adjusting log output based on the environment or user settings.
18-24: Introduction of CodeLocation struct for better logging context.The
CodeLocationstruct is a well-thought-out addition that encapsulates the file identifier and line number. This will greatly enhance the debugging process by providing precise locations in the code where log messages are generated.
26-36: Review of InternalLogger protocol.The
InternalLoggerprotocol is designed to provide a flexible and powerful logging interface for internal SDK components. It allows detailed logging without exposing too much complexity to the SDK users. The inclusion ofcodeLocationin thelogmethod is particularly useful for associating log messages with their source in the code, enhancing traceability.
38-44: Convenience method for logging with automatic code location capture.This extension to
InternalLoggeradds a convenience method that automatically captures the file ID and line number. This is a practical feature that simplifies the logging process for developers, making it easier to include detailed contextual information without manual effort.
46-64: Implementation of DefaultInternalLogger.The
DefaultInternalLoggerclass is well-implemented, providing a concrete logging mechanism that checks the log level before logging messages. The use of dependency injection for theLogHandlerand the defaulting mechanism for both the handler and log level are good practices that enhance the flexibility and testability of the logging system.
79-91: Mapping LogLevel to OSLogType.The private extension that maps
LogLeveltoOSLogTypeis a clean and efficient way to integrate with the OS logging system. The handling of different log levels and the decision to returnnilfor.silentto effectively skip logging is well thought out.
The aim of this commit is to: - provide infrastructure for internal components to be able to log messages - add some log messages to the existing functionality (I am not particularly concerned by the quality or utility of these log messages at the moment) - provide a basic default logging backend so that users can see log messages in the console The choice to have a separate internal logging interface is taken from Asset Tracking (AAT) and ably-cocoa, where I think it works well. I haven’t currently copied across AAT’s subsystem mechanism, a large part of the motivation for which was to provide a way to integrate the logs emitted by the AAT-instantiated Realtime client into those emitted by AAT itself; this is not currently a goal for Chat, which does not instantiate a Realtime client. I briefly toyed with the idea of writing a macro that would log the inputs and outputs of public API methods, but figuring out how to do that didn’t seem like a good use of time now. Resolves #38.
lawrence-forooghian
force-pushed
the
38-implement-logging
branch
from
888f2ea
to
4b72ffc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- Sources/AblyChat/ChatClient.swift (1 hunks)
- Sources/AblyChat/Logging.swift (1 hunks)
- Sources/AblyChat/Room.swift (2 hunks)
- Sources/AblyChat/RoomStatus.swift (2 hunks)
- Sources/AblyChat/Rooms.swift (2 hunks)
- Tests/AblyChatTests/DefaultInternalLoggerTests.swift (1 hunks)
- Tests/AblyChatTests/DefaultRoomStatusTests.swift (1 hunks)
- Tests/AblyChatTests/DefaultRoomTests.swift (4 hunks)
- Tests/AblyChatTests/DefaultRoomsTests.swift (3 hunks)
- Tests/AblyChatTests/Helpers/SynchronizedAccess.swift (1 hunks)
- Tests/AblyChatTests/Helpers/TestLogger.swift (1 hunks)
- Tests/AblyChatTests/InternalLoggerTests.swift (1 hunks)
- Tests/AblyChatTests/Mocks/MockInternalLogger.swift (1 hunks)
- Tests/AblyChatTests/Mocks/MockLogHandler.swift (1 hunks)
Files skipped from review due to trivial changes (3)
- Sources/AblyChat/RoomStatus.swift
- Tests/AblyChatTests/Helpers/TestLogger.swift
- Tests/AblyChatTests/Mocks/MockInternalLogger.swift
Files skipped from review as they are similar to previous changes (9)
- Sources/AblyChat/ChatClient.swift
- Sources/AblyChat/Logging.swift
- Sources/AblyChat/Rooms.swift
- Tests/AblyChatTests/DefaultInternalLoggerTests.swift
- Tests/AblyChatTests/DefaultRoomStatusTests.swift
- Tests/AblyChatTests/DefaultRoomTests.swift
- Tests/AblyChatTests/DefaultRoomsTests.swift
- Tests/AblyChatTests/Helpers/SynchronizedAccess.swift
- Tests/AblyChatTests/Mocks/MockLogHandler.swift
Additional comments not posted (3)
Tests/AblyChatTests/InternalLoggerTests.swift (1)
4-16: Well-implemented test for logging functionality.The test method
test_protocolExtension_logMessage_defaultArguments_populatesFileIDAndLineis correctly implemented and verifies essential aspects of the logging functionality. No further changes are necessary at this point.Sources/AblyChat/Room.swift (2)
27-35: Good integration of logging in theDefaultRoominitializer.The addition of the
loggerparameter in the initializer and its subsequent use in initializing_statusis well-implemented. This change enhances the logging capabilities of theDefaultRoomactor, aligning with the PR objectives to improve visibility and error handling.
75-80: Effective error handling and logging in channel operations.The use of
do-catchblocks in theattachanddetachmethods to log errors and rethrow them is a robust approach to error handling. This ensures that errors are not only logged but also properly propagated, allowing higher-level handlers to manage them accordingly.Also applies to: 87-92
The aim of this commit is to:
The choice to have a separate internal logging interface is taken from Asset Tracking (AAT) and ably-cocoa, where I think it works well. I haven’t currently copied across AAT’s subsystem mechanism, a large part of the motivation for which was to provide a way to integrate the logs emitted by the AAT-instantiated Realtime client into those emitted by AAT itself; this is not currently a goal for Chat, which does not instantiate a Realtime client.
I briefly toyed with the idea of writing a macro that would log the inputs and outputs of public API methods, but figuring out how to do that didn’t seem like a good use of time now.
Resolves #38.
Summary by CodeRabbit
New Features
InternalLoggerprotocol for better logging context and control.Bug Fixes
Tests