Redesign the Logger API

[tgoyne]

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 than Logger.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 in do_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.

[dianaafanador3]

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. https://github.com/realm/realm-kotlin/pull/1692/files#diff-834583a499b615bec40974670a893a7d34c2972fbf6d42c705cb7a1df7963f86 You can also check the design document

[dianaafanador3]

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. So, I don't see any harm exposing this internally.

[tgoyne]

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.

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.

my personal opinion is that we may want to log some things sdk side, which is done by others SDKs as well.

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.

[tgoyne]

I do not see a design doc which mentions anything other than the switch to a nested enum.

[nirinchev]

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.

[tgoyne]

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 Logger.shared and modifying that, and that's weirdly roundabout compared to having the user just write that.

[tgoyne]

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 Logger.shared resets all of the log levels to the default because despite what the API implies, the log levels are actually set on specific logger and aren't a global setting.

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]

Updated this to an API based on the Kotlin one but tweaked a bit for language differences. You now do Logger.add { level, category, message in ... } and dynamically adding/removing logger callbacks is supported.