Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
Original file line number Diff line number Diff line change
Expand Up @@ -184,10 +184,9 @@ class NavigationRoute internal constructor(
}
val deferredRouteOptionsParsing = async(ThreadController.DefaultDispatcher) {
RouteOptions.fromUrl(URL(routeRequestUrl)).also {
logD(
"parsed request url to RouteOptions: ${it.toUrl("***")}",
LOG_CATEGORY
)
logD(LOG_CATEGORY) {
"parsed request url to RouteOptions: ${it.toUrl("***")}"
}
}
}
create(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,6 @@ internal object NativeLoggerWrapper {
fun error(message: String, category: String?) {
Log.error(message, category)
}

fun getLogLevel() = LogConfiguration.getLoggingLevel()
}
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
package com.mapbox.navigation.utils.internal

import com.mapbox.common.LoggingLevel
import com.mapbox.common.NativeLoggerWrapper

private const val NAV_SDK_CATEGORY = "nav-sdk"

interface LoggerFrontend {
fun getLogLevel(): LoggingLevel?
fun logV(msg: String, category: String? = null)
fun logD(msg: String, category: String? = null)
fun logI(msg: String, category: String? = null)
Expand All @@ -13,6 +15,9 @@ interface LoggerFrontend {
}

internal class MapboxCommonLoggerFrontend : LoggerFrontend {

override fun getLogLevel() = NativeLoggerWrapper.getLogLevel()

override fun logV(msg: String, category: String?) {
val message = createMessage(msg, category)
// There's no com.mapbox.common.Log.verbose available - using Log.debug instead
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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].
Expand Down Expand Up @@ -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.
Expand All @@ -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.
Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It also should be info, not inline

thanks 👍
I should have waited for tomorrow instead of quickly finishing PR this evening 🤣

Copy link
Copy Markdown
Contributor Author

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"?

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?

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor

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.

* 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.
Expand All @@ -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.
Expand All @@ -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.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How does it help with inlining?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Clarified documentation. Does it make sense now?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

By the way, you can make it internal if you add @PublishedApi annotation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Amazing! ❤️
#6447

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,
Comment thread
vadzim-vys marked this conversation as resolved.
Outdated
atLeast = LoggingLevel.INFO,
expected = true
),
LogLevelData(
what = LoggingLevel.ERROR,
Comment thread
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
)
}
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
package com.mapbox.navigation.testing

import android.annotation.SuppressLint
import com.mapbox.common.LoggingLevel
import com.mapbox.navigation.utils.internal.LoggerFrontend
import com.mapbox.navigation.utils.internal.LoggerProvider
import org.junit.rules.TestRule
import org.junit.runner.Description
import org.junit.runners.model.Statement

private object NoLoggingFrontend : LoggerFrontend {

override fun getLogLevel(): LoggingLevel? = null

override fun logV(msg: String, category: String?) {
}

Expand Down