>
> I do not agree we should remove isDebugEnabled tests, as debug logging
> should be possible to turn off and avoid any penalty.
>
if (logger.isDebugEnabled())
logger.debug("hello")
is performance-wise same as doing
logger.debug("hello")
we already went through this, the latter version internally checks what
level it is on so us wrapping it in additional "if" does not make any
difference.
The only reasonable use-case I see to justify the wrapping into
isDebugEnabled() is if the subsequent logger.debug logs something which
takes considerable time / memory to construct.
> However, it is not very important so I will not fight that battle. I will
> however fight any attempt to codify weird rules for logging levels.
>
> On 23 Jul 2024, at 14:26, J. D. Jordan <jeremiah.jor...@gmail.com> wrote:
>
> I don’t know that I agree we should remove use if isDebugEnabled, but we
> should be assuming debug is enabled and not doing anything too crazy there.
> The description of the log levels from the old wiki describes the state of
> our logging very well, +1 to get that back into the docs.
> If someone wants to propose and implement a new logging scheme going
> forward that would be a different thread. We should make sure the docs and
> recommendations match the current state of the code.
>
> -Jeremiah
>
> On Jul 23, 2024, at 8:06 AM, Štefan Miklošovič <smikloso...@apache.org>
> wrote:
>
>
> For the record, I am OK with removing logger.isDebugEnabled() in the same
> spirit as it was done with logger.isTracingEnabled(). That is, some
> invocations of isDebugEnabled() are not necessary, logger.debug() is just
> enough, I already went through the reasons why it is so in the first email
> from which the decision to remove isTracingEnabled when it was not a heavy
> operation was made.
>
> The primary reason for not removing isDebugEnabled was just my hesitation
> to introduce any big diff but if we all think it is OK now to remove
> isDebugEnabled in trunk I am fine with that.
>
> I am not completely sure how to respond to all other stuff though so I'll
> skip that and see where this goes.
>
> On Tue, Jul 23, 2024 at 1:36 PM Mick Semb Wever <m...@apache.org> wrote:
>
>> 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.
>>
>>
>>
>>
>
