Redesign the Logger API #8645
Redesign the Logger API #8645
Conversation
tgoyne
force-pushed
the
tg/logging-fixes
branch
2 times, most recently
from
4f3ac24 to
6ee666b
Compare
|
The change to set/get a level for a specific category was changed to a static function in all SDKs to avoid the user of the API setting different levels for categories in different loggers and expect the log messages to be filtered by the level for a category set specifically for this logger. |
|
Even though we don't want to expose logging to the end user (even though there is a category for App logs intended for this), my personal opinion is that we may want to log some things sdk side, which is done by others SDKs as well. That's why there are specifically some categories for SDK in the log categories. |
Why would anyone expect the log level set on a logger other than the one in use to have any effect? This concern makes zero sense to me. Using static functions is just setting ourselves up for another API break when we reintroduce the ability to actually use multiple loggers.
We should not do this via RLMLogger. Despite the name, RLMLogger is a log sink, and producing logs is a different operation which should be done via a different type. |
|
I do not see a design doc which mentions anything other than the switch to a nested enum. |
|
Both https://docs.google.com/document/d/1Bna-4oBtcj9Ha3hAGmrKsQz3Jb2cGSG4zbFxauZUkcY/edit and https://docs.google.com/document/d/1Okr1jTBunco0S524GBc_RU9PKZPWLXIsdPJd4Q1U-Qw/edit talk about the logger being singleton and having a single log level. Similarly, all the other SDKs have exposed an API where the log level is a static property that affects all the configured loggers. I don't think we should be deviating from the other SDKs here or from how Core is using it. |
|
Core specifically does not use static functions and puts quite a bit of effort into passing around loggers. The only place where the global logger is read is at the entry points which take a config. A static API and the logger being a singleton are two contradictory ideas. The entire idea of a singleton is that you aren't using static/global functions and there just happens to only be one instance of the object. "A static function that affects all the configured loggers" is also pretty weird because there's only one configured logger. The static function is just fetching |
|
For broader context, the main reason I don't like the static function approach is that as soon as I tried to actually use the new logger functionality I found it both confusing and obnoxious. I first couldn't figure out how to set the log level at all because the idea that you construct a logger, set it as a shared logger, and then set the log levels via a different thing did not even occur to me as an option. After that I tried to switch loggers around to only get trace logging on the specific thing I was debugging, and the API for that turned out to be incredibly weird. Even though the log level is set via a static function, assigning a new logger to If the API was instead that you don't create Logger objects at all and there's just global log levels and a global callback then the API would at least be consistent, but I think that's a worse API than having a singleton object. |
tgoyne
changed the title
tgoyne
force-pushed
the
tg/logging-fixes
branch
4 times, most recently
from
8abdb10 to
cee9c86
Compare
|
Updated this to an API based on the Kotlin one but tweaked a bit for language differences. You now do |
| To remove the default NSLog-logger, call `[RLMLogger removeAll];` first. | ||
|
|
||
| To change the log level, call `[RLMLogger setLevel:forCategory:]`. The | ||
| `RLMLogCategoryRealm` will update all log categories at once. All log | ||
| callbacks share the same log levels and are called for every category. |
There was a problem hiding this comment.
To remove the default NSLog-logger, call [RLMLogger removeAll]; first.
To change the log level, call [RLMLogger setLevel:forCategory:].
The RLMLogCategoryRealm will update all log categories at once. All log
callbacks share the same log levels and are called for every category.
| */ | ||
| @property (nonatomic) RLMLogLevel level | ||
| __attribute__((deprecated("Use `setLevel(level:category)` or `setLevel:category` instead."))); | ||
| + (RLMLoggerToken *)addLogFunction:(RLMLogCategoryFunction)function NS_REFINED_FOR_SWIFT; |
There was a problem hiding this comment.
Seems like docstrings for the new APIs are missing parameters and returns details
Rather than creating Logger instances but then setting the log level via static methods, skip creating Logger instances entirely and do something similar to Kotlin's API with
Logger.add(logFn)which registers a new callback to be called. More than one callback can be registered at once, and callbacks can be dynamically registered and unregistered rather than having to set up the logger before opening the Realm. This differs from Kotlin's design in that we use a token to unregister rather thanLogger.remove()as the latter has some potentially surprising quirks.Logging messages from the SDK is now decoupled from RLMLogger and uses a separate
RLMLog()function which logs directly to the registered callbacks.logCategoryForCategoryName()was unnecessarily slow as it converted the category's name to a NSString, constructed a large NSDictionary, performed a single lookup in that dictionary, and then discarded it for every message logged. Although obj-c does now support creating literal NSDictionaries as compile-time constants that would make this reasonable, that requires a deployment target of iOS 14 and we currently target iOS 12. This change cuts the time spent indo_log()(which includes the actual logging of the strings) when running the tests with loglevel=all by about 20%.The dictionary lookup also turns out to just not be faster than a linear scan on a const array. This may change if we add sufficiently more categories in the future.
The deprecated old version of the logger function still needs to be tested until it's removed entirely.