RCOCOA-2399: Add support for Logger categories/ Improve current logger. #8608
Conversation
RealmSwift/Logger.swift
Outdated
| - parameter category: The log category for the message. | ||
| - parameter message: The message to log. | ||
| */ | ||
| internal func log(level: LogLevel, category: LogCategory = Category.realm, message: String) { |
There was a problem hiding this comment.
This is just makes it easier for us internally to log in the SDK side, we do have a category for this kind of logs Category.sdk in Swift
There was a problem hiding this comment.
There is also a category for app logs Category.app, so we could expose this to the user in that matters, but I agree that we don't need to expose this to the users and add noise to our logging mechanism, but I do agree that we could have some logging in the sdk side, we should discuss how granular we want this because we don't want to contaminate Core's log
dianaafanador3
force-pushed
the
dp/improve-logging-categories
branch
3 times, most recently
from
1472ad6
to
02913a7
Compare
Realm/RLMLogger.mm
Outdated
| } | ||
|
|
||
| - (instancetype)initWithLevel:(RLMLogLevel)level | ||
| category:(RLMLogCategory)category |
There was a problem hiding this comment.
The loggers created with this function are of the default category realm. If you like to filter by category you might need to instantiate a Logger with the category.
There was a problem hiding this comment.
Changed the approach, and now you can only set the global log level for a category using setLogLevel. initWithLevel is gonna is deprecated
dianaafanador3
force-pushed
the
dp/improve-logging-categories
branch
3 times, most recently
from
ee66e9c
to
fea2867
Compare
| XCTAssertTrue(logs.value.contains("Transaction test entry")) | ||
| logs.wrappedValue = "" | ||
|
|
||
| logger.log(with: .info, categoryName: Category.realm.rawValue, message: "REALM test entry") |
There was a problem hiding this comment.
Why is this passing the raw category name rather than the LogCategory?
There was a problem hiding this comment.
Because we cannot expose internal function to the test suite, instead we are using Realm.Private API to generate the logs for the tests
dianaafanador3
force-pushed
the
dp/improve-logging-categories
branch
from
734224d
to
7c94c77
Compare
| */ | ||
| typedef NS_ENUM(NSUInteger, RLMLogCategory) { | ||
| /// Top level log category for Realm, updating this category level would set all other subcategories too. | ||
| RLMLogCategoryRealm, |
There was a problem hiding this comment.
[nit] This naming scheme is causing a bit of tautology as we have the RLM prefix and the Realm suffix/infix. One option would be to have
RLMLogCategoryAll,
RLMLogCategorySDK,
...
Essentially dropping the Realm as it's part of every category, thus not bringing any new information. Not a blocker though.
There was a problem hiding this comment.
Should we change RLMLogCategoryRealm as well?, given that this is the keyword we use for this categories in all SDKs?
There was a problem hiding this comment.
Yeah, I feel a bit ambiguous about it - I'd be fine with either keeping it as RLMLogCategoryRealm or with replacing it with RLMLogCategoryAll. I think in other SDKs we can get away with it because we don't have the RLM prefix (similarly to what we do in Swift). That being said, perhaps we shouldn't overthink it as Obj C users are so few and whether we have Realm in there or not is unlikely to make a huge difference.
There was a problem hiding this comment.
In Swift we have Category.realm so to keep uniformity in both APIs, we can keep it like that, and then remove Realm for the rest of the categories
There was a problem hiding this comment.
Added changes
dianaafanador3
force-pushed
the
dp/improve-logging-categories
branch
from
39bc5d6
to
762ab1e
Compare
dianaafanador3
changed the title
…r. (#8608) Add support for Logger categories
…r. (#8608) Add support for Logger categories
the log level for each category as well.
This has a proposal for the Categories enum which includes nested categories, for Objective-C we are using strings as it may be very complicated to have nested enums and it may be too mucho work for the number of users using Objective-C