Skip to content

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.

Sign up for GitHub

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

Merged
VysotskiVadim merged 7 commits into from
Oct 6, 2022
Merged

Lazy logs #6439

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
264ce4e
introduced lazy logging
VysotskiVadim Oct 5, 2022
9858d51
made log inline
VysotskiVadim Oct 5, 2022
773484f
fixed styles + added lazy messages to all log options
VysotskiVadim Oct 5, 2022
60c5793
fixed tests
VysotskiVadim Oct 6, 2022
5a5c55c
clarified docs
VysotskiVadim Oct 6, 2022
d138292
fixed docs
VysotskiVadim Oct 6, 2022
0a76da4
fixed getting of log level from common
VysotskiVadim Oct 6, 2022
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
7 changes: 3 additions & 4 deletions libnavigation-base/src/main/java/com/mapbox/navigation/base/route/NavigationRoute.kt
View file
Open in desktop
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
10 changes: 10 additions & 0 deletions libnavigation-util/src/main/java/com/mapbox/navigation/utils/internal/LoggerFrontend.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
package com.mapbox.navigation.utils.internal

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

private const val NAV_SDK_CATEGORY = "nav-sdk"

interface LoggerFrontend {
fun logV(msg: String, category: String? = null)
fun logD(msg: String, category: String? = null)
fun logD(category: String? = null, lazyMsg: () -> String)
fun logI(msg: String, category: String? = null)
fun logE(msg: String, category: String? = null)
fun logW(msg: String, category: String? = null)
Expand All @@ -24,6 +27,13 @@ internal class MapboxCommonLoggerFrontend : LoggerFrontend {
NativeLoggerWrapper.debug(message, NAV_SDK_CATEGORY)
}

override fun logD(category: String?, lazyMsg: () -> String) {
Copy link
Contributor

@dzinad dzinad Oct 5, 2022

Choose a reason for hiding this comment

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

I like that. The lambda would still be created but I don't see how we could evade that.
Even if we make the logD method inline (which is not possible with interfaces), LogConfiguration.getLoggingLevel() is not a constant, so the lambda would not be removed anyway.

Copy link

@LukasPaczos LukasPaczos Oct 5, 2022

Choose a reason for hiding this comment

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

LogConfiguration.getLoggingLevel() is not a constant

Inlining doesn't require everything in the lambda to be constant, does it? LogConfiguration.getLoggingLevel() is statically accessible to the lambda, so it should work just fine (at least that's what I see in decompiled classes). @VysotskiVadim I think it would be worth exploring how we could make this setup work with inline functions, I guess the biggest hurdle would be organizing the interface in a way that makes it possible.

Copy link
Contributor Author

@VysotskiVadim VysotskiVadim Oct 5, 2022

Choose a reason for hiding this comment

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

Made methods inline to avoid creation of an object for lambda

Copy link
Contributor

@dzinad dzinad Oct 5, 2022

Choose a reason for hiding this comment

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

Oh, right, I was thinking in other terms. If LogConfiguration.getLoggingLevel() was a constant and the inlining was used, the whole code might have been removed after the compilation because it would be unreachable.
But now yes, we have a code that creates a lambda but it won't be invoked if unnecessary, that works.

when (LogConfiguration.getLoggingLevel()) {
Copy link
Contributor

@dzinad dzinad Oct 5, 2022

Choose a reason for hiding this comment

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

This level can change. We should keep in mind (better even in docs) that if you had level info, invoked the lazy log and then changed the level to debug, you won't see the log. I'm not sure how it works with usual logs now but in AS it's just a filter so you'll see the old logs as well.

Copy link

@LukasPaczos LukasPaczos Oct 5, 2022

Choose a reason for hiding this comment

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

LogConfiguration is a runtime option. By default it's set to INFO and changing it in the runtime won't print all the old DEBUG logs, so it's consistent across the board.

LoggingLevel.DEBUG -> logD(lazyMsg(), category)
LoggingLevel.ERROR, LoggingLevel.INFO, LoggingLevel.WARNING -> { }
}
}

override fun logI(msg: String, category: String?) {
val message = createMessage(msg, category)
NativeLoggerWrapper.info(message, NAV_SDK_CATEGORY)
Expand Down
10 changes: 10 additions & 0 deletions libnavigation-util/src/main/java/com/mapbox/navigation/utils/internal/LoggerProvider.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,16 @@ 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 then 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`.
*/
fun logD(category: String? = null, lazyMsg: () -> String) {
LoggerProvider.frontend.logD(category, lazyMsg)
}

/**
* @param msg to log.
* @param category optional string to identify the source or category of the log message.
Expand Down
3 changes: 3 additions & 0 deletions ...ng-navigation-util/src/main/java/com/mapbox/navigation/testing/LoggingFrontendTestRule.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ private object NoLoggingFrontend : LoggerFrontend {
override fun logD(msg: String, category: String?) {
}

override fun logD(category: String?, lazyMsg: () -> String) {
}

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

Expand Down