Skip to content
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.

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

CBL-5396: Use os_log instead of NSLog for Console Log #3323

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

velicuvlad
Copy link
Contributor

@velicuvlad velicuvlad commented Aug 19, 2024

  1. each LogLevel has been matched with a type of os_log - same behaviour as .NET

  2. Additionally, os_log categories, mapped to DomainLevels, will now be created at the time logging is initialised, on open/create db. And every CBL log, with its specific domain, will now appear in its own corresponding category in console logs.

image

  1. There are just a couple of NSLogs only for DEBUG, which for I've decided that the os_log object to be created dynamically as we dont care that much and based on those messages, there's an issue somewhere else anyway. The production ones will be generated as mentioned above.

  2. Removed CBLLog+Admin.h as is no longer needed.

@velicuvlad velicuvlad requested a review from pasin August 27, 2024 14:44
@pasin
Copy link
Contributor

pasin commented Aug 28, 2024

For some reason, a test was hang for 14 hour. I just canceled it.

@@ -40,7 +40,8 @@ - (CBLDictionary*) predict: (CBLDictionary*)input {
NSString* inputWord = [input stringForKey: @"word"];

if (!inputWord) {
NSLog(@"No word input !!!");
os_log_t log = os_log_create("CouchbaseLite", "OSLogging");
os_log(log, "No word input !!!");
Copy link
Contributor

Choose a reason for hiding this comment

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

For this one we can use NSLog, I guessed or change it to assertion instead of logging.

Copy link
Contributor Author

@velicuvlad velicuvlad Aug 28, 2024

Choose a reason for hiding this comment

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

changed to assert

@@ -45,7 +45,8 @@ - (void) logWithLevel: (CBLLogLevel)level domain: (CBLLogDomain)domain message:

NSString* levelName = CBLLog_GetLevelName(level);
NSString* domainName = CBLLog_GetDomainName(domain);
NSLog(@"CouchbaseLite %@ %@: %@", domainName, levelName, message);
os_log_t log = os_log_create("CouchbaseLite", "OSDebug");
Copy link
Contributor

Choose a reason for hiding this comment

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

What is "OSDebug"? Where does it appear in the log?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to com.couchbase.lite.ios and the category now represents a domain (CBL)

@@ -115,7 +115,8 @@ to be done in CBLInit()which is called only once when the first CBLDatabase is c
*/
+ (void) initialize {
if (self == [CBLDatabase class]) {
NSLog(@"%@", [CBLVersion userAgent]);
os_log_t log = os_log_create("CouchbaseLite", "OSLogging");
Copy link
Contributor

Choose a reason for hiding this comment

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

What is "OSLogging"? Where does it appear in the log?

@@ -45,7 +45,8 @@ - (void) logWithLevel: (CBLLogLevel)level domain: (CBLLogDomain)domain message:

NSString* levelName = CBLLog_GetLevelName(level);
NSString* domainName = CBLLog_GetDomainName(domain);
NSLog(@"CouchbaseLite %@ %@: %@", domainName, levelName, message);
os_log_t log = os_log_create("CouchbaseLite", "OSDebug");
os_log(log, "CouchbaseLite %@ %@: %@", domainName, levelName, message);
Copy link
Contributor

Choose a reason for hiding this comment

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

There are different types of os_log function based on the log level. I think we should use those functions. What is .NET using?

@velicuvlad velicuvlad requested review from pasin and borrrden September 5, 2024 14:29
Objective-C/CBLDatabase.mm Outdated Show resolved Hide resolved
Objective-C/CBLLog.mm Outdated Show resolved Hide resolved
to be used with any additional in-code logging
@velicuvlad velicuvlad requested a review from pasin September 13, 2024 16:28
Objective-C/CBLConsoleLogger.m Outdated Show resolved Hide resolved
Objective-C/CBLConsoleLogger.m Outdated Show resolved Hide resolved
@velicuvlad velicuvlad requested a review from pasin October 4, 2024 16:38
@@ -32,6 +32,7 @@ - (instancetype) initWithLogLevel: (CBLLogLevel)level {
if (self) {
_level = level;
_domains = kCBLLogDomainAll;
_sysID = [[NSBundle mainBundle] bundleIdentifier];
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this is right as it think it will return the bundle identifier of the app not the framework. What is the value when you run the test? If this returns bundle id of the app, you may try to get the bundle id from one of the classes such as CBLConsoleLogger itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right, will fix

}
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
logger = os_log_create("com.couchbase.lite.ios", "Internal");
Copy link
Contributor

Choose a reason for hiding this comment

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

This one still uses the hardcode string, "com.couchbase.lite.ios".

@velicuvlad
Copy link
Contributor Author

velicuvlad commented Oct 22, 2024

I've changed internalLogger to logAlways which will log under DB domain in the default scope of os_log (hence the name), independently of the log level set by CBL, that is only used only in logWithLevel. So we don't have to use another dispatch. Tested a bit and it works as expected.

@velicuvlad velicuvlad requested a review from pasin October 22, 2024 13:01
@@ -190,7 +191,7 @@ - (instancetype) initWithDefault {
C4LogDomain domain = c4log_getDomain(domainName, true);
C4LogLevel level = string2level(defaults[key]);
c4log_setLevel(domain, level);
NSLog(@"CouchbaseLite logging to %s domain at level %s", domainName, kLevelNames[level]);
os_log(oslogger, "CouchbaseLite logging to %s domain at level %s", domainName, kLevelNames[level]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it use CBLConsoleLogger's logAlways as well?

}

+ (void) logAlways: (NSString*)message {
os_log(osLogDictionary[@(kCBLLogDomainDatabase)], "%@", message);
Copy link
Contributor

@pasin pasin Oct 22, 2024

Choose a reason for hiding this comment

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

Just noticed this. If CBLConsoleLogger is not inited yet, osLogDictionary will be empty as logAlways is a static function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants