[ 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:57 PM: ------------------------------------------------------------------------- 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} 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} > 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: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org