You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Mark Thomas <ma...@apache.org> on 2025/08/19 07:01:51 UTC
Logging clean-up
All,
I stumbled across quite few instances of log messages like this while I
was looking at something for $dayjob:
log.error(sm.getString("naming.bindFailed", e));
It would be lot more useful with the stack trace so I think that really
should be:
log.error(sm.getString("naming.bindFailed"), e);
Unless there are objections, I am going to change any log message with a
level of warning or greater that currently includes e, e.toString(),
e.getMessage() etc in the log message and doesn't log the full stack
trace to log the stack trace. I'll likely remove the exception from the
log message as well to avoid duplicating the text in the logs.
INFO level and is a little different. Those need looking at on a case by
case basis as I can imagine there are some cases where logging the full
stack trace is not necessary.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Logging clean-up
Posted by Mark Thomas <ma...@apache.org>.
On 19/08/2025 10:34, Rémy Maucherat wrote:
> On Tue, Aug 19, 2025 at 9:02 AM Mark Thomas <ma...@apache.org> wrote:
>>
>> All,
>>
>> I stumbled across quite few instances of log messages like this while I
>> was looking at something for $dayjob:
>>
>> log.error(sm.getString("naming.bindFailed", e));
>>
>> It would be lot more useful with the stack trace so I think that really
>> should be:
>>
>> log.error(sm.getString("naming.bindFailed"), e);
>>
>>
>> Unless there are objections, I am going to change any log message with a
>> level of warning or greater that currently includes e, e.toString(),
>> e.getMessage() etc in the log message and doesn't log the full stack
>> trace to log the stack trace. I'll likely remove the exception from the
>> log message as well to avoid duplicating the text in the logs.
>>
>> INFO level and is a little different. Those need looking at on a case by
>> case basis as I can imagine there are some cases where logging the full
>> stack trace is not necessary.
>
> Sometimes the exception message is more than enough though. I think
> the whole thing could be case by case.
I've just committed the changes for error level. Happy to discuss any
you think are unnecessary.
It was a bit of a mixed bag. There were a few where something more
useful than the exception message could be included in the log message
(like the container or the resource where naming failed), some were just
reducing duplication and one wrong log level.
The full stack trace might not be necessary but for error level issues
they don't happen that often and better to have the stack trace than
not. I've occasionally been in the position where I've had a log message
without a stack trace and really wished I'd had the stack trace too but
I've never thought "I wish I didn't have this stack trace".
And sometimes the root cause is helpful.
My expectation is that warning will be similar but info will be where
there are likely to be more cases where the stack trace makes less sense.
This is one of those areas where - once you start poking around you find
all sorts of things you want to tidy up.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Logging clean-up
Posted by Rémy Maucherat <re...@apache.org>.
On Tue, Aug 19, 2025 at 9:02 AM Mark Thomas <ma...@apache.org> wrote:
>
> All,
>
> I stumbled across quite few instances of log messages like this while I
> was looking at something for $dayjob:
>
> log.error(sm.getString("naming.bindFailed", e));
>
> It would be lot more useful with the stack trace so I think that really
> should be:
>
> log.error(sm.getString("naming.bindFailed"), e);
>
>
> Unless there are objections, I am going to change any log message with a
> level of warning or greater that currently includes e, e.toString(),
> e.getMessage() etc in the log message and doesn't log the full stack
> trace to log the stack trace. I'll likely remove the exception from the
> log message as well to avoid duplicating the text in the logs.
>
> INFO level and is a little different. Those need looking at on a case by
> case basis as I can imagine there are some cases where logging the full
> stack trace is not necessary.
Sometimes the exception message is more than enough though. I think
the whole thing could be case by case.
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Logging clean-up
Posted by Mark Thomas <ma...@apache.org>.
On 19/08/2025 17:43, Mark Thomas wrote:
> On 19/08/2025 17:38, Christopher Schultz wrote:
> > On 8/19/25 11:22 AM, Mark Thomas wrote:
>
> <snip/>
>
>>> I've currently got it working on
>>> "catch (Exception <not e>)" to "catch (Exception e)"
>>>
>>> although I want to look at the diff for that one to see how big it is
>>> before deciding whether or not it is worth the noise to commit.
>>>
>>> Depending on how that goes, I might get it to do
>>> "catch (IOException <not ioe>)" to "catch (IOException ioe)"
>>>
>>> After that ... ?
>>>
>>> There is a question of how far to take this and the commits already
>>> made to main might be enough. There is a trade off to make between
>>> consistent code being easier to read and noisy history making it
>>> harder to review changes over time.
>>>
>>> Thoughts?
>>
>> I couldn't care less about noisy history. More readable means more
>> maintainable.
>
> Ack
>
>>> P.S. I will be back-porting the logging changes but I might leave it
>>> until tomorrow in case folks object
>>
>> +1
>>
>> I would wait as well. You may a as well make a bunch of changes to
>> main, let anyone give feedback over a day or so, and then back-port
>> everything to the other branches.
>
> I'll commit what I have so far and I might have some more tomorrow.
> Back-ports likely to wait until Thursday.
I think I am done with the logging changes for now. Back-ports to follow
- probably tomorrow.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Logging clean-up
Posted by Mark Thomas <ma...@apache.org>.
On 19/08/2025 17:38, Christopher Schultz wrote:
> On 8/19/25 11:22 AM, Mark Thomas wrote:
<snip/>
>> I've currently got it working on
>> "catch (Exception <not e>)" to "catch (Exception e)"
>>
>> although I want to look at the diff for that one to see how big it is
>> before deciding whether or not it is worth the noise to commit.
>>
>> Depending on how that goes, I might get it to do
>> "catch (IOException <not ioe>)" to "catch (IOException ioe)"
>>
>> After that ... ?
>>
>> There is a question of how far to take this and the commits already
>> made to main might be enough. There is a trade off to make between
>> consistent code being easier to read and noisy history making it
>> harder to review changes over time.
>>
>> Thoughts?
>
> I couldn't care less about noisy history. More readable means more
> maintainable.
Ack
>> P.S. I will be back-porting the logging changes but I might leave it
>> until tomorrow in case folks object
>
> +1
>
> I would wait as well. You may a as well make a bunch of changes to main,
> let anyone give feedback over a day or so, and then back-port everything
> to the other branches.
I'll commit what I have so far and I might have some more tomorrow.
Back-ports likely to wait until Thursday.
Mark
>
> -chris
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Logging clean-up
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,
On 8/19/25 11:22 AM, Mark Thomas wrote:
> On 19/08/2025 15:52, Christopher Schultz wrote:
>> Mark,
>>
>> On 8/19/25 3:01 AM, Mark Thomas wrote:
>>> All,
>>>
>>> I stumbled across quite few instances of log messages like this while
>>> I was looking at something for $dayjob:
>>>
>>> log.error(sm.getString("naming.bindFailed", e));
>>>
>>> It would be lot more useful with the stack trace so I think that
>>> really should be:
>>>
>>> log.error(sm.getString("naming.bindFailed"), e);
>>
>> I had to read that 3 or 4 times to see that the ) had moved.
>>
>> +1 for this change. If there is an exception being logged, I'm 100% in
>> favor of logging a stack trace. Most logging frameworks can be
>> configured to log stack traces or not. But they can't log (or not) if
>> the exception itself is not present.
>>
>>> Unless there are objections, I am going to change any log message
>>> with a level of warning or greater that currently includes e,
>>> e.toString(), e.getMessage() etc in the log message and doesn't log
>>> the full stack trace to log the stack trace. I'll likely remove the
>>> exception from the log message as well to avoid duplicating the text
>>> in the logs.
>>>
>>> INFO level and is a little different. Those need looking at on a case
>>> by case basis as I can imagine there are some cases where logging the
>>> full stack trace is not necessary.
>>
>> +1
>
> Reviewing log messages you end up looking at a LOT of catch blocks and
> it is apparent that there are multiple naming conventions that have been
> used over time / by different developers.
>
> Since I am in the area I've got Cursor looking into using a consistent
> naming convention. So far, its fixed:
> "catch (Exception t)" to "catch (Exception e)"
> "catch (Throwable e)" to "catch (Throwable t)"
>
> which were the obviously confusing ones.
>
> I've currently got it working on
> "catch (Exception <not e>)" to "catch (Exception e)"
>
> although I want to look at the diff for that one to see how big it is
> before deciding whether or not it is worth the noise to commit.
>
> Depending on how that goes, I might get it to do
> "catch (IOException <not ioe>)" to "catch (IOException ioe)"
>
> After that ... ?
>
> There is a question of how far to take this and the commits already made
> to main might be enough. There is a trade off to make between consistent
> code being easier to read and noisy history making it harder to review
> changes over time.
>
> Thoughts?
I couldn't care less about noisy history. More readable means more
maintainable.
> P.S. I will be back-porting the logging changes but I might leave it
> until tomorrow in case folks object
+1
I would wait as well. You may a as well make a bunch of changes to main,
let anyone give feedback over a day or so, and then back-port everything
to the other branches.
-chris
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Logging clean-up
Posted by Mark Thomas <ma...@apache.org>.
On 19/08/2025 15:52, Christopher Schultz wrote:
> Mark,
>
> On 8/19/25 3:01 AM, Mark Thomas wrote:
>> All,
>>
>> I stumbled across quite few instances of log messages like this while
>> I was looking at something for $dayjob:
>>
>> log.error(sm.getString("naming.bindFailed", e));
>>
>> It would be lot more useful with the stack trace so I think that
>> really should be:
>>
>> log.error(sm.getString("naming.bindFailed"), e);
>
> I had to read that 3 or 4 times to see that the ) had moved.
>
> +1 for this change. If there is an exception being logged, I'm 100% in
> favor of logging a stack trace. Most logging frameworks can be
> configured to log stack traces or not. But they can't log (or not) if
> the exception itself is not present.
>
>> Unless there are objections, I am going to change any log message with
>> a level of warning or greater that currently includes e, e.toString(),
>> e.getMessage() etc in the log message and doesn't log the full stack
>> trace to log the stack trace. I'll likely remove the exception from
>> the log message as well to avoid duplicating the text in the logs.
>>
>> INFO level and is a little different. Those need looking at on a case
>> by case basis as I can imagine there are some cases where logging the
>> full stack trace is not necessary.
>
> +1
Reviewing log messages you end up looking at a LOT of catch blocks and
it is apparent that there are multiple naming conventions that have been
used over time / by different developers.
Since I am in the area I've got Cursor looking into using a consistent
naming convention. So far, its fixed:
"catch (Exception t)" to "catch (Exception e)"
"catch (Throwable e)" to "catch (Throwable t)"
which were the obviously confusing ones.
I've currently got it working on
"catch (Exception <not e>)" to "catch (Exception e)"
although I want to look at the diff for that one to see how big it is
before deciding whether or not it is worth the noise to commit.
Depending on how that goes, I might get it to do
"catch (IOException <not ioe>)" to "catch (IOException ioe)"
After that ... ?
There is a question of how far to take this and the commits already made
to main might be enough. There is a trade off to make between consistent
code being easier to read and noisy history making it harder to review
changes over time.
Thoughts?
Mark
P.S. I will be back-porting the logging changes but I might leave it
until tomorrow in case folks object
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Logging clean-up
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,
On 8/19/25 3:01 AM, Mark Thomas wrote:
> All,
>
> I stumbled across quite few instances of log messages like this while I
> was looking at something for $dayjob:
>
> log.error(sm.getString("naming.bindFailed", e));
>
> It would be lot more useful with the stack trace so I think that really
> should be:
>
> log.error(sm.getString("naming.bindFailed"), e);
I had to read that 3 or 4 times to see that the ) had moved.
+1 for this change. If there is an exception being logged, I'm 100% in
favor of logging a stack trace. Most logging frameworks can be
configured to log stack traces or not. But they can't log (or not) if
the exception itself is not present.
> Unless there are objections, I am going to change any log message with a
> level of warning or greater that currently includes e, e.toString(),
> e.getMessage() etc in the log message and doesn't log the full stack
> trace to log the stack trace. I'll likely remove the exception from the
> log message as well to avoid duplicating the text in the logs.
>
> INFO level and is a little different. Those need looking at on a case by
> case basis as I can imagine there are some cases where logging the full
> stack trace is not necessary.
+1
-chris
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org