[Discussion] Log format of per component log

[ranshid]

Currently we have no strictly defined logging format for different program components (eg replication, cluster-bus, scripting etc...).
In #999 we introduced a unified log format for all Dual-channel related logging which follows the modules semantics, so that every log message is prefixed by '< Dual Channel >'.
Following this there are some concerns that this might be interpreted as if there is some new loadable module (which is not true in case of dual-channel component).

This issue was opened in order to host the discussion of how to format per-component logging.

[ranshid]

@valkey-io/core-team please comment your opinions

[zuiderkwast]

Those < and > marks were unfortunate. I missed those in the review.

I don't think we should change the legacy log messages too much at this point, but we could add a tag when other log formats are used, like JSON or logfmt. For those formats, we could add a tag or something, outside of the message.

I suggest we remove the < > marks and just use a human readable prefix with a colon:

-<Dual Channel> Psync established after rdb load
+Dual channel replication: Psync established after rdb load

[rjd15372]

I still don't have much experience in looking at server logs for debugging problems in Valkey, but IMO having the component prefix in log messages is always a good thing.
It helps the debugging process, specially if we are only concerned with the log messages of a specific component.

The component prefix should be something that is easy to grep for, and therefore it should be enclosed by some special character. For instance, if < > is already being used for module names, we can use [ ] for component names.

[madolson]

I suggest we remove the < > marks and just use a human readable prefix with a colon:

Worth mentioning this was not backported, so we can still change it.

My opinion

I think since we have a new log format config log-format, we should introduce a new option which is legacy-with-component. Which basically does what Ran was sort of implying, it will generate a log-line like:

<Component name> Blah blah

So that it becomes easier to consistently grep on those well defined component names. I will also say, I don't think that Dual channel should be a component, I think replication is a component. I think the components should also be tied to individual log levels, so that you can change the log level of just one of the components (like replication or cluster) without changing the rest of them.

[PingXie]

Looks like #1022 has been merged and it could give us a way to build the new log in a backward compatible way.

[ranshid]

So to summarize the current discussion:

1. It seems we prefer to avoid any kind of new "local" format for legacy log messages ATM.
2. We will have to revert the DualChannel logging and replace the < > marks with just a human readable prefix with a colon.
3. We would probably want to consider supporting component loggers which can be dynamically controlled via assigned debug-level

4. Looks like Configurable log and timestamp formats (logfmt, ISO8601) #1022 has been merged and it could give us a way to build the new log in a backward compatible way.

I think this feature is currently not providing a "component" level granularity, but only general application role (replica/AOF/sentinal). We could change the role to indicate a component, but would require us to change/add logging API which will accept component name and set it as a role or we could extend the role with a component (eg replica:networking etc...)

@valkey-io/core-team I think 3,4 are something we should probably discuss separately. Do we all agree on 1+2?

[enjoy-binbin]

i agree on 1+2

[hwware]

I agree with 1+2 as well.

[PingXie]

aligned on 1+2.

3. We would probably want to consider supporting component loggers which can be dynamically controlled via assigned debug-level

I like this idea. I think it will be very useful for debugging.

4. I think this feature is currently not providing a "component" level granularity, but only general application role (replica/AOF/sentinal). We could change the role to indicate a component, but would require us to change/add logging API which will accept component name and set it as a role or we could extend the role with a component (eg replica:networking etc...)

Yeah, my point is that we can/should extend #1022 with the component.