[RFC] core: notify plugins when a log line is emitted

[vincenzopalazzo]

While our logging is super flexible, we are missing a feature to give the power to a plugin to be notified about logging.

This is useful when you are deploying core lightning in production and want a dashboard where it is possible to monitor the log with proper tools like Graphana, or tracing information with a framework like open telemetry

[cdecker]

Apologies for reviewing a draft, but the PR caught my attention, since it's something I have been looking into as well.

The choice of augmenting the warning notification seems a bit strange, shouldn't the log topic contain all logs, and warning logs be replicated. We should try to avoid coupling semantics of different things together.

One major issue with the log notification is the fact that it trivially produces infinite recursions: print a log line while handling a log notification and voila, you just caused a deadlock in the form of an infinite recursion.

We should track the origin of logs, and not deliver logs originating from log subscribers to other log subscribers. That even fixes the problem of two plugins playing seesaw, by seeing each other's log messages produced during log processing :-)

[cdecker]

Also, this is a draft, so please do not attach a milestone to it. Milestones are for the release captain to signal their intent to include a PR in a release, not for maintainers and developers to just tag and throw things over the wall.

[vincenzopalazzo]

Also, this is a draft, so please do not attach a milestone to it. Milestones are for the release captain to signal their intent to include a PR in a release

I forgot to turn on this, I was fighting with the CI and did not realize that the last push was finding the CI. So I guess this is ready to be review and I would love that you consider the inclusion inside this release if possible 😄

[vincenzopalazzo]

One major issue with the log notification is the fact that it trivially produces infinite recursions: print a log line while handling a log notification and voila, you just caused a deadlock in the form of an infinite recursion.

Yes but this is true also for hooks, we can write the hook in a way that will create a loop.

The idea here for this PoC is that the log notification needs to be specified by the user so the user should be able to not log in to this function. An example that I am thinking of is a programmer who writes an infinite recursion.

OFC I can add a /* FIXME: ... */ there and we can try to implement your tracking call proposal?

[cdecker]

I don't think that'll be necessary. These deadlocks should be pretty evident, and we can add the extra warning later.

I'd still like the warnings to also end up in the log topic, so please decouple log and warning notifications and then I'm very happy with this PR 👍

[vincenzopalazzo]

I'd still like the warnings to also end up in the log topic, so please decouple log and warning notifications and then I'm very happy with this PR 👍

Thank you. I would like to clarify that the warning should be replicated with the previous body, and a new log notification should also be emitted for the warning log, right?

[cdecker]

Please decouple the log from the warning topic. log should contain every log event, while warning should receive only a subset of the logs. We can add deadlock-protection in a separate step.

[vincenzopalazzo]

Ok @cdecker this should be ready for another review, sorry for the delay

[vincenzopalazzo]

Review addressed @cdecker

Rebased to resolve some conflicts too!

[vincenzopalazzo]

Trivial rebase to resolve the common/Makefile conflicts

[vincenzopalazzo]

Trivial Rebase and I think this is ready for considering the inclusion

I don't think that'll be necessary. These deadlocks should be pretty evident, and we can add the extra warning later.

I am using this assumption and trying to fix the recursion later, for the simple fact that we have these recursion problems also for hooks, so we should design a detect loop solution that will work also for hooks, but IMHO this is unrelated to this PR