reply below…
On Thu, 30 May 2024 at 14:34, Štefan Miklošovič <stefan.mikloso...@gmail.com> wrote: > I see the feedback is overall positive. I will merge that and I will > improve the documentation on the website along with what Benedict suggested. > I think the codestyle needs to be more explicit that this applies _only_ to trace statements, and also take the opportunity to provide more context to our unusual use of log levels and its implications. tl;dr For the debug log level I would like to propose we: 1. codestyle should mention that: debug doesn't mean debug to us, it is for background activities intended to be enabled in production, 2. doc/comment in logback.xml that the debug logger should not be disabled 3. codestyle against (and remove all) isDebugEnabled long format… 1. Our debug logging is not "debug" but the "non read/write path logging". We hijacked the debug loglevel for the separation of all background activities. This trips most newcomers up. We can do a better job of documenting this (see (2). system.log == read/write path. debug.log == also background activities. There's some nuance here – there is logging around background activities that we do want in system.log, which is correspondingly at the info loglevel. But generally, on the read/write path, we want to use the trace level when one typically thinks about "debug", and on the background paths, use debug when thinking about "info". More history on this in CASSANDRA-10241 and in https://lists.apache.org/thread/mgmbmdw4cp4rq684wcllfjc3sd1sbhkm This separation of log files between foreground and background activity *is* very valuable to the operator (and expert). We have a lot of experience on this. And we did have some good docs about logging here: https://cwiki.apache.org/confluence/display/CASSANDRA2/LoggingGuidelines This should be revived IMHO. 2. We often see operators in production disable the debug loglevel (or appender), thinking they are doing the right thing. We are then left unable to investigate issues related to background tasks (compaction, streaming, repairs, etc), and have to ask them to enable it again before collecting the logs again. Aside: we do tell operators to disable either the file appenders or stdout, as having both running is unnecessary. When they do this they often then go and make the mistake of also disabling the ASYNCDEBUGLOG. There's been calls in the past to better name our log files. I do favour the renaming system.log to foreground.log, and debug.log to everything.log, or anything along those lines. In the referenced ML thread there was also discussion about using a marker, so to remove foreground activities from debug.log, in which case a name like background.log would make more sense. But renaming log files is a compatibility break, and is a separate discussion. 3. The use of isDebugEnabled should be avoided, as it confuses - heavy methods calls in debug cannot be avoid - that debug is always enabled (just like assertions) - debug isn't appropriate on the read/write path (use trace instead) - info isn't appropriate on the background paths, unless you intentionally want it in system.log Debug should never cause a production performance problem. A past example of this confusion: https://github.com/apache/cassandra/commit/ac77e5e7742548f7c7c25da3923841f59d4b2713 It's my understanding this has already been the intentional practice and precedence for most of us, ~10% of debug calls today are wrapped by isDebugEnabled, compared to ~37% for trace being wrapped with isTraceEnabled. While the use of isDebugEnabled might offer some small value to those that adominantly want to disable the background logging, and see some minimal perf improvement from it, I don't think that argument is popular, or wise, enough against a healthy code style and precedence that is intuitive to its intent. Like our assert statements needing not to cause production performance problems (because like debug we expect it to be enabled in production), best we document our idiosyncrasies rather than let them be tribal knowledge.
