-
Notifications
You must be signed in to change notification settings - Fork 320
Lazy logs #6439
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
Lazy logs #6439
Changes from 3 commits
264ce4e
9858d51
773484f
60c5793
5a5c55c
d138292
0a76da4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ package com.mapbox.navigation.utils.internal | |
|
|
||
| import androidx.annotation.VisibleForTesting | ||
| import com.mapbox.base.common.logger.Logger | ||
| import com.mapbox.common.LoggingLevel | ||
|
|
||
| /** | ||
| * Singleton provider of [Logger]. | ||
|
|
@@ -39,6 +40,18 @@ fun logD(msg: String, category: String? = null) { | |
| LoggerProvider.frontend.logD(msg, category) | ||
| } | ||
|
|
||
| /** | ||
| * @param category optional string to identify the source or category of the log message. | ||
| * @param lazyMsg is a lazy message to log. Isn't executed if current log level less verbose than Debug. | ||
| * Noting that the category is appended to the log message to give extra context along with the `[nav-sdk]` parent category. | ||
| * As an example, this is how the logs would look like `D/Mapbox: [nav-sdk] [ConnectivityHandler] NetworkStatus=ReachableViaWiFi`. | ||
| */ | ||
| inline fun logD(category: String? = null, lazyMsg: () -> String) { | ||
| if (logLevel().atLeast(LoggingLevel.DEBUG)) { | ||
| logD(lazyMsg(), category) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * @param msg to log. | ||
| * @param category optional string to identify the source or category of the log message. | ||
|
|
@@ -49,6 +62,18 @@ fun logI(msg: String, category: String? = null) { | |
| LoggerProvider.frontend.logI(msg, category) | ||
| } | ||
|
|
||
| /** | ||
| * @param category optional string to identify the source or category of the log message. | ||
| * @param lazyMsg is a lazy message to log. Isn't executed if current log level less verbose than Inline. | ||
| * Noting that the category is appended to the log message to give extra context along with the `[nav-sdk]` parent category. | ||
| * As an example, this is how the logs would look like `I/Mapbox: [nav-sdk] [ConnectivityHandler] NetworkStatus=ReachableViaWiFi`. | ||
| */ | ||
| inline fun logI(category: String? = null, lazyMsg: () -> String) { | ||
| if (logLevel().atLeast(LoggingLevel.INFO)) { | ||
| logI(lazyMsg(), category) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * @param msg to log. | ||
| * @param category optional string to identify the source or category of the log message. | ||
|
|
@@ -59,6 +84,18 @@ fun logW(msg: String, category: String? = null) { | |
| LoggerProvider.frontend.logW(msg, category) | ||
| } | ||
|
|
||
| /** | ||
| * @param category optional string to identify the source or category of the log message. | ||
| * @param lazyMsg is a lazy message to log. Isn't executed if current log level less verbose than Warning. | ||
| * Noting that the category is appended to the log message to give extra context along with the `[nav-sdk]` parent category. | ||
| * As an example, this is how the logs would look like `W/Mapbox: [nav-sdk] [ConnectivityHandler] NetworkStatus=ReachableViaWiFi`. | ||
| */ | ||
| inline fun logW(category: String? = null, lazyMsg: () -> String) { | ||
| if (logLevel().atLeast(LoggingLevel.WARNING)) { | ||
| logW(lazyMsg(), category) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * @param msg to log. | ||
| * @param category optional string to identify the source or category of the log message. | ||
|
|
@@ -68,3 +105,20 @@ fun logW(msg: String, category: String? = null) { | |
| fun logE(msg: String, category: String? = null) { | ||
| LoggerProvider.frontend.logE(msg, category) | ||
| } | ||
|
|
||
| /** | ||
| * @param category optional string to identify the source or category of the log message. | ||
| * @param lazyMsg is a lazy message to log. Isn't executed if current log level less verbose than Error. | ||
| * Noting that the category is appended to the log message to give extra context along with the `[nav-sdk]` parent category. | ||
| * As an example, this is how the logs would look like `E/Mapbox: [nav-sdk] [ConnectivityHandler] NetworkStatus=ReachableViaWiFi`. | ||
| */ | ||
| inline fun logE(category: String? = null, lazyMsg: () -> String) { | ||
| if (logLevel().atLeast(LoggingLevel.ERROR)) { | ||
| logE(lazyMsg(), category) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Should not be used directly. Added to support inline calls. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does it help with inlining?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Clarified documentation. Does it make sense now?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aaah, I see. But why shouldn't it be used directly? It doesn't look like it can hurt anyone.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It just feels like internal implementation detail and I can't keep it internal because of inline, so I decided to restrict usage by forbidding it in the comments 😅 |
||
| */ | ||
| fun logLevel() = LoggerProvider.frontend.getLogLevel() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By the way, you can make it
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Amazing! ❤️ |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| package com.mapbox.navigation.utils.internal | ||
|
|
||
| import com.mapbox.common.LoggingLevel | ||
|
|
||
| fun LoggingLevel?.atLeast(loggingLevel: LoggingLevel): Boolean { | ||
| return toPriority(this) >= toPriority(loggingLevel) | ||
| } | ||
|
|
||
| private fun toPriority(loggingLevel: LoggingLevel?) = when (loggingLevel) { | ||
| null -> 0 | ||
| LoggingLevel.DEBUG -> 1 | ||
| LoggingLevel.INFO -> 2 | ||
| LoggingLevel.WARNING -> 3 | ||
| LoggingLevel.ERROR -> 4 | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| package com.mapbox.navigation.utils.internal | ||
|
|
||
| import com.mapbox.common.LoggingLevel | ||
| import org.junit.Assert.assertEquals | ||
| import org.junit.Test | ||
| import org.junit.runner.RunWith | ||
| import org.junit.runners.Parameterized | ||
|
|
||
| @RunWith(Parameterized::class) | ||
| class LoggingLevelUtilKtTest(val parameter: LogLevelData) { | ||
|
|
||
| companion object { | ||
|
|
||
| @JvmStatic | ||
| @Parameterized.Parameters(name = "{0}") | ||
| fun parameters() = listOf( | ||
| LogLevelData( | ||
| what = null, | ||
| atLeast = LoggingLevel.ERROR, | ||
| expected = false | ||
| ), | ||
| LogLevelData( | ||
| what = null, | ||
| atLeast = LoggingLevel.WARNING, | ||
| expected = false | ||
| ), | ||
| LogLevelData( | ||
| what = null, | ||
| atLeast = LoggingLevel.INFO, | ||
| expected = false | ||
| ), | ||
| LogLevelData( | ||
| what = null, | ||
| atLeast = LoggingLevel.DEBUG, | ||
| expected = false | ||
| ), | ||
|
|
||
| LogLevelData( | ||
| what = LoggingLevel.ERROR, | ||
| atLeast = LoggingLevel.ERROR, | ||
| expected = true | ||
| ), | ||
| LogLevelData( | ||
| what = LoggingLevel.ERROR, | ||
| atLeast = LoggingLevel.WARNING, | ||
| expected = true | ||
| ), | ||
| LogLevelData( | ||
| what = LoggingLevel.ERROR, | ||
| atLeast = LoggingLevel.INFO, | ||
| expected = true | ||
| ), | ||
| LogLevelData( | ||
| what = LoggingLevel.ERROR, | ||
| atLeast = LoggingLevel.DEBUG, | ||
| expected = true | ||
| ), | ||
|
|
||
| LogLevelData( | ||
| what = LoggingLevel.WARNING, | ||
| atLeast = LoggingLevel.ERROR, | ||
| expected = false | ||
| ), | ||
| LogLevelData( | ||
| what = LoggingLevel.WARNING, | ||
| atLeast = LoggingLevel.WARNING, | ||
| expected = true | ||
| ), | ||
| LogLevelData( | ||
| what = LoggingLevel.ERROR, | ||
|
vadzim-vys marked this conversation as resolved.
Outdated
|
||
| atLeast = LoggingLevel.INFO, | ||
| expected = true | ||
| ), | ||
| LogLevelData( | ||
| what = LoggingLevel.ERROR, | ||
|
vadzim-vys marked this conversation as resolved.
Outdated
|
||
| atLeast = LoggingLevel.DEBUG, | ||
| expected = true | ||
| ), | ||
|
|
||
| LogLevelData( | ||
| what = LoggingLevel.INFO, | ||
| atLeast = LoggingLevel.ERROR, | ||
| expected = false | ||
| ), | ||
| LogLevelData( | ||
| what = LoggingLevel.INFO, | ||
| atLeast = LoggingLevel.WARNING, | ||
| expected = false | ||
| ), | ||
| LogLevelData( | ||
| what = LoggingLevel.INFO, | ||
| atLeast = LoggingLevel.INFO, | ||
| expected = true | ||
| ), | ||
| LogLevelData( | ||
| what = LoggingLevel.INFO, | ||
| atLeast = LoggingLevel.DEBUG, | ||
| expected = true | ||
| ), | ||
|
|
||
| LogLevelData( | ||
| what = LoggingLevel.DEBUG, | ||
| atLeast = LoggingLevel.ERROR, | ||
| expected = false | ||
| ), | ||
| LogLevelData( | ||
| what = LoggingLevel.DEBUG, | ||
| atLeast = LoggingLevel.WARNING, | ||
| expected = false | ||
| ), | ||
| LogLevelData( | ||
| what = LoggingLevel.DEBUG, | ||
| atLeast = LoggingLevel.INFO, | ||
| expected = false | ||
| ), | ||
| LogLevelData( | ||
| what = LoggingLevel.DEBUG, | ||
| atLeast = LoggingLevel.DEBUG, | ||
| expected = true | ||
| ), | ||
| ) | ||
| } | ||
|
|
||
| @Test | ||
| fun `test atLeast`() { | ||
| val result = parameter.what.atLeast(parameter.atLeast) | ||
| assertEquals(parameter.expected, result) | ||
| } | ||
|
|
||
| data class LogLevelData( | ||
| val what: LoggingLevel?, | ||
| val atLeast: LoggingLevel, | ||
| val expected: Boolean | ||
| ) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is "current log level less verbose than Inline"?
It also should be info, not inline. But I still don't get the meaning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks 👍
I should have waited for tomorrow instead of quickly finishing PR this evening 🤣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After fixing grammar errors and typos it sounds like " The lazy message isn't executed if current log level is less verbose than Info". With less verbose log level you see less details in the logs. For example Error is less verbose than Info, if you set current log level to Error you will see less logs. It doesn't make sense to execute lazy message with verbose level Info if current log level Error, because nobody will see the message anyway. Does it make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Then "if current log level is less verbose" and we should be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you've fixed that already.