I disagree with all of this. Debug logging can be enabled for production clusters, but it should not be on by default.
DEBUG should not be taken to mean “background activities” and we should stamp this out wherever this has been occurring. DEBUG means verbose logging for use debugging system behaviour. It should be safe to enable in production, but it should not be on by default. If you want to siphon some log messages to one log file, and others to another, we should be using some explicit mechanism such as a separate logger hierarchy, and not the log level. > On 23 Jul 2024, at 12:35, Mick Semb Wever <m...@apache.org> wrote: > > reply below… > > > On Thu, 30 May 2024 at 14:34, Štefan Miklošovič <stefan.mikloso...@gmail.com > <mailto: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. > > >
