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

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

No description provided.

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

sonarqubecloud · 2026-09-09T14:31:28Z

Quality Gate failed

Failed conditions
65.8% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

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.

figueroa1395

Partial review. I'll continue later

figueroa1395 · 2026-09-10T06:59:40Z


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


This was initially strange for me, but it makes sense. See this Herb Sutter article for a nice explanation.

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?

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?

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.

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.

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.

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.

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?

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?

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?

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?

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.

start from poc

ff29eda

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

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

make mutex mutable

ecc99da

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

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

		// 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.

Uh oh!

Conversation

nitbharambe commented Sep 7, 2026

Uh oh!

sonarqubecloud Bot commented Sep 9, 2026

Quality Gate failed

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!

figueroa1395 left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mgovers Sep 10, 2026 •

edited

Loading

mgovers Sep 10, 2026 •

edited

Loading