Logging updates #130

evan-masseau · 2024-02-13T21:32:17Z

Description

We intentionally disabled all log output from the SDK in our earlier releases. In order to provide some useful logging, I'm enabling it and merging log implementations. Rather than silence all output, I'm putting in a configurable log level that defaults to less verbose output. Warning+ for debug and Error+ for release.

Check List

Are you changing anything with the public API? NO
Are your changes backwards compatible with previous SDK Versions? YES
Have you tested this change on real device? YES
Have you added unit test coverage for your changes? YES
Have you verified that your changes are compatible with all Android versions the SDK currently supports? YES

Changelog / Code Overview

Enable logging in published SDK
Added warning level
Added level-aware logging with a configurable threshold
Threshold has a default based on debug vs release, and can be configured from code and/or manifest file
Adjusted log levels throughout the code
Updated tests, removed some old log related-test code from earlier build variants/flavors

Test Plan

Unit tests
Will test more extensively from android and react native test apps as I work on the wrapped API methods

Related Issues/Tickets

CHNL-5491

- Enable logging in published SDK - Added level-aware logging with a configurable threshold - Added warning level - Threshold has a default based on debug/release, can be configured from code and from manifest - Updated tests, removed some old log related test code from earlier build variants/flavors - Adjusted log levels throughout the code

Make sure our log tag contains "Klaviyo"

ajaysubra

NICE!

ndurell · 2024-02-16T17:03:50Z

sdk/analytics/src/main/java/com/klaviyo/analytics/networking/KlaviyoApiClient.kt

                emptyArray<String>()
            }
        }?.forEach { uuid ->
            Registry.dataStore.fetch(uuid).let { json ->
                if (json == null) {
-                    Registry.log.error("Missing request JSON for $uuid")
+                    Registry.log.debug("Missing request JSON for $uuid")


hmm why go from error to debug?

Previously, all these levels were only here for us to see, because in a published build there was no logging.
I didn't think this was worth logging in the client's application, so I lowered it so it won't be visible by the default logging level in a release build.

ndurell · 2024-02-16T17:25:41Z

sdk/core/src/main/java/com/klaviyo/core/KLog.kt

+         */
+        private const val LOG_LEVEL = "com.klaviyo.core.log_level"
+
+        private val defaultLogLevel = if (BuildConfig.DEBUG) {


Can you override this with the manifest setting?

Yep, this is the default based on build config, then the host app can set it from the manifest, or by importing core and changing logLevel at runtime (which I probably won't advertise, but wanted for the test app)

evan-masseau · 2024-02-16T17:29:48Z

sdk/core/src/main/java/com/klaviyo/core/KLog.kt

+
+            return _logLevel ?: defaultLogLevel
+        }
+        set(value) {


Since this is a public setter, you can also change log level from runtime if you import core from gradle

ndurell · 2024-02-16T17:33:10Z

sdk/analytics/src/main/java/com/klaviyo/analytics/networking/KlaviyoApiClient.kt

@@ -152,13 +152,13 @@ internal object KlaviyoApiClient : ApiClient {
            } catch (exception: JSONException) {
                wasMutated = true
                Registry.log.wtf("Invalid persistent queue JSON", exception)
-                Registry.log.debug(it)
+                Registry.log.info(it)


Just noting this jumped from debug to info whereas other jumps were maybe not as severe I think.

You're correct. Almost all cases I reduced log level. But there were two cases where we had a wtf error followed by some additional explanatory data. I thought in those cases info made more sense.

ndurell · 2024-02-16T17:34:54Z

sdk/analytics/src/main/java/com/klaviyo/analytics/networking/KlaviyoApiClient.kt

@@ -169,8 +169,8 @@ internal object KlaviyoApiClient : ApiClient {
                    } catch (exception: JSONException) {
                        wasMutated = true
                        Registry.log.wtf("Invalid request JSON $uuid", exception)
+                        Registry.log.info(json)


Just double checking here...you changed the order of operations but this seems consistent with the block above so I think it's fine.

ndurell · 2024-02-16T17:35:22Z

sdk/analytics/src/test/java/com/klaviyo/analytics/DevicePropertiesTest.kt

@@ -13,6 +13,8 @@ import org.junit.Test
 internal class DevicePropertiesTest : BaseTest() {

    private val mockVersionCode = 123
+
+    @Suppress("DEPRECATION")


PackageInfo is deprecated?

I forget which, either versionCode or longVersionCode is deprecated, but we have to use it to support past versions of android so its annoying to have the warning always popping up.

ndurell · 2024-02-16T17:37:25Z

sdk/core/src/main/java/com/klaviyo/core/KLog.kt

+            if (_logLevel == null && Registry.isRegistered<Config>()) {
+                _logLevel = Registry.config.getManifestInt(LOG_LEVEL, defaultLogLevel.ordinal).let {
+                    Level.entries.getOrNull(it) ?: defaultLogLevel.also { default ->
+                        default.log(


That's a mindbender logging that you can't get the log level! 😂

ndurell · 2024-02-16T17:37:46Z

sdk/core/src/main/java/com/klaviyo/core/KLog.kt

+                _logLevel = Registry.config.getManifestInt(LOG_LEVEL, defaultLogLevel.ordinal).let {
+                    Level.entries.getOrNull(it) ?: defaultLogLevel.also { default ->
+                        default.log(
+                            "KlaviyoLog",


Worth a constant maybe?

ndurell · 2024-02-16T17:38:42Z

sdk/core/src/main/java/com/klaviyo/core/LogLevel.kt

+    Level.Warning -> Log.w(tag, msg, ex)
+    Level.Error -> Log.e(tag, msg, ex)
+    Level.Assert -> Log.wtf(tag, msg, ex)
+    Level.None -> 0


Does log return a number or just have to return something?

Yeah all the android util methods return Int so I just kept it consistent. I could also convert it to return void/Unit i suppose.

ndurell · 2024-02-16T17:41:22Z

sdk/core/src/main/java/com/klaviyo/core/KLog.kt

+    private var _logLevel: Level? = null
+    override var logLevel: Level
+        get() {
+            if (_logLevel == null && Registry.isRegistered<Config>()) {


Why check isRegistered here?

This is checking if applicationContext will be available through Config. Without it, we can't read from the manifest.

Otherwise, accessing Registry.Config would throw an exception, then we'd try to log the exception... ♾️

ndurell · 2024-02-16T17:46:11Z

sdk/core/src/main/java/com/klaviyo/core/config/Log.kt

+     *
+     * To configure from manifest, specify the minimum log level to output:
+     *
+     * 0 = Disable logging


Hmm...is this reversed from the setting we documented?

Oh crud, yeah good catch, now to remember which one is correct 🤣

Maybe not reversed is assert in the wrong spot though or doesn't matter?

I think the readme is currently incorrect. I think I went with 0 = disable, 1 = verbose+, 2 = debug+ etc.

https://developer.android.com/reference/android/util/Log it should match stuff in here right?

Yes, in spirit anyway. For some reason theirs go from 2 to 7. But lower number = lower severity was my intent. I fixed values in the readme

sdk/core/src/test/java/com/klaviyo/core/RegistryTest.kt

sdk/core/src/main/java/com/klaviyo/core/KLog.kt

…ecm/logging # Conflicts: # sdk/analytics/src/main/java/com/klaviyo/analytics/Klaviyo.kt # sdk/core/src/test/java/com/klaviyo/core/RegistryTest.kt

…ze method (#131)* Internalize the lifecycle listener attachment.* Mark the public property to be deprecated and replace with a no-op implementation to avoid duplicate listeners.* Update readme instructions, migration guide.---------Co-authored-by: Evan Masseau <>" (#136) * Auto generate documentation PRs (#124) Automate all the things Co-authored-by: Evan Masseau <> * Minor README and Documentation Updates (#121) * Fixed SDK name in license section * Fixed appearance of some of the advanced section after seeing it on the dokka html * Other grammar/spelling fixes --------- Co-authored-by: Evan Masseau <> * Add note about anonymous profiles to readme (#127) --------- Co-authored-by: Evan Masseau <> * Wrap public methods in try/catch (#129) Added safety checks around public API methods to prevent throwing exceptions that would otherwise crash the host application --------- Co-authored-by: Evan Masseau <> * Logging (#130) - Enable logging in published SDK - Added level-aware logging with a configurable threshold from code or manifest file. - Added warning level - Threshold has a default based on debug/release, can be configured from code and from manifest - Updated tests, removed some old log related test code from earlier build variants/flavors - Adjusted log levels throughout the code --------- Co-authored-by: Evan Masseau <> * Attach lifecycle listeners internally in initialize method (#131) * Internalize the lifecycle listener attachment. * Mark the public property to be deprecated and replace with a no-op implementation to avoid duplicate listeners. * Update readme instructions, migration guide. --------- Co-authored-by: Evan Masseau <> * Generated docs for e9598b0 --------- Co-authored-by: Evan C Masseau <5167687+evan-masseau@users.noreply.github.com> Co-authored-by: github-actions <github-actions@github.com>

…ation * commit 'b12ef65b0d460b343c4d1eb2bac6e227a6f51d62': Release Version 2.1.0 (#137) Attach lifecycle listeners internally in initialize method (#131) Logging (#130) Wrap public methods in try/catch (#129) Add note about anonymous profiles to readme (#127) Minor README and Documentation Updates (#121) Auto generate documentation PRs (#124)

evan-masseau requested a review from a team as a code owner February 13, 2024 21:32

evan-masseau requested review from ajaysubra and kennyklaviyo February 13, 2024 21:32

Evan Masseau added 2 commits February 13, 2024 17:04

Add instructions for manifest option

7758abf

Log if manifest value is invalid.

d6fe280

Make sure our log tag contains "Klaviyo"

ajaysubra approved these changes Feb 14, 2024

View reviewed changes

evan-masseau mentioned this pull request Feb 16, 2024

Attach lifecycle listeners internally in initialize method #131

Merged

5 tasks

ndurell reviewed Feb 16, 2024

View reviewed changes

evan-masseau commented Feb 16, 2024

View reviewed changes

ndurell reviewed Feb 16, 2024

View reviewed changes

Fix level integers in the readme

4a7580d

ndurell reviewed Feb 16, 2024

View reviewed changes

sdk/core/src/test/java/com/klaviyo/core/RegistryTest.kt Outdated Show resolved Hide resolved

ndurell approved these changes Feb 16, 2024

View reviewed changes

evan-masseau mentioned this pull request Feb 16, 2024

Wrap public methods in try/catch #129

Merged

5 tasks

kennyklaviyo reviewed Feb 20, 2024

View reviewed changes

sdk/core/src/main/java/com/klaviyo/core/KLog.kt Outdated Show resolved Hide resolved

Evan Masseau added 2 commits February 20, 2024 16:01

Merge branch 'master' of github.com:klaviyo/klaviyo-android-sdk into …

ceb73d5

…ecm/logging # Conflicts: # sdk/analytics/src/main/java/com/klaviyo/analytics/Klaviyo.kt # sdk/core/src/test/java/com/klaviyo/core/RegistryTest.kt

Eliminate extra/erroneous listing of the log level numbers

f05cbcf

kennyklaviyo approved these changes Feb 20, 2024

View reviewed changes

evan-masseau merged commit 1500879 into master Feb 20, 2024
1 of 2 checks passed

evan-masseau deleted the ecm/logging branch February 20, 2024 21:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Logging updates #130

Logging updates #130

evan-masseau commented Feb 13, 2024

ajaysubra left a comment

ndurell Feb 16, 2024

evan-masseau Feb 16, 2024

ndurell Feb 16, 2024

evan-masseau Feb 16, 2024

evan-masseau Feb 16, 2024

ndurell Feb 16, 2024

evan-masseau Feb 16, 2024

ndurell Feb 16, 2024

ndurell Feb 16, 2024

evan-masseau Feb 16, 2024

ndurell Feb 16, 2024

ndurell Feb 16, 2024

ndurell Feb 16, 2024

evan-masseau Feb 16, 2024

ndurell Feb 16, 2024

evan-masseau Feb 16, 2024

evan-masseau Feb 16, 2024

ndurell Feb 16, 2024

evan-masseau Feb 16, 2024

ndurell Feb 16, 2024

evan-masseau Feb 16, 2024

ndurell Feb 16, 2024

evan-masseau Feb 16, 2024

Logging updates #130

Logging updates #130

Conversation

evan-masseau commented Feb 13, 2024

Description

Check List

Changelog / Code Overview

Test Plan

Related Issues/Tickets

ajaysubra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment