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

Merged
merged 1 commit into from
Nov 18, 2024
Merged
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
2 changes: 2 additions & 0 deletions 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 @@ -46,6 +47,7 @@ dependencies {
api(libs.ably.android)
implementation(libs.gson)
implementation(libs.coroutine.core)
coreLibraryDesugaring(libs.desugar.jdk.libs)
ttypic marked this conversation as resolved.
Show resolved Hide resolved

testImplementation(libs.junit)
testImplementation(libs.mockk)
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(
sacOO7 marked this conversation as resolved.
Show resolved Hide resolved
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",
staticContext = 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",
staticContext = 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
26 changes: 24 additions & 2 deletions chat-android/src/main/java/com/ably/chat/ChatClient.kt
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
@file:Suppress("StringLiteralDuplication", "NotImplementedDeclaration")
@file:Suppress("NotImplementedDeclaration")

package com.ably.chat

Expand Down 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,
buildLogContext(),
)
} else {
AndroidLogger(clientOptions.logLevel, buildLogContext())
}

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

override val rooms: Rooms = DefaultRooms(
realtimeClient = realtime,
Expand All @@ -58,4 +68,16 @@ internal class DefaultChatClient(

override val clientId: String
get() = realtime.auth.clientId

private fun buildLogContext() = LogContext(
tag = "ChatClient",
staticContext = mapOf(
"clientId" to clientId,
"instanceId" to generateUUID(),
),
dynamicContext = mapOf(
"connectionId" to { realtime.connection.id },
"connectionState" to { realtime.connection.state.name },
),
)
}
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
136 changes: 136 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,136 @@
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 staticContext: Map<String, String> = mapOf(),
val dynamicContext: Map<String, () -> String> = mapOf(),
Copy link

Choose a reason for hiding this comment

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

Nice one, I was thinking something similar 👍

)
Comment on lines +10 to +14
Copy link

Choose a reason for hiding this comment

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

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

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


internal interface Logger {
val context: LogContext
fun withContext(
tag: String? = null,
staticContext: Map<String, String> = mapOf(),
dynamicContext: Map<String, () -> String> = mapOf(),
): Logger

fun log(
message: String,
level: LogLevel,
throwable: Throwable? = null,
newTag: String? = null,
newStaticContext: Map<String, String> = mapOf(),
)
}

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

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

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

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

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

internal fun LogContext.mergeWith(
tag: String? = null,
staticContext: Map<String, String> = mapOf(),
dynamicContext: Map<String, () -> String> = mapOf(),
): LogContext {
return LogContext(
tag = tag ?: this.tag,
staticContext = this.staticContext + staticContext,
dynamicContext = this.dynamicContext + dynamicContext,
)
}

internal class AndroidLogger(
Copy link

Choose a reason for hiding this comment

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

Suggested change
internal class AndroidLogger(
internal class DefaultAndroidLogger(

wdyt

private val minimalVisibleLogLevel: LogLevel,
override val context: LogContext,
) : Logger {

override fun withContext(tag: String?, staticContext: Map<String, String>, dynamicContext: Map<String, () -> String>): Logger {
return AndroidLogger(
minimalVisibleLogLevel = minimalVisibleLogLevel,
context = context.mergeWith(tag, staticContext, dynamicContext),
)
}

override fun log(message: String, level: LogLevel, throwable: Throwable?, newTag: String?, newStaticContext: 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, newStaticContext)
val tag = finalContext.tag
val completeContext = finalContext.staticContext + finalContext.dynamicContext.mapValues { it.value() }

val contextString = ", context: $completeContext"
val formattedMessage = "[${LocalDateTime.now()}] $tag ${level.name} ably-chat: ${message}$contextString"
Comment on lines +98 to +99
Copy link

Choose a reason for hiding this comment

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

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

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

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 +102 to +103
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?, staticContext: Map<String, String>, dynamicContext: Map<String, () -> String>): Logger {
return CustomLogger(
logHandler = logHandler,
minimalVisibleLogLevel = minimalVisibleLogLevel,
context = context.mergeWith(tag, staticContext, dynamicContext),
)
}

override fun log(message: String, level: LogLevel, throwable: Throwable?, newTag: String?, newStaticContext: 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, newStaticContext)
logHandler.log(
message = message,
level = level,
throwable = throwable,
context = finalContext,
)
}
}
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 @@ -7,6 +7,7 @@ import io.ably.lib.types.AblyException
import io.ably.lib.types.ChannelOptions
import io.ably.lib.types.ErrorInfo
import io.ably.lib.types.Param
import java.util.UUID
import kotlin.coroutines.resume
import kotlin.coroutines.resumeWithException
import kotlin.coroutines.suspendCoroutine
Expand Down Expand Up @@ -125,6 +126,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")))

/**
* @nospec
Expand Down
2 changes: 1 addition & 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,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(LogContext(tag = "TEST"))))
private lateinit var messages: DefaultMessages

private val channelStateListenerSlot = slot<ChannelStateListener>()
Expand Down
5 changes: 5 additions & 0 deletions chat-android/src/test/java/com/ably/chat/TestUtils.kt
Original file line number Diff line number Diff line change
Expand Up @@ -49,3 +49,8 @@ fun mockOccupancyApiResponse(realtimeClientMock: RealtimeClient, response: JsonE
)
}
}

internal class EmptyLogger(override val context: LogContext) : Logger {
override fun withContext(tag: String?, staticContext: Map<String, String>, dynamicContext: Map<String, () -> String>): Logger = this
override fun log(message: String, level: LogLevel, throwable: Throwable?, newTag: String?, newStaticContext: Map<String, String>) = Unit
}
3 changes: 2 additions & 1 deletion 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,7 +68,7 @@ dependencies {
implementation(libs.androidx.ui.graphics)
implementation(libs.androidx.ui.tooling.preview)
implementation(libs.androidx.material3)
implementation(libs.konfetti.compose)
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 @@ -23,6 +24,7 @@ build-config = "5.4.0"
ktor = "3.0.1"

[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
Loading