[
https://issues.apache.org/jira/browse/CASSANDRA-19632?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17848940#comment-17848940
]
Stefan Miklosovic edited comment on CASSANDRA-19632 at 5/23/24 12:59 PM:
-------------------------------------------------------------------------
I went through all logger.trace in the production code and I modified only 59
files instead of 127 in the first PR.
[https://github.com/apache/cassandra/pull/3329]
The perception I got by going through all of that is that people were already
following the rule of "if it has more than 2 arguments then wrap it in
logger.isTraceEnabled" so I went by that logic as well everywhere where it was
not done like that.
There were also inconsistent usages of logger.trace() with 0 / 1 / 2 arguments.
Sometimes it was wrapped in isTraceEnabled, sometimes it was not, without any
apparent reason. I think that for simple cases it is not necessary to wrap it,
we have majority of cases like that in the code base (not wrapped).
I have also fixed the cases where string concatenation was used and similar.
Not all people also seem to understand that when it is logged like this:
{code:java}
logger.trace("abc {}", object);
{code}
then the actual object.toString() is evaluated _after_ we are absolutely sure
we go to indeed log. I do not think that this is necessary, even "object" is
some "heavyweight" when it comes to toString because it is not called
prematurely anyway.
{code:java}
if (logger.isTraceEnabled())
logger.trace("abc {}", object);
{code}
as per [https://www.slf4j.org/faq.html#string_contents]
{quote}The logging system will invoke complexObject.toString() method only
after it has ascertained that the log statement was enabled. Otherwise, the
cost of complexObject.toString() conversion will be advantageously avoided.
{quote}
was (Author: smiklosovic):
I went through all logger.trace in the production code and I modified only 59
files instead of 127 in the first one.
https://github.com/apache/cassandra/pull/3329
The perception I got by going through all of that is that people were already
following the rule of "it it has more than 2 arguments then wrap it in
logger.isTraceEnabled" so I went by that logic as well everywhere where it was
not done like that.
There were also inconsistent usages of logger.trace() with 0 / 1 / 2 arguments.
Sometimes it was wrapped in isTraceEnabled, sometimes it was not, without any
apparent reason. I think that for simple cases it is not necessary to wrap it,
we have majority of cases like that in the code base (not wrapped).
I have also fixed the cases where string concatenation was used and similar.
Not all people also seem to understand that when it is logged like this:
{code}
logger.trace("abc {}", object);
{code}
then the actual object.toString() is evaluated _after_ we are absolutely sure
we go to indeed log. I do not think that this is necessary, even "object" is
some "heavyweight" when it comes to toString because it is not called
prematurely anyway.
{code}
if (logger.isTraceEnabled())
logger.trace("abc {}", object);
{code}
as per https://www.slf4j.org/faq.html#string_contents
{quote}
The logging system will invoke complexObject.toString() method only after it
has ascertained that the log statement was enabled. Otherwise, the cost of
complexObject.toString() conversion will be advantageously avoided.
{quote}
> wrap tracing logs in isTraceEnabled across the codebase
> -------------------------------------------------------
>
> Key: CASSANDRA-19632
> URL: https://issues.apache.org/jira/browse/CASSANDRA-19632
> Project: Cassandra
> Issue Type: Improvement
> Components: Legacy/Core
> Reporter: Stefan Miklosovic
> Assignee: Stefan Miklosovic
> Priority: Normal
> Fix For: 5.x
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> Our usage of logger.isTraceEnabled across the codebase is inconsistent. This
> would also fix issues similar in e.g. CASSANDRA-19429 as [~rustyrazorblade]
> suggested.
> We should fix this at least in trunk and 5.0 (not critical though) and
> probably come up with a checkstyle rule to prevent not calling isTraceEnabled
> while logging with TRACE level.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
