New logging API #2194

snej · 2024-12-16T21:27:38Z

API:

Implemented new C4LogObserver API as described in the design doc.
It was necessary to change some includes: c4Base.h no longer includes c4Log.h, it's now the other way around. Platform code may need to add some explicit #include c4log.h statements.
The existing API for logging callbacks and files still exists, but should eventually be deprecated / removed.
There is no logging by default: something has to register an observer. c4log_initConsole() adds an observer that logs to the console. CppTests and C4Tests call it at startup.

Implementation:

Lots of refactoring of logging code. New class LogObserver is used to receive log messages.
File and callback logging are implemented as subclasses LogFiles and LogCallback.

Review notes:

The first three commits are just some preparations, moving the c4log code and moving the logging files to a new dir.
"Implemented LogObserver. Refactored logging code." is the internal implementation, adding the LogObserver class and moving the file and callback logging code into implementations of it.
"Implemented C4LogObserver [API]" implements the C API for LogObserver.
"Heavily refactored LogFiles" is where I succumbed to the unnecessary task of cleaning up the file logging code. I merged the three parallel arrays (of file streams, log encoders and rotation counts) into an array of a new LogFile class, and most of the logic went into the new class.
"Reimplemented old c4log API using new one" simplifies the implementation of the old C file/callback log APIs by calling the C LogObserver APIs.
"c4Tests' logging stub should preserve the domain" just fixes a small mistake in the C4Tests logging glue: it should preserve the log domain instead of routing the logs to the default logger. This also eliminates the lint warning that the method could be made static.

cbl-bot · 2024-12-18T21:14:20Z

Code Coverage Results:

Type	Percentage
branches	66.32
functions	78.59
instantiations	70.81
lines	77.61
regions	73.38

- c4Log.h now includes c4Base.h, not the other way around (this is because the new log API requires C4Timestamp) - Added includes to sources files that now need it - Moved c4Log implementation out of c4Base.cc to new c4Log.cc

Old API is still present but "semi-deprecated".

The code got a lot cleaner when I replaced the three parallel arrays with a class `LogFile`.

The logging code no longer has to create and tear down a stringstream every time.

It was possible for the timestamps to differ by more than the last 3 digits, if the timestamp inside the file was written just after a new second started; i.e. the filename was constructed at time 12345678999 and the first log was written at 12345679001.

Apparently on Windows "%p" does not print a "0x" before the address.

snej · 2025-01-13T18:13:53Z

The Windows build failed in Jenkins, but I can't find an error anywhere in the full build log -- just thousands of linker warnings. Any idea, @borrrden ? It just ends with:

...
[2025-01-10T21:45:17.734Z] LINK : warning LNK4286: symbol 'kC4Cpp_DefaultLog' defined in 'DefaultLogger.obj' is imported by 'ThreadedMailbox.obj' [C:\Jenkins\workspace\line_couchbase-lite-core_PR-2194\couchbase-lite-core\build_cmake\x64\LiteCore\tests\CppTests.vcxproj]
[2025-01-10T21:45:19.287Z]   CppTests.vcxproj -> C:\Jenkins\workspace\line_couchbase-lite-core_PR-2194\couchbase-lite-core\build_cmake\x64\LiteCore\tests\Debug\CppTests.exe
[2025-01-10T21:45:19.287Z]   Building Custom Rule C:/Jenkins/workspace/line_couchbase-lite-core_PR-2194/couchbase-lite-core/CMakeLists.txt
[2025-01-10T21:45:20.007Z] C++ tests failed!

jianminzhao · 2025-01-13T20:32:21Z

C/c4Log.cc

+}
+
+void c4log_removeObserver(C4LogObserver* observer) noexcept {
+    LogObserver::remove(reinterpret_cast<LogObserver*>(observer));


Isn't the reinterpret_cast "toInternal?"

You're right, it could be changed to a call to toInternal().

jianminzhao · 2025-01-13T22:11:01Z

LiteCore/Logging/Logging.cc

+#pragma mark - INITIALIZATION:
+
+    LogDomain::LogDomain(const char* name, LogLevel level, bool internName)
+        : _level(level), _name(internName ? strdup(name) : name), _observers(new LogObservers) {


where is "strdup(name)" freed?

It isn't! Log domains are never freed.

Then, we probably don't need the extra argument, internName.

Most log domains are declared as globals and have names that are string constants. Those don't need to be copied. The internName flag is only for domains created dynamically via the C API.

Then, perhaps strdup at the place where C API is called.

snej force-pushed the feature/log-observer branch 7 times, most recently from 5d98e1e to b081c30 Compare December 18, 2024 20:33

snej added 11 commits January 7, 2025 11:41

Fixed initializer-ordering warning in c4ReplicatorImpl.hh

73295b3

c4Log adjustments, prior to adding new API

7a1c4b3

- c4Log.h now includes c4Base.h, not the other way around (this is because the new log API requires C4Timestamp) - Added includes to sources files that now need it - Moved c4Log implementation out of c4Base.cc to new c4Log.cc

Moved logging files to new LiteCore/Logging

9b6e777

Implemented LogObserver. Refactored logging code.

a448f90

Implemented C4LogObserver [API]

bc29f2a

Old API is still present but "semi-deprecated".

Heavily refactored LogFiles

73215cd

The code got a lot cleaner when I replaced the three parallel arrays with a class `LogFile`.

Reimplemented old c4log API using new one

46fb7f1

c4Tests' logging stub should preserve the domain

3b4499e

Added c4log_nextDomain() [API]

4656eb2

Fixed bug setting multiple custom log levels

282c1db

Added LogObserver tests. Fixed bugs.

867525a

snej force-pushed the feature/log-observer branch from 7d5d573 to ba1b80c Compare January 7, 2025 19:54

Made Logging::addLoggingKeyValuePairs more efficient

3a65c31

The logging code no longer has to create and tear down a stringstream every time.

snej force-pushed the feature/log-observer branch from ba1b80c to 3a65c31 Compare January 8, 2025 18:22

snej added 2 commits January 10, 2025 10:02

Fixed LogObserver tests failing on Windows

0b723fc

Apparently on Windows "%p" does not print a "0x" before the address.

snej force-pushed the feature/log-observer branch from e91f1f9 to 0b723fc Compare January 10, 2025 21:20

jianminzhao reviewed Jan 13, 2025

View reviewed changes

Update jenkins_win.ps1 to show exit code

4102a98

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New logging API #2194

New logging API #2194

snej commented Dec 16, 2024 •

edited

Loading

cbl-bot commented Dec 18, 2024 •

edited

Loading

snej commented Jan 13, 2025

jianminzhao Jan 13, 2025

snej Jan 14, 2025

jianminzhao Jan 13, 2025

snej Jan 14, 2025

jianminzhao Jan 14, 2025

snej Jan 14, 2025

jianminzhao Jan 15, 2025

New logging API #2194

Are you sure you want to change the base?

New logging API #2194

Conversation

snej commented Dec 16, 2024 • edited Loading

API:

Implementation:

Review notes:

cbl-bot commented Dec 18, 2024 • edited Loading

snej commented Jan 13, 2025

jianminzhao Jan 13, 2025

Choose a reason for hiding this comment

snej Jan 14, 2025

Choose a reason for hiding this comment

jianminzhao Jan 13, 2025

Choose a reason for hiding this comment

snej Jan 14, 2025

Choose a reason for hiding this comment

jianminzhao Jan 14, 2025

Choose a reason for hiding this comment

snej Jan 14, 2025

Choose a reason for hiding this comment

jianminzhao Jan 15, 2025

Choose a reason for hiding this comment

snej commented Dec 16, 2024 •

edited

Loading

cbl-bot commented Dec 18, 2024 •

edited

Loading