Lazy logs #6439
Lazy logs #6439
Conversation
VysotskiVadim
added
the
skip changelog
|
great idea 👍 what if we change API so that every message becomes a lazy message? it will align API |
Codecov Report
@@ Coverage Diff @@
## main #6439 +/- ##
============================================
- Coverage 69.41% 69.37% -0.05%
Complexity 4651 4651
============================================
Files 697 698 +1
Lines 27510 27536 +26
Branches 3204 3210 +6
============================================
+ Hits 19097 19104 +7
- Misses 7169 7187 +18
- Partials 1244 1245 +1
|
| NativeLoggerWrapper.debug(message, NAV_SDK_CATEGORY) | ||
| } | ||
|
|
||
| override fun logD(category: String?, lazyMsg: () -> String) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Made methods inline to avoid creation of an object for lambda
There was a problem hiding this comment.
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.
libnavigation-util/src/main/java/com/mapbox/navigation/utils/internal/LoggerFrontend.kt
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| override fun logD(category: String?, lazyMsg: () -> String) { | ||
| when (LogConfiguration.getLoggingLevel()) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| /** | ||
| * @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. |
There was a problem hiding this comment.
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.
It also should be info, not inline
thanks 👍
I should have waited for tomorrow instead of quickly finishing PR this evening 🤣
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Thanks. Then "if current log level is less verbose" and we should be good.
There was a problem hiding this comment.
Ah, you've fixed that already.
| } | ||
|
|
||
| /** | ||
| * Should not be used directly. Added to support inline calls. |
There was a problem hiding this comment.
How does it help with inlining?
There was a problem hiding this comment.
Clarified documentation. Does it make sense now?
There was a problem hiding this comment.
Aaah, I see. But why shouldn't it be used directly? It doesn't look like it can hurt anyone.
There was a problem hiding this comment.
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 😅
libnavigation-util/src/test/java/com/mapbox/navigation/utils/internal/LoggingLevelUtilKtTest.kt
Outdated
Show resolved
Hide resolved
libnavigation-util/src/test/java/com/mapbox/navigation/utils/internal/LoggingLevelUtilKtTest.kt
Outdated
Show resolved
Hide resolved
libnavigation-util/src/main/java/com/mapbox/common/NativeLoggerWrapper.kt
Show resolved
Hide resolved
| * Should not be used directly. | ||
| * Added to support inline calls. Public inline functions can use only public API inside. | ||
| */ | ||
| fun logLevel() = LoggerProvider.frontend.getLogLevel() |
There was a problem hiding this comment.
By the way, you can make it internal if you add @PublishedApi annotation.
5 participants
Description
I'd like to add many debug logs to #6434. For example I want to logs ids of passed routes:
routes.map { it.id }.joinToString(separator = ", ") {it}. But I worry that extensive logging with all this list transformations and concatenations may reduce performance for users who don't want to read them. What do you think about lazy logging where passed lazy messages are executed only if it makes sense for current log level?