Logger: Composite / Multiple loggers logger by nitbharambe · Pull Request #1574 · PowerGridModel/power-grid-model

nitbharambe · 2026-09-07T07:18:47Z

Implementation from POC in pgm/feature/logger-api-poc

mgovers · 2026-09-10T06:19:26Z

+        std::string result;
+        for (auto const& [tag, value] : data_) {
+            // Each line has format: EVENT_CODE\tVALUE
+            result += std::format("{}\t{}\n", std::to_underlying(tag), value);
+        }
+        return result;


please use std::stringstream or similar. std::string is not built for this kind of repeated appending in a loop

See how it's done in the TextLogger for reference.

mgovers · 2026-09-10T06:27:35Z

+    template <typename... Args> void log_all(Args&&... args) {
+        for (auto& child : children_) {
+            child->log(std::forward<Args>(args)...);


you can't forward the same object multiple times. please add a test case that this is not accidentally done. i'd have expected sonar to warn about this

An additional side note: Since we have some strong conventions about perfect forwarding, let's add a comment here for reference in the future. This cases do lay in one of the valid use cases: we don't care what Args... are nor about the qualification, we just pass them around. Same below.

Slipped my mind. The only thing which is stopping from forwarding multiple times is that none of the downstream logs have a rvalue overload.
I dont see a use case for when we would like to forward instead of const&. Hence restricting this.

figueroa1395

Partial review. I'll continue later

figueroa1395 · 2026-09-10T07:18:29Z

+    // Clear accumulated output. Default: no-op.
+    virtual void clear() {}


Why is the default no-op? Shouldn't the default just be to clear the underlying logged data?

Default on any logger can ideally be a no-op.
Currently this path gets used in NoMultiThreadedLogger and MultiThreadedCompositeLogger. Hence defaulted instead of pure virtual.

figueroa1395 · 2026-09-10T07:23:41Z

+
+    // The function is called exactly once with a string_view valid only for the duration of the call.
+    // Default: no op / delivers an empty view
+    virtual void get_output(std::function<void(std::string_view)> const& callback) const { callback({}); }


Is this similar to flush() in the TextLogger? Or is their purpose different now?

I see get_output takes the callback as an argument, whereas flush takes the callback via the TextLogger constructor. It feels to me that both are attempting very similar things and only one should remain.

That said, taking it as an argument is a lot more flexible and perhaps aligns best with the C-API. So maybe flush can be removed?

Thoughts?

Well, flush gets the data-> passes to flush handler -> clears the data.
And get_output only gets the data and passes to callback without clearing.

figueroa1395 · 2026-09-10T07:32:49Z

+        std::string result;
+        for (auto const& [tag, value] : data_) {
+            // Each line has format: EVENT_CODE\tVALUE
+            result += std::format("{}\t{}\n", std::to_underlying(tag), value);
+        }
+        return result;


See how it's done in the TextLogger for reference.

figueroa1395 · 2026-09-10T07:38:40Z

+
+  protected:
+    std::string snapshot_locked() const override { return get().string_report(); }
+    void clear_locked() override { get().clear(); }


A couple of questions:

Why is clear_locked protected? It should be accessible by "everyone" now, right? Edit: I see now, CRTP, right?

Why not just name it clear directly? The user would directly get this overload unless they explicitly cast the type to get the underlying clear. Also, this avoid potential naming confusion. Edit: Due to CRTP the way to access it is then via clear, as expected. This is just like clear_impl, right?

Same questions from above but for TextLogger.

(Renamed locked to thread_unsafe_impl as suggested by martijn)

Yes, for CRTP. we can make class friend / mark specific places protected. I chose later.
We do need separate handling in multithreaded logger to implement thread safe operations hence they were routed this way.

figueroa1395 · 2026-09-10T07:48:07Z

    void flush() { get().flush(); }
+
+  protected:
+    std::string snapshot_locked() const override { return get().report(); }


Can this be made more efficient if you just get the "raw" data and the turn into a "string" or whatever you may need at the multi threaded logger side? Same for calculation info.

I mention this because I believe this may copy the data twice, which can get expensive easily.

figueroa1395 · 2026-09-10T07:51:13Z

+        std::string snapshot;
+        {
+            std::lock_guard const lock{mutex_};
+            snapshot = snapshot_locked();


I think this is an extra copy made. Maybe just passing around string_views is fine and converting it once to string at the caller fn point below is sufficient?

Also, since this involves a callback which may throw, it may be a good idea to do Lippincot pattern or similar like in flush for the TextLogger such that we handle exceptions or at least we propagate to one that points towards hey, something is wrong with your callback, can't do anything.

figueroa1395 · 2026-09-10T07:59:38Z

+    void log(LogEvent tag, double value) override             { log_all(tag, value); }
+    void log(LogEvent tag, Idx value) override                { log_all(tag, value); }
+
+    using Logger::log;


Can this be placed in log_all? I believe it's only relevant there and it may lead to confusion later if we add another member function with log in the "wrong" place and unexpected behaviour triggers.

We need it because we dont define all overloads of log function

figueroa1395 · 2026-09-10T13:02:51Z

+// Dedupe: registering the same logger twice is a no-op (idempotent, consistent with logging conventions).
+// UB: modifying the logger list while a calculation is in progress.


Since this logger is what will be shared, let's make sure to have these two things explicit in the documentation.

This MultiThreadedCompositeLogger is not shared anywhere. Its owned fully by the handle. I guess this would be visible in C API PR.
Things inside it would have shared ownership

figueroa1395 · 2026-09-10T13:06:26Z

+        }
+        loggers_.push_back(std::move(logger));
+    }
+    void remove(MultiThreadedLogger const* logger) {


Should this also take in a share_ptr instead to keep it consistent? Probably not, but making sure.

Yes, I think not. I suppose that would be redundant. After being passed to remove, the composite logger and C++ core should not need it anymore.

figueroa1395 · 2026-09-10T13:07:48Z

+    }
+    void reset() { loggers_.clear(); }
+
+    std::unique_ptr<Logger> create_child() override {


We probably want this one and below marked as final to avoid user overriding things and messing them up. Or should we leave that up to them?

As per above comment, composite logger is never available to user. And we dont have a special need to make it explicitly final yet.

figueroa1395 · 2026-09-10T13:08:51Z

+    using MultiThreadedLogger::log;
+
+    // Fan out clear() to every registered logger.
+    void clear() override {


What's the difference in behaviour between reset and clear? Do we need both?

Reset which is a MultiThreadedCompositeLogger method, and it empties/clears the loggers registered in composite logger. Its loggers_.clear() where loggers_ is a vector here.
vs
clear is a generic Logger method applicable for all logger types. Its for means emptying / clearing output.
For MultiThreadedCompositeLogger, its clears all the child loggers inside the composite logger.

We need 2 distinct methods in the end.

figueroa1395 · 2026-09-10T13:13:40Z

+    template <typename... Args> void log_all(Args&&... args) {
+        for (auto& child : children_) {
+            child->log(std::forward<Args>(args)...);


An additional side note: Since we have some strong conventions about perfect forwarding, let's add a comment here for reference in the future. This cases do lay in one of the valid use cases: we don't care what Args... are nor about the qualification, we just pass them around. Same below.

figueroa1395 · 2026-09-10T13:15:16Z

It can become a bit obscure how the chain of logger, multithreadedlogger, compositelogger, multithreadedcompositelogger works, specially considering that after come the actual implementations. Can you add a brief description somewhere here explaining the flow a bit, otherwise in the tests.

figueroa1395 · 2026-09-10T13:16:59Z

+    }
+    void reset() { loggers_.clear(); }
+
+    std::unique_ptr<Logger> create_child() override {


Do we want to leave the user have this control? I make for C-API users it makes sense, but Python users and Cpp users (?) shouldn't need to, right?

Answered above. No need to expose it to user.

figueroa1395 · 2026-09-10T13:18:38Z

+// its own reference. This is what makes destroying the wrapper while still registered safe.
+// Dedupe: registering the same logger twice is a no-op (idempotent, consistent with logging conventions).
+// UB: modifying the logger list while a calculation is in progress.
+class MultiThreadedCompositeLogger : public MultiThreadedLogger {


I'm missing some reporting functionality at this stage, since the loggers will be under an abstraction, would reporting work directly via multithreaded? don't you need some overload where users can select from which or all loggers to report?

composite logger would not be exposed to the users. And indeed its get output would not be called. Its supposed to be "write only". The purpose of it is internal logging management.

It would be a bit complicated to have a "report all" functionality and we dont see a use case for it.

figueroa1395 · 2026-09-10T13:21:39Z

I'm missing tests in which a "custom" logger inherits from MultiThreadedCompositeLogger. Also the get_output functionality with a custom callback must be tested (you can get inspiration from the TextLogger tests.

mgovers · 2026-09-11T06:48:41Z


    LoggerType log_;
-    std::mutex mutex_;
+    mutable std::mutex mutex_;


why mutable?

To make get_output const.
I see this seems like an acceptable way as described in the article suggested by https://github.com/PowerGridModel/power-grid-model/pull/1574/changes#r3976369778

nitbharambe · 2026-09-16T12:21:08Z

+        }();
+        fn(snapshot);
+    }
+    void clear() final {


Thinking about this situation makes me think if we need special handling for multi threaded text logger and flushing. Will explore later

Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>

sonarqubecloud · 2026-09-18T11:54:47Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
82.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

nitbharambe changed the title ~~Multiple loggers logger~~ Composite / Multiple loggers logger Sep 7, 2026

nitbharambe added the feature New feature or request label Sep 7, 2026

figueroa1395 assigned nitbharambe Sep 9, 2026

nitbharambe marked this pull request as ready for review September 9, 2026 14:07

nitbharambe added improvement Improvement on internal implementation and removed feature New feature or request labels Sep 9, 2026

mgovers reviewed Sep 10, 2026

View reviewed changes

figueroa1395 reviewed Sep 10, 2026

View reviewed changes

mgovers reviewed Sep 11, 2026

View reviewed changes

Comment thread power_grid_model_c/power_grid_model/include/power_grid_model/common/multi_threaded_logging.hpp Outdated

mgovers reviewed Sep 11, 2026

View reviewed changes

Comment thread power_grid_model_c/power_grid_model/include/power_grid_model/common/multi_threaded_logging.hpp

mgovers reviewed Sep 11, 2026

View reviewed changes

nitbharambe commented Sep 16, 2026

View reviewed changes

nitbharambe changed the title ~~Composite / Multiple loggers logger~~ Logger: Composite / Multiple loggers logger Sep 16, 2026

nitbharambe added this pull request to stack #1592 September 18, 2026 11:27

nitbharambe added 8 commits September 18, 2026 13:29

start from poc

9bb8ef0

Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>

make mutex mutable

76a0374

Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>

address comments

b6ce514

Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>

address comments forwarding

37f37f6

Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>

address comments sonar + others

3fd603d

Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>

clang tidy

5e62c35

Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>

add get output test

8d55f7a

Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>

address sonar

c9dee9a

Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>

nitbharambe force-pushed the pgm/feature/multiple-loggers branch from bc7d190 to c9dee9a Compare September 18, 2026 11:29

		// Clear accumulated output. Default: no-op.
		virtual void clear() {}

		// Dedupe: registering the same logger twice is a no-op (idempotent, consistent with logging conventions).
		// UB: modifying the logger list while a calculation is in progress.

Conversation

nitbharambe commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgovers Sep 10, 2026 • 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!

mgovers Sep 10, 2026 • 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!

figueroa1395 left a comment

Choose a reason for hiding this comment

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!

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!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nitbharambe commented Sep 7, 2026 •

edited

Loading

mgovers Sep 10, 2026 •

edited

Loading

mgovers Sep 10, 2026 •

edited

Loading