From 3cc5f3e31cc6e1115fb49b7c9feac16d34d0e4af Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 10 Jan 2025 15:19:09 +0100 Subject: [PATCH 1/3] feat: use android.util.Log for logging This way, the SDK has a control over which logs will be used in production or debug variants of SDK users. As on outcome, this change reduces number of logs that the SDK is producing in production apps. --- .../parselyandroid/AdvertisementIdProvider.kt | 5 ++--- .../parselyandroid/DeviceInfoRepository.kt | 7 +++---- .../parselyandroid/EngagementManager.kt | 3 +-- .../parsely/parselyandroid/EventsBuilder.kt | 4 +--- .../com/parsely/parselyandroid/FlushQueue.kt | 16 +++++++--------- .../parsely/parselyandroid/InMemoryBuffer.kt | 5 ++--- .../parselyandroid/LocalStorageRepository.kt | 8 ++------ .../java/com/parsely/parselyandroid/Log.kt | 18 ++++++++++++++++++ .../java/com/parsely/parselyandroid/Logging.kt | 17 ----------------- .../parselyandroid/ParselyTrackerInternal.kt | 12 ++++++------ 10 files changed, 42 insertions(+), 53 deletions(-) create mode 100644 parsely/src/main/java/com/parsely/parselyandroid/Log.kt delete mode 100644 parsely/src/main/java/com/parsely/parselyandroid/Logging.kt diff --git a/parsely/src/main/java/com/parsely/parselyandroid/AdvertisementIdProvider.kt b/parsely/src/main/java/com/parsely/parselyandroid/AdvertisementIdProvider.kt index 87e93a84..03bfd8d5 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/AdvertisementIdProvider.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/AdvertisementIdProvider.kt @@ -3,7 +3,6 @@ package com.parsely.parselyandroid import android.content.Context import android.provider.Settings import com.google.android.gms.ads.identifier.AdvertisingIdClient -import com.parsely.parselyandroid.Logging.log import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.launch @@ -19,7 +18,7 @@ internal class AdvertisementIdProvider( try { adKey = AdvertisingIdClient.getAdvertisingIdInfo(context).id } catch (e: Exception) { - log("No Google play services or error!") + Log.e("No Google play services or error!", e) } } } @@ -41,7 +40,7 @@ internal class AndroidIdProvider(private val context: Context) : IdProvider { } catch (ex: Exception) { null } - log(String.format("Android ID: %s", uuid)) + Log.d("Android ID: $uuid") return uuid } } diff --git a/parsely/src/main/java/com/parsely/parselyandroid/DeviceInfoRepository.kt b/parsely/src/main/java/com/parsely/parselyandroid/DeviceInfoRepository.kt index 61abdc4f..b5980943 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/DeviceInfoRepository.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/DeviceInfoRepository.kt @@ -1,7 +1,6 @@ package com.parsely.parselyandroid import android.os.Build -import com.parsely.parselyandroid.Logging.log internal interface DeviceInfoRepository{ fun collectDeviceInfo(): Map @@ -25,7 +24,7 @@ internal open class AndroidDeviceInfoRepository( dInfo["parsely_site_uuid"] = parselySiteUuid dInfo["manufacturer"] = Build.MANUFACTURER dInfo["os"] = "android" - dInfo["os_version"] = String.format("%d", Build.VERSION.SDK_INT) + dInfo["os_version"] = Build.VERSION.SDK_INT.toString() return dInfo } @@ -35,12 +34,12 @@ internal open class AndroidDeviceInfoRepository( val adKey = advertisementIdProvider.provide() val androidId = androidIdProvider.provide() - log("adkey is: %s, uuid is %s", adKey, androidId) + Log.d("adkey is: $adKey, uuid is $androidId") return if (adKey != null) { adKey } else { - log("falling back to device uuid") + Log.d("falling back to device uuid") androidId .orEmpty() } } diff --git a/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt b/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt index 4bb39ac3..55651e78 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt @@ -1,6 +1,5 @@ package com.parsely.parselyandroid -import com.parsely.parselyandroid.Logging.log import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Job import kotlinx.coroutines.delay @@ -62,7 +61,7 @@ internal class EngagementManager( val event: MutableMap = HashMap( baseEvent ) - log(String.format("Enqueuing %s event.", event["action"])) + Log.d("Enqueuing ${event["action"]} event.") // Update `ts` for the event since it's happening right now. val baseEventData = (event["data"] as Map?)!! diff --git a/parsely/src/main/java/com/parsely/parselyandroid/EventsBuilder.kt b/parsely/src/main/java/com/parsely/parselyandroid/EventsBuilder.kt index e3783056..77a6a078 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/EventsBuilder.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/EventsBuilder.kt @@ -1,7 +1,5 @@ package com.parsely.parselyandroid -import com.parsely.parselyandroid.Logging.log - internal class EventsBuilder( private val deviceInfoRepository: DeviceInfoRepository, private val initializationSiteId: String, @@ -27,7 +25,7 @@ internal class EventsBuilder( uuid: String, siteIdSource: SiteIdSource, ): Map { - log("buildEvent called for %s/%s", action, url) + Log.d("buildEvent called for $action/$url") // Main event info val event: MutableMap = HashMap() diff --git a/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt b/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt index 561e7c19..c3413827 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt @@ -1,7 +1,6 @@ package com.parsely.parselyandroid import com.parsely.parselyandroid.JsonSerializer.toParselyEventsPayload -import com.parsely.parselyandroid.Logging.log import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.launch import kotlinx.coroutines.sync.Mutex @@ -19,7 +18,7 @@ internal class FlushQueue( operator fun invoke(skipSendingEvents: Boolean) { if (!connectivityStatusProvider.isReachable()) { - log("Network unreachable. Not flushing.") + Log.d("Network unreachable. Not flushing.") return } scope.launch { @@ -33,22 +32,21 @@ internal class FlushQueue( val jsonPayload = toParselyEventsPayload(eventsToSend) if (skipSendingEvents) { - log("Debug mode on. Not sending to Parse.ly. Otherwise, would sent ${eventsToSend.size} events: $jsonPayload") + Log.d("Debug mode on. Not sending to Parse.ly. Otherwise, would sent ${eventsToSend.size} events: $jsonPayload") repository.remove(eventsToSend) return@launch } - log("Sending request with %d events", eventsToSend.size) - log("POST Data %s", jsonPayload) - log("Requested %s", ParselyTrackerInternal.ROOT_URL) + Log.d("Sending request with ${eventsToSend.size} events") + Log.d("POST Data $jsonPayload") + Log.d("Requested ${ParselyTrackerInternal.ROOT_URL}") restClient.send(jsonPayload) .fold( onSuccess = { - log("Pixel request success") + Log.i("Pixel request success") repository.remove(eventsToSend) }, onFailure = { - log("Pixel request exception") - log(it.toString()) + Log.e("Pixel request exception", it) } ) } diff --git a/parsely/src/main/java/com/parsely/parselyandroid/InMemoryBuffer.kt b/parsely/src/main/java/com/parsely/parselyandroid/InMemoryBuffer.kt index 63f6e70a..069c818e 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/InMemoryBuffer.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/InMemoryBuffer.kt @@ -1,6 +1,5 @@ package com.parsely.parselyandroid -import com.parsely.parselyandroid.Logging.log import kotlin.time.Duration.Companion.seconds import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.delay @@ -23,7 +22,7 @@ internal class InMemoryBuffer( while (isActive) { mutex.withLock { if (buffer.isNotEmpty()) { - log("Persisting ${buffer.size} events") + Log.d("Persisting ${buffer.size} events") localStorageRepository.insertEvents(buffer) buffer.clear() } @@ -36,7 +35,7 @@ internal class InMemoryBuffer( fun add(event: Map) { coroutineScope.launch { mutex.withLock { - log("Event added to buffer") + Log.d("Event added to buffer") buffer.add(event) onEventAddedListener() } diff --git a/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt b/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt index f8dc30fe..0b99c4a6 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt @@ -1,7 +1,6 @@ package com.parsely.parselyandroid import android.content.Context -import com.parsely.parselyandroid.Logging.log import java.io.EOFException import java.io.FileNotFoundException import java.io.ObjectInputStream @@ -35,7 +34,7 @@ internal class LocalStorageRepository(private val context: Context) : QueueRepos oos.close() fos.close() } catch (ex: Exception) { - log("Exception thrown during queue serialization: %s", ex.toString()) + Log.e("Exception thrown during queue serialization", ex) } } @@ -53,10 +52,7 @@ internal class LocalStorageRepository(private val context: Context) : QueueRepos } catch (ex: FileNotFoundException) { // Nothing to do here. Means there was no saved queue. } catch (ex: Exception) { - log( - "Exception thrown during queue deserialization: %s", - ex.toString() - ) + Log.e("Exception thrown during queue deserialization", ex) } return storedQueue } diff --git a/parsely/src/main/java/com/parsely/parselyandroid/Log.kt b/parsely/src/main/java/com/parsely/parselyandroid/Log.kt new file mode 100644 index 00000000..1640b5e0 --- /dev/null +++ b/parsely/src/main/java/com/parsely/parselyandroid/Log.kt @@ -0,0 +1,18 @@ +package com.parsely.parselyandroid + +import android.util.Log + +internal object Log { + + fun i(message: String) { + Log.i("Parsely", message) + } + + fun d(message: String) { + Log.d("Parsely", message) + } + + fun e(message: String, throwable: Throwable? = null) { + Log.e("Parsely", message, throwable) + } +} diff --git a/parsely/src/main/java/com/parsely/parselyandroid/Logging.kt b/parsely/src/main/java/com/parsely/parselyandroid/Logging.kt deleted file mode 100644 index cdee406a..00000000 --- a/parsely/src/main/java/com/parsely/parselyandroid/Logging.kt +++ /dev/null @@ -1,17 +0,0 @@ -package com.parsely.parselyandroid - -import java.util.Formatter - -internal object Logging { - - /** - * Log a message to the console. - */ - @JvmStatic - fun log(logString: String, vararg objects: Any?) { - if (logString == "") { - return - } - println(Formatter().format("[Parsely] $logString", *objects).toString()) - } -} diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTrackerInternal.kt b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTrackerInternal.kt index 66333c86..2a188550 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTrackerInternal.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTrackerInternal.kt @@ -44,7 +44,7 @@ internal class ParselyTrackerInternal internal constructor( inMemoryBuffer = InMemoryBuffer(sdkScope, localStorageRepository) { if (!flushTimerIsActive()) { startFlushTimer() - Logging.log("Flush flushTimer set to %ds", flushManager.intervalMillis / 1000) + Log.d("Flush flushTimer set to ${flushManager.intervalMillis / 1000}") } } flushQueue = FlushQueue( @@ -92,7 +92,7 @@ internal class ParselyTrackerInternal internal constructor( siteIdSource: SiteIdSource, ) { if (url.isBlank()) { - Logging.log("url cannot be empty") + Log.e("url cannot be empty") return } @@ -119,12 +119,12 @@ internal class ParselyTrackerInternal internal constructor( siteIdSource: SiteIdSource, ) { if (url.isBlank()) { - Logging.log("url cannot be empty") + Log.e("url cannot be empty") return } val pageViewUuid = lastPageviewUuid if (pageViewUuid == null) { - Logging.log("engagement session cannot start without calling trackPageview first") + Log.e("engagement session cannot start without calling trackPageview first") return } @@ -147,7 +147,7 @@ internal class ParselyTrackerInternal internal constructor( override fun stopEngagement() { engagementManager?.let { it.stop() - Logging.log("Engagement session has been stopped") + Log.d("Engagement session has been stopped") } engagementManager = null } @@ -160,7 +160,7 @@ internal class ParselyTrackerInternal internal constructor( siteIdSource: SiteIdSource, ) { if (url.isBlank()) { - Logging.log("url cannot be empty") + Log.e("url cannot be empty") return } From 62d05a1d5d6030527f7b5c62dc0b30c761ce5abc Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 10 Jan 2025 15:41:10 +0100 Subject: [PATCH 2/3] tests: replace logging implementation when unit testing EventsBuilder Fixes "Method d in android.util.Log not mocked" --- .../java/com/parsely/parselyandroid/Log.kt | 30 ++++++++++++++----- .../parselyandroid/EventsBuilderTest.kt | 1 + .../com/parsely/parselyandroid/FakeLog.kt | 7 +++++ 3 files changed, 30 insertions(+), 8 deletions(-) create mode 100644 parsely/src/test/java/com/parsely/parselyandroid/FakeLog.kt diff --git a/parsely/src/main/java/com/parsely/parselyandroid/Log.kt b/parsely/src/main/java/com/parsely/parselyandroid/Log.kt index 1640b5e0..2e879b5d 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/Log.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/Log.kt @@ -1,18 +1,32 @@ package com.parsely.parselyandroid -import android.util.Log +import android.util.Log as AndroidLog -internal object Log { +internal object AndroidLogWrapper : Log { - fun i(message: String) { - Log.i("Parsely", message) + override fun i(message: String) { + AndroidLog.i("Parsely", message) } - fun d(message: String) { - Log.d("Parsely", message) + override fun d(message: String) { + AndroidLog.d("Parsely", message) } - fun e(message: String, throwable: Throwable? = null) { - Log.e("Parsely", message, throwable) + override fun e(message: String, throwable: Throwable?) { + AndroidLog.e("Parsely", message, throwable) + } +} + +internal interface Log { + fun i(message: String) + fun d(message: String) + fun e(message: String, throwable: Throwable?) + + companion object { + var instance: Log = AndroidLogWrapper + + fun i(message: String) = instance.i(message) + fun d(message: String) = instance.d(message) + fun e(message: String, throwable: Throwable? = null) = instance.e(message, throwable) } } diff --git a/parsely/src/test/java/com/parsely/parselyandroid/EventsBuilderTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/EventsBuilderTest.kt index cec50c7c..f5ee6deb 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/EventsBuilderTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/EventsBuilderTest.kt @@ -18,6 +18,7 @@ internal class EventsBuilderTest { TEST_SITE_ID, clock ) + Log.instance = FakeLog } @Test diff --git a/parsely/src/test/java/com/parsely/parselyandroid/FakeLog.kt b/parsely/src/test/java/com/parsely/parselyandroid/FakeLog.kt new file mode 100644 index 00000000..d149d094 --- /dev/null +++ b/parsely/src/test/java/com/parsely/parselyandroid/FakeLog.kt @@ -0,0 +1,7 @@ +package com.parsely.parselyandroid + +internal object FakeLog : Log { + override fun i(message: String) = println(message) + override fun d(message: String) = println(message) + override fun e(message: String, throwable: Throwable?) = println(message + throwable?.message) +} From a44209d62b2f07c92442ef9042c0adfd6af5bc40 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 10 Jan 2025 15:54:49 +0100 Subject: [PATCH 3/3] style: extract AndroidLog tag --- parsely/src/main/java/com/parsely/parselyandroid/Log.kt | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/Log.kt b/parsely/src/main/java/com/parsely/parselyandroid/Log.kt index 2e879b5d..54c7620a 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/Log.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/Log.kt @@ -4,16 +4,18 @@ import android.util.Log as AndroidLog internal object AndroidLogWrapper : Log { + private const val TAG = "Parsely" + override fun i(message: String) { - AndroidLog.i("Parsely", message) + AndroidLog.i(TAG, message) } override fun d(message: String) { - AndroidLog.d("Parsely", message) + AndroidLog.d(TAG, message) } override fun e(message: String, throwable: Throwable?) { - AndroidLog.e("Parsely", message, throwable) + AndroidLog.e(TAG, message, throwable) } }