Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ECO-4998] feat: add logger abstraction #30

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion chat-android/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ android {
}

compileOptions {
isCoreLibraryDesugaringEnabled = true
sourceCompatibility = JavaVersion.VERSION_17
targetCompatibility = JavaVersion.VERSION_17
}
Expand All @@ -45,7 +46,7 @@ buildConfig {
dependencies {
api(libs.ably.android)
implementation(libs.gson)

coreLibraryDesugaring(libs.desugar.jdk.libs)
ttypic marked this conversation as resolved.
Show resolved Hide resolved
testImplementation(libs.junit)
testImplementation(libs.mockk)
testImplementation(libs.coroutine.test)
Expand Down
24 changes: 23 additions & 1 deletion chat-android/src/main/java/com/ably/chat/ChatApi.kt
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,11 @@ private const val PROTOCOL_VERSION_PARAM_NAME = "v"
private const val RESERVED_ABLY_CHAT_KEY = "ably-chat"
private val apiProtocolParam = Param(PROTOCOL_VERSION_PARAM_NAME, API_PROTOCOL_VERSION.toString())

internal class ChatApi(private val realtimeClient: RealtimeClient, private val clientId: String) {
internal class ChatApi(
Copy link

@sacOO7 sacOO7 Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have LoggerContext for ChatApi as well

data class ChatApi(
    val tag: String,
    val chatApiInstanceId: String,
    val clientId: String,
    val contextMap: Map<String, String> = mapOf(), // contextMap will be set by `ChatApi` initializer class.
)

private val realtimeClient: RealtimeClient,
private val clientId: String,
private val logger: Logger,
) {

/**
* Get messages from the Chat Backend
Expand Down Expand Up @@ -134,6 +138,15 @@ internal class ChatApi(private val realtimeClient: RealtimeClient, private val c
}

override fun onError(reason: ErrorInfo?) {
logger.error(
"ChatApi.makeAuthorizedRequest(); failed to make request",
contextMap = mapOf(
"url" to url,
"statusCode" to reason?.statusCode.toString(),
"errorCode" to reason?.code.toString(),
"errorMessage" to reason?.message.toString(),
),
)
// (CHA-M3e)
continuation.resumeWithException(AblyException.fromErrorInfo(reason))
}
Expand All @@ -159,6 +172,15 @@ internal class ChatApi(private val realtimeClient: RealtimeClient, private val c
}

override fun onError(reason: ErrorInfo?) {
logger.error(
"ChatApi.makeAuthorizedPaginatedRequest(); failed to make request",
contextMap = mapOf(
"url" to url,
"statusCode" to reason?.statusCode.toString(),
"errorCode" to reason?.code.toString(),
"errorMessage" to reason?.message.toString(),
),
)
continuation.resumeWithException(AblyException.fromErrorInfo(reason))
}
},
Expand Down
12 changes: 11 additions & 1 deletion chat-android/src/main/java/com/ably/chat/ChatClient.kt
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,17 @@ internal class DefaultChatClient(
override val clientOptions: ClientOptions,
) : ChatClient {

private val chatApi = ChatApi(realtime, clientId)
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))
}
Comment on lines +48 to +56
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider improving logger initialization for better maintainability and log correlation.

A few suggestions to enhance the logger initialization:

  1. Consider using a stable identifier for instanceId instead of a random UUID to maintain log correlation across client restarts
  2. Extract the common LogContext creation logic to reduce duplication
  3. Add validation for the logHandler when provided
  4. 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.

Suggested change
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)
}


private val chatApi = ChatApi(realtime, clientId, logger.withContext(tag = "AblyChatAPI"))

override val rooms: Rooms = DefaultRooms(
realtimeClient = realtime,
Expand Down
2 changes: 0 additions & 2 deletions chat-android/src/main/java/com/ably/chat/ClientOptions.kt
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
package com.ably.chat

import io.ably.lib.util.Log.LogHandler

/**
* Configuration options for the chat client.
*/
Expand Down
119 changes: 119 additions & 0 deletions chat-android/src/main/java/com/ably/chat/Logger.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
package com.ably.chat

import android.util.Log
import java.time.LocalDateTime

fun interface LogHandler {
fun log(message: String, level: LogLevel, throwable: Throwable?, context: LogContext)
}

data class LogContext(
val tag: String,
val clientId: String,
val instanceId: String,
val contextMap: Map<String, String> = mapOf(),
)
Comment on lines +10 to +15
Copy link

@sacOO7 sacOO7 Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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(),
)

Copy link

@sacOO7 sacOO7 Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(),
}


internal interface Logger {
val context: LogContext
fun withContext(tag: String? = null, contextMap: Map<String, String> = mapOf()): Logger
fun log(
message: String,
level: LogLevel,
throwable: Throwable? = null,
newTag: String? = null,
newContextMap: Map<String, String> = mapOf(),
)
}

internal fun Logger.trace(message: String, throwable: Throwable? = null, tag: String? = null, contextMap: Map<String, String> = mapOf()) {
log(message, LogLevel.Trace, throwable, tag, contextMap)
}

internal fun Logger.debug(message: String, throwable: Throwable? = null, tag: String? = null, contextMap: Map<String, String> = mapOf()) {
log(message, LogLevel.Debug, throwable, tag, contextMap)
}

internal fun Logger.info(message: String, throwable: Throwable? = null, tag: String? = null, contextMap: Map<String, String> = mapOf()) {
log(message, LogLevel.Info, throwable, tag, contextMap)
}

internal fun Logger.warn(message: String, throwable: Throwable? = null, tag: String? = null, contextMap: Map<String, String> = mapOf()) {
log(message, LogLevel.Warn, throwable, tag, contextMap)
}

internal fun Logger.error(message: String, throwable: Throwable? = null, tag: String? = null, contextMap: Map<String, String> = mapOf()) {
log(message, LogLevel.Error, throwable, tag, 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 = this.contextMap + contextMap,
)
}
Comment on lines +49 to +56
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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(this@mergeWith.contextMap)
+            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.

Suggested change
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)
},
)
}


internal class AndroidLogger(
private val minimalVisibleLogLevel: LogLevel,
override val context: LogContext,
) : Logger {

override fun withContext(newTag: String?, newContextMap: Map<String, String>): Logger {
return AndroidLogger(
minimalVisibleLogLevel = minimalVisibleLogLevel,
context = context.mergeWith(newTag, newContextMap),
)
}

override fun log(message: String, level: LogLevel, throwable: Throwable?, newTag: String?, newContextMap: Map<String, String>) {
if (level.logLevelValue <= minimalVisibleLogLevel.logLevelValue) return
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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) return

Committable suggestion was skipped due to low confidence.

val finalContext = context.mergeWith(newTag, newContextMap)
val tag = finalContext.tag
val contextMap = finalContext.contextMap + ("instanceId" to finalContext.instanceId)

val contextString = ", context: $contextMap"
val formattedMessage = "[${LocalDateTime.now()}] ${level.name} ably-chat: ${message}$contextString"
Copy link

@sacOO7 sacOO7 Oct 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

@coderabbitai coderabbitai bot Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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}" }}"

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, seems log entries separated by | might make more sense.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sacOO7, glad you agree! Let me know if there's anything else I can assist with.

(^•ω•^)

when (level) {
// We use Logcat's info level for Trace and Debug
LogLevel.Trace -> Log.i(tag, formattedMessage, throwable)
LogLevel.Debug -> Log.i(tag, formattedMessage, throwable)
Comment on lines +80 to +81
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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)

LogLevel.Info -> Log.i(tag, formattedMessage, throwable)
LogLevel.Warn -> Log.w(tag, formattedMessage, throwable)
LogLevel.Error -> Log.e(tag, formattedMessage, throwable)
LogLevel.Silent -> {}
}
}
}

internal class CustomLogger(
private val logHandler: LogHandler,
private val minimalVisibleLogLevel: LogLevel,
override val context: LogContext,
) : Logger {

override fun withContext(tag: String?, contextMap: Map<String, String>): Logger {
return CustomLogger(
logHandler = logHandler,
minimalVisibleLogLevel = minimalVisibleLogLevel,
context = context.mergeWith(tag, contextMap),
)
}

override fun log(message: String, level: LogLevel, throwable: Throwable?, newTag: String?, newContextMap: Map<String, String>) {
if (level.logLevelValue <= minimalVisibleLogLevel.logLevelValue) return
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
if (level.logLevelValue <= minimalVisibleLogLevel.logLevelValue) return
if (level.logLevelValue < minimalVisibleLogLevel.logLevelValue) return

val finalContext = context.mergeWith(newTag, newContextMap)
logHandler.log(
message = message,
level = level,
throwable = throwable,
context = finalContext,
)
}
}

internal class EmptyLogger(override val context: LogContext) : Logger {
override fun withContext(newTag: String?, newContextMap: Map<String, String>): Logger = this
override fun log(message: String, level: LogLevel, throwable: Throwable?, newTag: String?, newContextMap: Map<String, String>) = Unit
}
3 changes: 3 additions & 0 deletions chat-android/src/main/java/com/ably/chat/Utils.kt
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import io.ably.lib.realtime.CompletionListener
import io.ably.lib.types.AblyException
import io.ably.lib.types.ChannelOptions
import io.ably.lib.types.ErrorInfo
import java.util.UUID
import kotlin.coroutines.resume
import kotlin.coroutines.resumeWithException
import kotlin.coroutines.suspendCoroutine
Expand Down Expand Up @@ -47,6 +48,8 @@ fun ChatChannelOptions(init: (ChannelOptions.() -> Unit)? = null): ChannelOption
return options
}

fun generateUUID() = UUID.randomUUID().toString()

/**
* A value that can be evaluated at a later time, similar to `kotlinx.coroutines.Deferred` or a JavaScript Promise.
*
Expand Down
3 changes: 2 additions & 1 deletion chat-android/src/test/java/com/ably/chat/ChatApiTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ import org.junit.Test
class ChatApiTest {

private val realtime = mockk<RealtimeClient>(relaxed = true)
private val chatApi = ChatApi(realtime, "clientId")
private val chatApi =
ChatApi(realtime, "clientId", logger = EmptyLogger(LogContext(tag = "TEST", instanceId = generateUUID(), clientId = "clientId")))

/**
* @nospec
Expand Down
3 changes: 2 additions & 1 deletion chat-android/src/test/java/com/ably/chat/MessagesTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ 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(LogContext(tag = "TEST", instanceId = generateUUID(), clientId = "clientId"))))
private lateinit var messages: DefaultMessages

private val channelStateListenerSlot = slot<ChannelStateListener>()
Expand Down
2 changes: 2 additions & 0 deletions example/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ android {
}
}
compileOptions {
isCoreLibraryDesugaringEnabled = true
sourceCompatibility = JavaVersion.VERSION_1_8
targetCompatibility = JavaVersion.VERSION_1_8
}
Expand Down Expand Up @@ -67,6 +68,7 @@ dependencies {
implementation(libs.androidx.ui.graphics)
implementation(libs.androidx.ui.tooling.preview)
implementation(libs.androidx.material3)
coreLibraryDesugaring(libs.desugar.jdk.libs)
testImplementation(libs.junit)
androidTestImplementation(libs.androidx.junit)
androidTestImplementation(libs.androidx.espresso.core)
Expand Down
2 changes: 2 additions & 0 deletions gradle/libs.versions.toml
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
[versions]
ably-chat = "0.0.1"
ably = "1.2.43"
desugar-jdk-libs = "2.1.2"
junit = "4.13.2"
agp = "8.5.2"
detekt = "1.23.6"
Expand All @@ -21,6 +22,7 @@ coroutine = "1.8.1"
build-config = "5.4.0"

[libraries]
desugar-jdk-libs = { module = "com.android.tools:desugar_jdk_libs", version.ref = "desugar-jdk-libs" }
ttypic marked this conversation as resolved.
Show resolved Hide resolved
junit = { group = "junit", name = "junit", version.ref = "junit" }
detekt-formatting = { module = "io.gitlab.arturbosch.detekt:detekt-formatting", version.ref = "detekt" }
ably-android = { module = "io.ably:ably-android", version.ref = "ably" }
Expand Down