[ECO-4998] feat: add logger abstraction #30
Conversation
WalkthroughThe changes in this pull request involve modifications to several files in the Android project. Key updates include enabling core library desugaring in the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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: 4
🧹 Outside diff range and nitpick comments (8)
chat-android/src/main/java/com/ably/chat/ChatClient.kt (3)
48-52: LGTM! Consider extracting logger creation to a separate method.The implementation of the
loggerproperty is well-structured and flexible, allowing for custom logging when provided or defaulting to an Android-specific logger. It correctly uses thelogLevelfromclientOptionsfor both cases.For improved readability and potential reusability, consider extracting the logger creation logic into a separate private method:
private fun createLogger(options: ClientOptions): Logger = if (options.logHandler != null) { CustomLogger(options.logHandler, options.logLevel) } else { AndroidLogger(options.logLevel) } private val logger: Logger = createLogger(clientOptions)This refactoring would make the
DefaultChatClientclass more modular and easier to maintain.
54-54: LGTM! Consider extracting the log context tag as a constant.The update to the
chatApiproperty correctly incorporates the new logger functionality. The use ofwithContextto provide a specific logging context is a good practice for enhanced debugging and tracing.For consistency and easier maintenance, consider extracting the log context tag as a private constant:
private const val CHAT_API_LOG_TAG = "AblyChatAPI" private val chatApi = ChatApi(realtime, clientId, logger.withContext(LogContext(tag = CHAT_API_LOG_TAG)))This change would make it easier to update the tag in the future if needed and keeps the code more organized.
48-54: Overall, the logger abstraction is well-implemented and enhances the chat client.The changes introduce a flexible logging mechanism to the
DefaultChatClientclass, allowing for custom logging handlers while providing a default Android logger. This implementation aligns well with the PR objectives and enhances the debugging and tracing capabilities of the chat client.The logger is effectively integrated into the
ChatApi, providing context-specific logging. These changes improve the overall maintainability and observability of the chat system without disrupting existing functionality.As the project evolves, consider creating a dedicated logging module or package to centralize logging-related code, including the logger creation logic and any common logging utilities. This would further improve the modularity and reusability of the logging functionality across the project.
chat-android/src/test/java/com/ably/chat/ChatApiTest.kt (1)
16-16: LGTM! Consider extracting EmptyLogger to a constant.The addition of the
loggerparameter withEmptyLoggeris a good practice for test cases. It aligns with the PR objective of adding a logger abstraction.To improve readability and maintainability, consider extracting
EmptyLoggerto a constant at the class level:private val testLogger = EmptyLogger private val chatApi = ChatApi(realtime, "clientId", logger = testLogger)This change would make it easier to update the logger for all test cases if needed in the future.
chat-android/src/main/java/com/ably/chat/ChatApi.kt (2)
141-151: LGTM! Consider adding method name to log context.The addition of detailed error logging in
makeAuthorizedRequestis excellent. It significantly improves error traceability and will be valuable for debugging.Consider adding the method name to the log context for even better traceability:
logger.error( "ChatApi.makeAuthorizedRequest(); failed to make request", context = LogContext( contextMap = mapOf( + "method" to method, "url" to url, "statusCode" to reason?.statusCode.toString(), "errorCode" to reason?.code.toString(), "errorMessage" to reason?.message.toString(), ), ), )
177-187: LGTM! Consider adding method name to log context.The addition of detailed error logging in
makeAuthorizedPaginatedRequestis consistent with the changes inmakeAuthorizedRequest. This consistency is excellent for maintainability and comprehensive error tracking.As suggested for
makeAuthorizedRequest, consider adding the method name to the log context:logger.error( "ChatApi.makeAuthorizedPaginatedRequest(); failed to make request", context = LogContext( contextMap = mapOf( + "method" to method, "url" to url, "statusCode" to reason?.statusCode.toString(), "errorCode" to reason?.code.toString(), "errorMessage" to reason?.message.toString(), ), ), )chat-android/src/test/java/com/ably/chat/MessagesTest.kt (1)
31-31: LGTM with suggestions for improvementThe addition of
EmptyLoggerto theChatApiconstructor is a good change that aligns with the new logging abstraction. UsingEmptyLoggerin tests is a good practice to prevent unnecessary logging during test execution.However, consider the following suggestions:
- It might be beneficial to add tests that verify the logging behavior with a mock logger in certain scenarios.
- Consider parameterizing some tests to run with both
EmptyLoggerand a mock logger to ensure consistent behavior.Would you like assistance in implementing these suggestions?
chat-android/src/main/java/com/ably/chat/Logger.kt (1)
62-62: Consider usingfinalContextwhen checking for context dataCurrently, the
contextStringis only appended ifthis.defaultContext.contextMapis not empty. This might exclude additional context provided during the log call. To ensure all context data is considered, checkfinalContext.contextMap.isEmpty()instead.You can modify the code as follows:
- val contextString = if (this.defaultContext.contextMap.isEmpty()) "" else ", context: $finalContext" + val contextString = if (finalContext.contextMap.isEmpty()) "" else ", context: $finalContext"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- chat-android/build.gradle.kts (2 hunks)
- chat-android/src/main/java/com/ably/chat/ChatApi.kt (3 hunks)
- chat-android/src/main/java/com/ably/chat/ChatClient.kt (1 hunks)
- chat-android/src/main/java/com/ably/chat/ClientOptions.kt (0 hunks)
- chat-android/src/main/java/com/ably/chat/Logger.kt (1 hunks)
- chat-android/src/test/java/com/ably/chat/ChatApiTest.kt (1 hunks)
- chat-android/src/test/java/com/ably/chat/MessagesTest.kt (1 hunks)
- example/build.gradle.kts (2 hunks)
- gradle/libs.versions.toml (2 hunks)
💤 Files with no reviewable changes (1)
- chat-android/src/main/java/com/ably/chat/ClientOptions.kt
🧰 Additional context used
🔇 Additional comments (10)
chat-android/build.gradle.kts (1)
30-33: Excellent update to modern Java and Android standards!The changes to enable core library desugaring and update Java/Kotlin versions to 17 are beneficial:
- Core library desugaring allows the use of newer Java APIs on older Android versions, improving backwards compatibility.
- Updating to Java 17 provides access to the latest language features and performance improvements.
- The consistent update in
kotlinOptionsensures Kotlin and Java are aligned.These changes follow best practices in modern Android development.
example/build.gradle.kts (2)
40-40: LGTM: Core library desugaring enabledEnabling core library desugaring is a good practice. It allows the use of newer Java APIs on older Android versions, which is beneficial for maintaining backwards compatibility while using modern Java features. This change aligns well with the PR objective of adding a logger abstraction, as it may involve using newer Java APIs.
71-71: LGTM: Desugaring dependency added correctlyThe addition of the desugaring dependency is correct and necessary to support the enabled core library desugaring. It's appropriately placed in the dependencies block and uses the version catalog for version management.
To ensure the version catalog is properly set up, please run the following script:
✅ Verification successful
Verification successful: Desugaring dependency setup is correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the presence and correct setup of the desugaring library in the version catalog. # Test: Check if the desugaring library is defined in the version catalog rg --type toml 'desugar\.jdk\.libs' gradle/libs.versions.toml # Test: Verify the dependency is correctly referenced in other build files rg --type kotlin 'libs\.desugar\.jdk\.libs' .Length of output: 268
gradle/libs.versions.toml (2)
7-7: LGTM: Addition of desugar_jdk_libs version referenceThe addition of the version reference for
desugar-jdk-libsis a good practice. This allows the use of Java 8+ APIs on older Android devices (API level < 24) through desugaring. The version "2.1.2" is relatively recent, ensuring access to the latest improvements and bug fixes.
7-7: Clarify project compatibility goals with desugar_jdk_libs additionThe addition of
desugar-jdk-libssuggests an intention to use Java 8+ APIs on older Android devices. This is a positive change for expanding API availability, but it's important to ensure it aligns with the project's compatibility goals.Could you please clarify:
- What is the current minimum supported API level for the project?
- Are there specific Java 8+ APIs that this change is intended to support?
- Has the impact on app size been considered, given that desugaring can increase the APK size?
To help assess the current state, please run the following script:
#!/bin/bash # Description: Check for minimum SDK version and desugar usage # Test: Find the minimum SDK version echo "Minimum SDK version:" rg --type gradle "minSdk\s*=\s*\d+" # Test: List Java 8+ API usages that might require desugaring echo "\nPotential Java 8+ API usages:" rg --type kotlin "java\.time\.|java\.util\.stream\.|java\.util\.function\."This will help us understand the current compatibility settings and potential uses of Java 8+ APIs in the project.
Also applies to: 25-25
chat-android/src/main/java/com/ably/chat/ChatApi.kt (2)
Line range hint
1-214: Overall, excellent improvements to error logging!The changes in this file significantly enhance the error logging capabilities of the
ChatApiclass. The consistent approach to logging across methods is commendable and will greatly improve debugging and error tracing. The core functionality of the class remains unchanged, which aligns with the PR objectives.Great job on implementing these improvements!
18-22: LGTM! Verify impact onChatApiinstantiation.The addition of the
loggerparameter to theChatApiconstructor is a good practice for enhancing logging capabilities. This change improves the class's ability to provide detailed error information.To ensure all
ChatApiinstantiations have been updated, run the following script:✅ Verification successful
All
ChatApiinstantiations include theloggerparameter.Verified that every instance of
ChatApihas been updated to include the newloggerparameter as intended. No issues found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all instantiations of ChatApi include the logger parameter. # Test: Search for ChatApi instantiations. Expect: All occurrences should have three parameters. rg --type kotlin "ChatApi\s*\([^)]*\)"Length of output: 474
chat-android/src/test/java/com/ably/chat/MessagesTest.kt (1)
31-31: Summary of review and next stepsThe addition of
EmptyLoggerto theChatApiconstructor is a positive change that introduces logging capabilities without disrupting existing tests. The implementation is sound and aligns with the PR objectives.Next steps to consider:
- Implement logging-specific tests as suggested earlier.
- Review the
ChatApiclass to ensure logging is used effectively and consistently.- Consider adding documentation about the logging abstraction and its usage in the
ChatApiclass.Overall, this change enhances the project's logging capabilities while maintaining existing functionality.
chat-android/src/main/java/com/ably/chat/Logger.kt (2)
63-63: Verify compatibility ofLocalDateTime.now()with target Android API levelsThe use of
java.time.LocalDateTime.now()may not be compatible with Android API levels below 26. Ensure that core library desugaring is enabled in your project to supportjava.timeAPIs on lower API levels.If desugaring is not enabled, consider using alternative methods such as
System.currentTimeMillis()orDatewithSimpleDateFormatfor broader compatibility.
58-58: Verify the log level comparison logic inlog()methodsThe condition
if (level.logLevelValue <= minimalVisibleLogLevel.logLevelValue) returnin bothAndroidLoggerandCustomLoggermight not correctly filter log messages based on their severity. Depending on howLogLevel.logLevelValueis defined (whether higher values represent higher or lower severity), the comparison operator may need adjustment to ensure that messages of appropriate levels are logged.Please run the following script to verify the ordering of
LogLevel.logLevelValueand confirm that the comparison logic functions as intended:Also applies to: 91-91
| @@ -28,7 +28,7 @@ class MessagesTest { | |||
| private val realtimeClient = mockk<RealtimeClient>(relaxed = true) | |||
| private val realtimeChannels = mockk<Channels>(relaxed = true) | |||
| private val realtimeChannel = spyk<Channel>(buildRealtimeChannel()) | |||
| private val chatApi = spyk(ChatApi(realtimeClient, "clientId")) | |||
| private val chatApi = spyk(ChatApi(realtimeClient, "clientId", EmptyLogger)) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding logging-specific tests
While the addition of EmptyLogger is good for existing tests, consider adding specific tests to verify logging behavior in critical scenarios. This could include:
- Verifying that certain operations log appropriate messages.
- Ensuring that sensitive information is not logged.
- Checking that log levels are used correctly.
These tests would help ensure that the logging abstraction is used correctly throughout the ChatApi class.
Would you like assistance in drafting some example logging-specific tests?
There was a problem hiding this comment.
@ttypic, sure! Here are some example logging-specific tests that you might consider adding to verify the logging behavior in critical scenarios.
- Test that operations log appropriate messages:
You can create a test that verifies whether certain methods are logging the expected messages. For instance, testing that error conditions are logged correctly.
import io.mockk.mockk
import io.mockk.verify
import io.mockk.every
import io.mockk.slot
import org.junit.Test
import io.mockk.junit5.MockKExtension
import org.junit.jupiter.api.extension.ExtendWith
@ExtendWith(MockKExtension::class)
class ChatApiLoggingTest {
private val mockLogHandler = mockk<LogHandler>(relaxed = true)
private val logger = CustomLogger(mockLogHandler)
private val realtimeClient = mockk<RealtimeClient>(relaxed = true)
private val chatApi = ChatApi(realtimeClient, "clientId", logger)
@Test
fun `should log error when makeAuthorizedRequest fails`() = runTest {
// Simulate an error response from the API
every { chatApi.makeAuthorizedRequest(any(), any(), any()) } throws AblyException.fromErrorInfo(ErrorInfo("Test error", 50000, 500))
try {
chatApi.makeAuthorizedRequest("GET", "/test-endpoint", params = emptyMap())
} catch (e: AblyException) {
// Expected exception
}
verify {
mockLogHandler.log(
LogLevel.ERROR,
any(),
match { it.contains("Error during request to /test-endpoint") },
any()
)
}
}
}- Test that sensitive information is not logged:
Ensure that logs do not contain any sensitive data like authentication tokens or personal information.
import io.mockk.mockk
import io.mockk.verify
import io.mockk.slot
import org.junit.Test
import io.mockk.junit5.MockKExtension
import org.junit.jupiter.api.extension.ExtendWith
@ExtendWith(MockKExtension::class)
class SensitiveDataLoggingTest {
private val mockLogHandler = mockk<LogHandler>(relaxed = true)
private val logger = CustomLogger(mockLogHandler)
private val realtimeClient = mockk<RealtimeClient>(relaxed = true)
private val chatApi = ChatApi(realtimeClient, "clientId", logger)
@Test
fun `should not log sensitive information`() = runTest {
val sensitiveData = "secretToken123"
chatApi.performOperationWithSensitiveData(sensitiveData)
verify(exactly = 0) {
mockLogHandler.log(
any(),
any(),
match { it.contains(sensitiveData) },
any()
)
}
}
}- Test that log levels are used correctly:
Verify that messages are logged at the appropriate log levels during different operations.
import io.mockk.mockk
import io.mockk.verify
import io.mockk.slot
import org.junit.Test
import io.mockk.junit5.MockKExtension
import org.junit.jupiter.api.extension.ExtendWith
@ExtendWith(MockKExtension::class)
class LogLevelTest {
private val mockLogHandler = mockk<LogHandler>(relaxed = true)
private val logger = CustomLogger(mockLogHandler)
private val realtimeClient = mockk<RealtimeClient>(relaxed = true)
private val chatApi = ChatApi(realtimeClient, "clientId", logger)
@Test
fun `should log info level messages during normal operations`() = runTest {
chatApi.performNormalOperation()
verify {
mockLogHandler.log(
LogLevel.INFO,
any(),
any(),
any()
)
}
}
}These tests use mockk to create a mock LogHandler that can intercept and verify log messages without outputting them during tests. Adjust the method names and logic to match your actual implementation.
Let me know if you need further assistance or any adjustments to these examples.
|
Hey, you might like to take a look at |
ttypic
force-pushed
the
ECO-4998/add-logger-abstraction
branch
from
ac24472
to
2de465e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
chat-android/src/main/java/com/ably/chat/ChatClient.kt (2)
48-52: LGTM! Consider using null-safe call for logHandler.The logger initialization looks good. It provides flexibility by allowing both custom and default logging options.
For improved null safety, consider using the safe call operator
?.when accessingclientOptions.logHandler:private val logger: Logger = clientOptions.logHandler?.let { CustomLogger(it, clientOptions.logLevel) } ?: AndroidLogger(clientOptions.logLevel)This change ensures that even if
clientOptionsitself is nullable, the code will still work correctly.
54-54: LGTM! Consider extracting the log tag as a constant.The update to the
chatApiproperty looks good. Including the logger in theChatApiconstructor enhances the logging capabilities.To improve code readability and maintainability, consider extracting the log tag as a private constant:
private const val CHAT_API_LOG_TAG = "AblyChatAPI" private val chatApi = ChatApi( realtime, clientId, logger.withContext(LogContext(tag = CHAT_API_LOG_TAG)) )This change makes it easier to update the tag in the future if needed and improves the overall code structure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- chat-android/build.gradle.kts (2 hunks)
- chat-android/src/main/java/com/ably/chat/ChatApi.kt (3 hunks)
- chat-android/src/main/java/com/ably/chat/ChatClient.kt (1 hunks)
- chat-android/src/main/java/com/ably/chat/ClientOptions.kt (0 hunks)
- chat-android/src/main/java/com/ably/chat/Logger.kt (1 hunks)
- chat-android/src/test/java/com/ably/chat/ChatApiTest.kt (1 hunks)
- chat-android/src/test/java/com/ably/chat/MessagesTest.kt (1 hunks)
- example/build.gradle.kts (2 hunks)
- gradle/libs.versions.toml (2 hunks)
💤 Files with no reviewable changes (1)
- chat-android/src/main/java/com/ably/chat/ClientOptions.kt
🚧 Files skipped from review as they are similar to previous changes (6)
- chat-android/src/main/java/com/ably/chat/ChatApi.kt
- chat-android/src/main/java/com/ably/chat/Logger.kt
- chat-android/src/test/java/com/ably/chat/ChatApiTest.kt
- chat-android/src/test/java/com/ably/chat/MessagesTest.kt
- example/build.gradle.kts
- gradle/libs.versions.toml
🧰 Additional context used
📓 Learnings (1)
chat-android/build.gradle.kts (1)
Learnt from: ttypic PR: ably-labs/ably-chat-kotlin#30 File: chat-android/build.gradle.kts:49-49 Timestamp: 2024-10-18T08:53:00.629Z Learning: Ensure to verify the dependencies in `chat-android/build.gradle.kts` before suggesting that a dependency has been removed, to avoid incorrect comments.
🔇 Additional comments (3)
chat-android/build.gradle.kts (2)
30-33: Excellent updates to build configuration!The changes to enable core library desugaring and update Java and Android SDK versions are beneficial:
- Enabling
isCoreLibraryDesugaringEnabledallows the use of newer Java APIs on older Android versions, improving compatibility.- Updating to
compileSdk = 34and Java 17 keeps the project aligned with the latest Android development standards.These updates will enhance the project's compatibility and maintainability.
49-49: Appropriate addition of desugaring libraryThe inclusion of
coreLibraryDesugaring(libs.desugar.jdk.libs)is correct and necessary to support the enabled desugaring option incompileOptions. This allows the project to use newer Java APIs on older Android versions, enhancing compatibility across different Android versions.chat-android/src/main/java/com/ably/chat/ChatClient.kt (1)
48-54: Summary: Logging enhancements improve error tracking and debugging capabilities.The introduction of the
loggerproperty and its integration withchatApisignificantly enhances the logging capabilities of theDefaultChatClient. This improvement will facilitate better error tracking and debugging in the chat application.Key points:
- Flexible logging configuration allows for both custom and default logging options.
- The logger is properly integrated with the
ChatApi, ensuring consistent logging throughout the chat operations.- The use of a specific log tag for the chat API context aids in log filtering and analysis.
These changes align well with the PR objective of adding a logger abstraction to the project.
| val tag = finalContext.tag ?: "AblyChatSDK" | ||
|
|
||
| val contextString = if (this.defaultContext.contextMap.isEmpty()) "" else ", context: $finalContext" | ||
| val formattedMessage = "[${LocalDateTime.now()}] ${level.name} ably-chat: ${message}$contextString" |
There was a problem hiding this comment.
Can we use formatting mention here -> #20 (comment).
Even in android, logs are sent to log analysers, so will be helpful for parsing if we use standard formatting for logging messages.
There was a problem hiding this comment.
|
@ttypic you can also have a read -> https://betterstack.com/community/guides/logging/logging-microservices/ |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (2)
chat-android/src/main/java/com/ably/chat/Utils.kt (1)
51-52: Add KDoc and consider internal visibility.Since this function is used for logging purposes, consider:
- Adding KDoc documentation explaining its purpose and usage
- Making it
internalif it's only used within the module+/** + * Generates a random UUID string for use as unique identifiers in logging. + * @return A randomly generated UUID as a string + */ -fun generateUUID() = UUID.randomUUID().toString() +internal fun generateUUID() = UUID.randomUUID().toString()chat-android/src/main/java/com/ably/chat/Logger.kt (1)
17-27: Add KDoc documentation to the Logger interface.Consider adding documentation to explain:
- The purpose and lifecycle of the Logger interface
- When to use
withContextvs passingnewTag/newContextMaptolog- The relationship between Logger and LogHandler
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- chat-android/src/main/java/com/ably/chat/ChatApi.kt (3 hunks)
- chat-android/src/main/java/com/ably/chat/ChatClient.kt (1 hunks)
- chat-android/src/main/java/com/ably/chat/Logger.kt (1 hunks)
- chat-android/src/main/java/com/ably/chat/Utils.kt (2 hunks)
- chat-android/src/test/java/com/ably/chat/ChatApiTest.kt (1 hunks)
- chat-android/src/test/java/com/ably/chat/MessagesTest.kt (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- chat-android/src/main/java/com/ably/chat/ChatApi.kt
- chat-android/src/test/java/com/ably/chat/ChatApiTest.kt
- chat-android/src/test/java/com/ably/chat/MessagesTest.kt
🧰 Additional context used
🔇 Additional comments (4)
chat-android/src/main/java/com/ably/chat/ChatClient.kt (1)
58-58: Extract tag constant and verify ChatApi logger integration.
- Consider extracting the "AblyChatAPI" tag to a constant for better maintainability
- Ensure that ChatApi properly handles the logger parameter for error handling
Let's verify the ChatApi implementation:
✅ Verification successful
Let me gather more information about the ChatApi constructor and logger initialization.
Based on the results, I can now provide the final response:
ChatApi properly handles logger, but tag constant recommendation stands
The verification shows that:
- ChatApi correctly accepts and stores the logger parameter in its constructor
- The logger is actively used for error logging in request handling
- No existing TAG constants were found in the codebase
While the logger integration is properly implemented, the recommendation to extract the "AblyChatAPI" tag to a constant still holds for better maintainability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify ChatApi constructor and error handling implementation # Test 1: Verify ChatApi constructor signature ast-grep --pattern 'class ChatApi { $$$ constructor($$$, logger: Logger) { $$$ } $$$ }' # Test 2: Check for error logging usage in ChatApi rg -A 3 'logger\.' chat-android/src/main/java/com/ably/chat/ChatApi.ktLength of output: 592
Script:
#!/bin/bash # Check ChatApi constructor and class definition rg -A 10 'class ChatApi' chat-android/src/main/java/com/ably/chat/ChatApi.kt # Check if there are any existing constants for logger tags rg 'val.*TAG.*=.*"' chat-android/src/main/java/com/ably/chat/Length of output: 396
chat-android/src/main/java/com/ably/chat/Logger.kt (3)
6-15: Well-designed logging interfaces!The combination of a functional interface for custom logging and an immutable context data class provides a clean and flexible foundation for the logging framework.
116-119: Clean no-op implementation!The EmptyLogger implementation follows the null object pattern correctly and provides a clean way to disable logging in tests.
3-5: Verify Android API compatibility for LocalDateTime usage.Using
java.time.LocalDateTimerequires either API level 26+ or core library desugaring. While the AI summary mentions that desugaring is enabled inbuild.gradle.kts, it's important to verify this configuration.✅ Verification successful
Let me try a different approach to verify the Android API compatibility configuration.
Usage of LocalDateTime is properly configured and safe
The codebase has the necessary configuration for using
java.time.LocalDateTime. Both the main library module (chat-android) and the example app have:
- Core library desugaring enabled:
isCoreLibraryDesugaringEnabled = true- The desugaring dependency:
coreLibraryDesugaring(libs.desugar.jdk.libs)- Minimum SDK set to 24, which is compatible with desugaring
The import of
java.time.LocalDateTimeis safe to use in this context.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if core library desugaring is properly configured rg -A 5 "desugar" build.gradle.ktsLength of output: 34
Script:
#!/bin/bash # Check build.gradle files recursively for desugaring configuration fd build.gradle --exec cat {} \; # Also check for kotlin-android configuration which might include compiler options fd build.gradle.kts --exec cat {} \;Length of output: 10571
| private val logger: Logger = if (clientOptions.logHandler != null) { | ||
| CustomLogger( | ||
| clientOptions.logHandler, | ||
| clientOptions.logLevel, | ||
| LogContext(tag = "ChatClient", instanceId = generateUUID(), clientId = clientId), | ||
| ) | ||
| } else { | ||
| AndroidLogger(clientOptions.logLevel, LogContext(tag = "ChatClient", instanceId = generateUUID(), clientId = clientId)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving logger initialization for better maintainability and log correlation.
A few suggestions to enhance the logger initialization:
- Consider using a stable identifier for
instanceIdinstead of a random UUID to maintain log correlation across client restarts - Extract the common LogContext creation logic to reduce duplication
- Add validation for the logHandler when provided
- Add KDoc documentation for the logger property
Here's a suggested refactoring:
+ /** Logger instance configured based on clientOptions */
+ private val logContext = LogContext(
+ tag = "ChatClient",
+ instanceId = clientId, // Using clientId as a stable identifier
+ clientId = clientId
+ )
+
private val logger: Logger = if (clientOptions.logHandler != null) {
+ require(clientOptions.logHandler.isInitialized()) { "LogHandler must be initialized" }
CustomLogger(
clientOptions.logHandler,
clientOptions.logLevel,
- LogContext(tag = "ChatClient", instanceId = generateUUID(), clientId = clientId),
+ logContext
)
} else {
- AndroidLogger(clientOptions.logLevel, LogContext(tag = "ChatClient", instanceId = generateUUID(), clientId = clientId))
+ AndroidLogger(clientOptions.logLevel, logContext)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private val logger: Logger = if (clientOptions.logHandler != null) { | |
| CustomLogger( | |
| clientOptions.logHandler, | |
| clientOptions.logLevel, | |
| LogContext(tag = "ChatClient", instanceId = generateUUID(), clientId = clientId), | |
| ) | |
| } else { | |
| AndroidLogger(clientOptions.logLevel, LogContext(tag = "ChatClient", instanceId = generateUUID(), clientId = clientId)) | |
| } | |
| /** Logger instance configured based on clientOptions */ | |
| private val logContext = LogContext( | |
| tag = "ChatClient", | |
| instanceId = clientId, // Using clientId as a stable identifier | |
| clientId = clientId | |
| ) | |
| private val logger: Logger = if (clientOptions.logHandler != null) { | |
| require(clientOptions.logHandler.isInitialized()) { "LogHandler must be initialized" } | |
| CustomLogger( | |
| clientOptions.logHandler, | |
| clientOptions.logLevel, | |
| logContext | |
| ) | |
| } else { | |
| AndroidLogger(clientOptions.logLevel, logContext) | |
| } |
| } | ||
|
|
||
| override fun log(message: String, level: LogLevel, throwable: Throwable?, newTag: String?, newContextMap: Map<String, String>) { | ||
| if (level.logLevelValue <= minimalVisibleLogLevel.logLevelValue) return |
There was a problem hiding this comment.
Fix inverted log level check in CustomLogger.
The same log level check issue exists here as in AndroidLogger.
- if (level.logLevelValue <= minimalVisibleLogLevel.logLevelValue) return
+ if (level.logLevelValue < minimalVisibleLogLevel.logLevelValue) return📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (level.logLevelValue <= minimalVisibleLogLevel.logLevelValue) return | |
| if (level.logLevelValue < minimalVisibleLogLevel.logLevelValue) return |
| LogLevel.Trace -> Log.i(tag, formattedMessage, throwable) | ||
| LogLevel.Debug -> Log.i(tag, formattedMessage, throwable) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider log level mapping.
Using Log.i for both Trace and Debug levels makes it harder to filter logs effectively. Consider using Log.v for Trace and Log.d for Debug.
- LogLevel.Trace -> Log.i(tag, formattedMessage, throwable)
- LogLevel.Debug -> Log.i(tag, formattedMessage, throwable)
+ LogLevel.Trace -> Log.v(tag, formattedMessage, throwable)
+ LogLevel.Debug -> Log.d(tag, formattedMessage, throwable)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| LogLevel.Trace -> Log.i(tag, formattedMessage, throwable) | |
| LogLevel.Debug -> Log.i(tag, formattedMessage, throwable) | |
| LogLevel.Trace -> Log.v(tag, formattedMessage, throwable) | |
| LogLevel.Debug -> Log.d(tag, formattedMessage, throwable) |
| internal fun LogContext.mergeWith(tag: String? = null, contextMap: Map<String, String> = mapOf()): LogContext { | ||
| return LogContext( | ||
| tag = tag ?: this.tag, | ||
| instanceId = instanceId, | ||
| clientId = clientId, | ||
| contextMap = this.contextMap + contextMap, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing context map merging for large maps.
The current implementation this.contextMap + contextMap creates a new map for every merge. For large context maps, consider using buildMap for better performance:
- contextMap = this.contextMap + contextMap,
+ contextMap = buildMap(this.contextMap.size + contextMap.size) {
+ putAll([email protected])
+ putAll(contextMap)
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| internal fun LogContext.mergeWith(tag: String? = null, contextMap: Map<String, String> = mapOf()): LogContext { | |
| return LogContext( | |
| tag = tag ?: this.tag, | |
| instanceId = instanceId, | |
| clientId = clientId, | |
| contextMap = this.contextMap + contextMap, | |
| ) | |
| } | |
| internal fun LogContext.mergeWith(tag: String? = null, contextMap: Map<String, String> = mapOf()): LogContext { | |
| return LogContext( | |
| tag = tag ?: this.tag, | |
| instanceId = instanceId, | |
| clientId = clientId, | |
| contextMap = buildMap(this.contextMap.size + contextMap.size) { | |
| putAll(this@mergeWith.contextMap) | |
| putAll(contextMap) | |
| }, | |
| ) | |
| } |
| } | ||
|
|
||
| override fun log(message: String, level: LogLevel, throwable: Throwable?, newTag: String?, newContextMap: Map<String, String>) { | ||
| if (level.logLevelValue <= minimalVisibleLogLevel.logLevelValue) return |
There was a problem hiding this comment.
Fix inverted log level check.
The current check skips logging when the level is less than or equal to the minimal level, which is the opposite of what's intended.
- if (level.logLevelValue <= minimalVisibleLogLevel.logLevelValue) return
+ if (level.logLevelValue < minimalVisibleLogLevel.logLevelValue) returnCommittable suggestion was skipped due to low confidence.
| val contextMap = finalContext.contextMap + ("instanceId" to finalContext.instanceId) | ||
|
|
||
| val contextString = ", context: $contextMap" | ||
| val formattedMessage = "[${LocalDateTime.now()}] ${level.name} ably-chat: ${message}$contextString" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize log message format.
As suggested in the referenced issue #20, consider adopting a standard log format that's easier to parse by log analyzers.
- val formattedMessage = "[${LocalDateTime.now()}] ${level.name} ably-chat: ${message}$contextString"
+ val formattedMessage = "${LocalDateTime.now()}|${level.name}|ably-chat|${message}|${contextMap.entries.joinToString("|") { "${it.key}=${it.value}" }}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val formattedMessage = "[${LocalDateTime.now()}] ${level.name} ably-chat: ${message}$contextString" | |
| val formattedMessage = "${LocalDateTime.now()}|${level.name}|ably-chat|${message}|${contextMap.entries.joinToString("|") { "${it.key}=${it.value}" }}" |
There was a problem hiding this comment.
Yeah, seems log entries separated by | might make more sense.
There was a problem hiding this comment.
@sacOO7, glad you agree! Let me know if there's anything else I can assist with.
(^•ω•^)
| data class LogContext( | ||
| val tag: String, | ||
| val clientId: String, | ||
| val instanceId: String, | ||
| val contextMap: Map<String, String> = mapOf(), | ||
| ) |
There was a problem hiding this comment.
| data class LogContext( | |
| val tag: String, | |
| val clientId: String, | |
| val instanceId: String, | |
| val contextMap: Map<String, String> = mapOf(), | |
| ) | |
| data class ChatClientContext( | |
| val tag: String, | |
| val chatClientInstanceId: String, | |
| val clientId: String = realtimeClient.auth.clientId, | |
| val connectionId: String? = realtimeClient.connection.Id, // null means connection is not active, retrying or failed depending on connectionState. | |
| val connectionState: ConnectionState = realtimeClient.connection.state, // denotes connection state of shared realtimeClient | |
| val contextMap: Map<String, String> = mapOf(), | |
| ) |
There was a problem hiding this comment.
We should have data class for RoomContext as well, which will be derived from ChatClientContext
val chatClientLogger = Logger(ChatClientContext());
val roomLogger = chatClientLogger.derivedFrom(RoomContext(roomId, roomStatus, supportedRoomFeatures))
data class RoomContext {
val roomId: String,
val roomStatus: RoomStatus,
val roomFeatures: listOf<String>(), // Supported feature names for a given room
val contextMap: Map<String, String> = mapOf(),
}
Underlying room feature/contributor will also have one more data class RoomFeatureContext.
This will be used for roomFeatureLogger that will created inside contributor/room feature instance.
val roomFeatureLogger = roomLogger.derivedFrom(RoomFeatureContext(roomId, channel.Name, channel.Status))
data class RoomFeatureContext {
val featureName: String,
val channelName: String,
val channelState: AblyRealtimeChannel.ChannelState,
val contextMap: Map<String, String> = mapOf(),
}
|
Not sure where this needs review since I can't see new commits on the PR : ( |
ttypic
force-pushed
the
ECO-4998/add-logger-abstraction
branch
from
33bfe05
to
2359633
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
chat-android/src/main/java/com/ably/chat/Logger.kt (3)
6-8: Add KDoc documentation for the public LogHandler interface.Since this is a public API, comprehensive documentation would help users understand its purpose and usage.
Add documentation explaining the purpose, parameters, and usage example:
+/** + * Functional interface for custom log handling implementations. + * + * @param message The log message to be processed + * @param level The severity level of the log + * @param throwable Optional exception associated with the log + * @param context Additional contextual information for the log + */ fun interface LogHandler { fun log(message: String, level: LogLevel, throwable: Throwable?, context: LogContext) }
138-141: Add documentation for EmptyLogger.Document the purpose and use cases of the EmptyLogger implementation.
+/** + * A no-op logger implementation that discards all log messages. + * Useful for testing scenarios or when logging needs to be disabled. + */ internal class EmptyLogger(override val context: LogContext) : Logger {
1-141: Consider implementing ChatClientContext as suggested.As discussed in previous comments, consider implementing specific context classes (ChatClientContext, RoomContext, RoomFeatureContext) to provide more structured logging contexts for different components of the system.
This would provide:
- Better type safety for context information
- Clear separation of concerns
- More maintainable and structured logging across the SDK
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
chat-android/build.gradle.kts(2 hunks)chat-android/src/main/java/com/ably/chat/ChatApi.kt(3 hunks)chat-android/src/main/java/com/ably/chat/ChatClient.kt(3 hunks)chat-android/src/main/java/com/ably/chat/ClientOptions.kt(0 hunks)chat-android/src/main/java/com/ably/chat/Logger.kt(1 hunks)chat-android/src/main/java/com/ably/chat/Utils.kt(2 hunks)chat-android/src/test/java/com/ably/chat/ChatApiTest.kt(1 hunks)chat-android/src/test/java/com/ably/chat/MessagesTest.kt(1 hunks)example/build.gradle.kts(2 hunks)gradle/libs.versions.toml(2 hunks)
💤 Files with no reviewable changes (1)
- chat-android/src/main/java/com/ably/chat/ClientOptions.kt
🚧 Files skipped from review as they are similar to previous changes (8)
- chat-android/build.gradle.kts
- chat-android/src/main/java/com/ably/chat/ChatApi.kt
- chat-android/src/main/java/com/ably/chat/ChatClient.kt
- chat-android/src/main/java/com/ably/chat/Utils.kt
- chat-android/src/test/java/com/ably/chat/ChatApiTest.kt
- chat-android/src/test/java/com/ably/chat/MessagesTest.kt
- example/build.gradle.kts
- gradle/libs.versions.toml
| data class LogContext( | ||
| val tag: String, | ||
| val staticContext: Map<String, String> = mapOf(), | ||
| val dynamicContext: Map<String, () -> String> = mapOf(), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for empty tag in LogContext.
The tag parameter should not be empty as it's used for log identification.
Add validation in the constructor:
data class LogContext(
- val tag: String,
+ val tag: String,
val staticContext: Map<String, String> = mapOf(),
val dynamicContext: Map<String, () -> String> = mapOf(),
-)
+) {
+ init {
+ require(tag.isNotBlank()) { "Tag must not be blank" }
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| data class LogContext( | |
| val tag: String, | |
| val staticContext: Map<String, String> = mapOf(), | |
| val dynamicContext: Map<String, () -> String> = mapOf(), | |
| ) | |
| data class LogContext( | |
| val tag: String, | |
| val staticContext: Map<String, String> = mapOf(), | |
| val dynamicContext: Map<String, () -> String> = mapOf(), | |
| ) { | |
| init { | |
| require(tag.isNotBlank()) { "Tag must not be blank" } | |
| } | |
| } |
| val contextString = ", context: $completeContext" | ||
| val formattedMessage = "[${LocalDateTime.now()}] $tag ${level.name} ably-chat: ${message}$contextString" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement standardized log format as discussed.
As suggested in issue #20 and previous comments, implement a pipe-separated format for better log parsing.
- val contextString = ", context: $completeContext"
- val formattedMessage = "[${LocalDateTime.now()}] $tag ${level.name} ably-chat: ${message}$contextString"
+ val contextString = completeContext.entries.joinToString("|") { "${it.key}=${it.value}" }
+ val formattedMessage = "${LocalDateTime.now()}|${level.name}|$tag|ably-chat|$message|$contextString"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val contextString = ", context: $completeContext" | |
| val formattedMessage = "[${LocalDateTime.now()}] $tag ${level.name} ably-chat: ${message}$contextString" | |
| val contextString = completeContext.entries.joinToString("|") { "${it.key}=${it.value}" } | |
| val formattedMessage = "${LocalDateTime.now()}|${level.name}|$tag|ably-chat|$message|$contextString" |
| data class LogContext( | ||
| val tag: String, | ||
| val staticContext: Map<String, String> = mapOf(), | ||
| val dynamicContext: Map<String, () -> String> = mapOf(), |
There was a problem hiding this comment.
Nice one, I was thinking something similar 👍
| } | ||
| } | ||
|
|
||
| internal class EmptyLogger(override val context: LogContext) : Logger { |
There was a problem hiding this comment.
If the use-case of EmptyLogger is limited to tests, then it can be moved to TestUtils.kt and renamed to TestLogger or EmptyTestLogger wdyt
| ) | ||
| } | ||
|
|
||
| internal class AndroidLogger( |
There was a problem hiding this comment.
| internal class AndroidLogger( | |
| internal class DefaultAndroidLogger( |
wdyt
Added logger abstraction
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores