Integration with `quill` logging library by antonio2368 · Pull Request #77107 · ClickHouse/ClickHouse

antonio2368 · 2025-03-04T13:03:10Z

Changelog category (leave one):

Not for changelog (changelog entry is not required)

https://github.com/odygrd/quill

There are some things still missing like syslog but most of the functionalities should be supported as before.

Documentation entry for user-facing changes

Documentation is written (mandatory for new features)

clickhouse-gh · 2025-03-04T13:03:37Z

Workflow [PR], commit [5394230]

antonio2368 · 2025-03-04T15:00:31Z

@maxknv why aren't my changes in ci/jobs/fast_test.py picked up?

maxknv · 2025-03-04T18:37:06Z

@maxknv why aren't my changes in ci/jobs/fast_test.py picked up?

because of fasttest's build script design. if module is required for fasttest build you need to add it into this array: https://github.com/ClickHouse/ClickHouse/blob/master/tests/docker_scripts/fasttest_runner.sh#L122

base/poco/Foundation/include/Poco/Logger.h

Algunenano

Leaving a few comments now but I'll continue reviewing

programs/disks/DisksApp.cpp

src/Common/QuillLogger.h

Algunenano · 2025-03-21T14:24:07Z

contrib/quill-cmake/CMakeLists.txt

+        ${LIBRARY_DIR}/include/quill/backend/TransitEventBuffer.h
+        ${LIBRARY_DIR}/include/quill/backend/BackendUtilities.h
+
+        ${LIBRARY_DIR}/include/quill/bundled/fmt/args.h


I wonder if this won't cause issues with our own bundled fmt if/when the versions don't match exactly

Quill bundles libfmt under a custom namespace and uses its header-only version internally.

There shouldn't be any issues if you're using a different libfmt external version. The only potential for ABI incompatibilities arises if you reuse existing fmt formatters for custom types with Quill (see external fmt formatter specializations).

However, I didn’t see that being used in this PR, so you should be fine.

We use custom formatters that AFAIK we format everything before passing it to Quill (at least we do for other loggers). But maybe @antonio2368 can confirm this is still the case.

fmt is still used in background thread regardless where we do the main formatting.

src/Common/QuillLogger.h

Algunenano · 2025-03-21T14:50:04Z

src/Common/SystemLogBase.cpp

 {
    queue.reserve(settings.reserved_size_rows);

-    if (settings.turn_off_logger)


Why? Is this missing / removed?

Algunenano

Some minor stuff. Still need to review some large diffs (the logger changes themselves 😄), understand the cmake changes and the tests changes too

src/Common/Config/ConfigProcessor.cpp

src/Daemon/BaseDaemon.cpp

src/Loggers/OwnPatternFormatter.h

src/Storages/MergeTree/MergeTreeData.cpp

src/Storages/RocksDB/StorageEmbeddedRocksDB.cpp

Algunenano · 2025-03-22T19:10:32Z

src/Common/Logger.h

+/// This wrapper helps to avoid too frequent and noisy log messages.
+/// For each pair (logger_name, format_string) it remembers when such a message was logged the last time.
+/// The message will not be logged again if less than min_interval_s seconds passed since the previously logged message.
+class LogFrequencyLimiterImpl


Maybe we can move this and LogSeriesLimiter to a different file to remove deps from Logger.h? I know it was in a bad place already.

Algunenano

Final batch of comments. Great job

base/poco/Foundation/include/Poco/Logger.h

Algunenano · 2025-03-24T17:23:00Z

src/Common/Logger.h

+LoggerPtr getLogger(std::string name, LoggerComponent component = LoggerComponent::Root);
+
+template <size_t n>
+LoggerPtr getLogger(const char (&name)[n])


Maybe we can put getLogger into Logger_fwd.h since it seems it's what most includes for Common/Logger.h are for. That way we avoid including some heavy headers

src/Common/Logger.cpp

src/Common/ThreadPool.cpp

Algunenano · 2025-03-24T17:50:32Z

src/Common/logger_useful.h

+                __LINE__,                                                                                                                  \
+                _format_string,                                                                                                            \
+                _format_string_args);                                                                                                      \
+            DB::ExtendedLogMessage _msg_ext = DB::ExtendedLogMessage::getFrom(_poco_message);                                              \


_msg_ext should be built after _should_log so we don't build it unnecessarily under LogFrequency or LogSeries

We need it for text log also later

Algunenano · 2025-03-24T18:27:05Z

src/Loggers/Loggers.cpp

+
    auto current_logger = config.getString("logger", "");
-    if (config_logger.has_value() && *config_logger == current_logger)
+    if (config_logger.has_value())// && *config_logger == current_logger)


This seems odd and maybe a leftover. Does this mean we can't change the config of the logger at runtime and, for example, change the log level?

This was a hack, so I added initialized flag.
We can't change loggers at runtime, we can only change levels but this is handled by function updateLevels below.

Algunenano · 2025-03-24T18:27:49Z

src/Loggers/Loggers.cpp

+        error_log_file = std::static_pointer_cast<DB::RotatingFileSink>(sink);
    }

+    // if (config.getBool("logger.use_syslog", false))


I think it's ok to delete this code and drop syslog support as part of this PR.

Algunenano · 2025-03-24T18:28:38Z

src/Loggers/Loggers.cpp

-            }
-        }
-    }
+    // // Set level and channel to all already created loggers


Deleted code

Algunenano · 2025-03-24T18:29:04Z

src/Loggers/Loggers.cpp

-            }
-        }
-    }
+    getRootLogger()->setLogLevel(min_log_level);


More deleted code

Algunenano · 2025-03-24T18:30:13Z

src/Loggers/TextLogSink.cpp

+    #ifndef WITHOUT_TEXT_LOG
+        auto logs_queue = CurrentThread::getInternalTextLogsQueue();
+
+        /// Log to "TCP queue" if message is not too noisy


What's TCP queue? Is it for client logs?

Guessing this comes and replaces OwnSplitChannel

clickhouse-gh · 2025-06-10T13:18:32Z

Dear @antonio2368, this PR hasn't been updated for a while. Will you continue working on it? If not, please close it. Otherwise, ignore this message.

clickhouse-gh · 2025-08-19T13:16:59Z

Dear @antonio2368, this PR hasn't been updated for a while. Will you continue working on it? If not, please close it. Otherwise, ignore this message.

clickhouse-gh · 2025-08-26T13:19:39Z

Dear @antonio2368, this PR hasn't been updated for a while. Will you continue working on it? If not, please close it. Otherwise, ignore this message.

alexey-milovidov · 2025-10-11T10:39:40Z

Not relevant after @Algunenano improved the logger.

antonio2368 added 9 commits February 26, 2025 18:02

Add quill

102b24a

Update quill

66af4bf

Better includes

1bcf7e5

Merge branch 'master' into logging-quill

96e0d22

Add text log support

a94bf2b

Add RotatingSink

1bf122c

Bring back special loggers

bfce5cb

Build loggers from config

bb02060

Merge branch 'master' into logging-quill

e661180

clickhouse-gh bot added pr-not-for-changelog This PR should not be mentioned in the changelog submodule changed At least one submodule changed in this PR. labels Mar 4, 2025

antonio2368 added 3 commits March 4, 2025 15:45

Fix

20f0d31

Revert change

eac6ba8

Fix linking

f9236de

Use TextLogSink

0046313

azat reviewed Mar 5, 2025

View reviewed changes

base/poco/Foundation/include/Poco/Logger.h Show resolved Hide resolved

azat self-assigned this Mar 5, 2025

antonio2368 force-pushed the logging-quill branch from 49c8353 to a77707c Compare March 5, 2025 09:42

Fix fast test

76c07c7

antonio2368 force-pushed the logging-quill branch from a77707c to 76c07c7 Compare March 5, 2025 09:51

antonio2368 added 5 commits March 5, 2025 15:42

Fix

c3f10d5

Merge branch 'master' into logging-quill

26acc10

Fix NATS

74f9dc1

Fix builds

5264c60

Fix some tests

f0046a8

antonio2368 added the pr-performance-tests label Mar 6, 2025

Fix linking

c6bdadc

antonio2368 force-pushed the logging-quill branch from cbafe29 to c0f77b4 Compare March 20, 2025 15:54

antonio2368 and others added 2 commits March 20, 2025 18:46

Fix console sink update

92db875

Even more fixes

e4ea278

Algunenano self-assigned this Mar 21, 2025

Algunenano reviewed Mar 21, 2025

View reviewed changes

antonio2368 added 3 commits March 21, 2025 16:02

Update quill

c81afc9

Merge branch 'master' into logging-quill

936d368

Limit amount of memory used

6e9ca99

Algunenano reviewed Mar 22, 2025

View reviewed changes

Algunenano reviewed Mar 24, 2025

View reviewed changes

antonio2368 and others added 7 commits April 1, 2025 12:13

Merge branch 'master' into logging-quill

a5d69e2

Update quill

2d7abc7

Fix builds

8c3717e

Another improvements

1f64b2d

More improvements

781fa34

Fix tidy

00efe2c

Fix tidy again

692cd2a

antonio2368 force-pushed the logging-quill branch from ef7daba to 3ccdeea Compare April 4, 2025 09:32

Fix all sans

8eb58a5

antonio2368 force-pushed the logging-quill branch from 3ccdeea to 8eb58a5 Compare April 4, 2025 09:38

antonio2368 added 2 commits April 4, 2025 12:39

Merge branch 'master' into logging-quill

72807ab

Fix build

5394230

maxknv removed the pr-performance-tests label Apr 10, 2025

Algunenano mentioned this pull request May 12, 2025

Move logging to a separate thread #80125

Merged

Merge branch 'master' into logging-quill

a7978c0

alexey-milovidov closed this Oct 11, 2025

Conversation

antonio2368 commented Mar 4, 2025

Changelog category (leave one):

Documentation entry for user-facing changes

Uh oh!

clickhouse-gh bot commented Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antonio2368 commented Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maxknv commented Mar 4, 2025

Uh oh!

Uh oh!

Algunenano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odygrd Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Algunenano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Algunenano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clickhouse-gh bot commented Jun 10, 2025

Uh oh!

clickhouse-gh bot commented Aug 19, 2025

Uh oh!

clickhouse-gh bot commented Aug 26, 2025

Uh oh!

alexey-milovidov commented Oct 11, 2025

Uh oh!

clickhouse-gh bot commented Mar 4, 2025 •

edited

Loading

antonio2368 commented Mar 4, 2025 •

edited

Loading

odygrd Mar 21, 2025 •

edited

Loading