Skip to content

Conversation

@rzara
Copy link
Member

@rzara rzara commented Dec 3, 2025

The stacktrace does not add relevant information, and thus pollutes the log file.

…d is exceeded

The stacktrace does not add relevant information, and thus pollutes the log file.
@ryahiaoui
Copy link
Member

For SizeLimitExceededException, this is a predictable error (user error). The error level may be excessive — a warn or debug level would be more appropriate. However, it's still important to log the exception itself (not just the message) to preserve the stack trace, which is useful for debugging and monitoring purposes in case of unexpected issues or patterns.

AppLogService.warn( "Size limit error for upload: {}", e.getMessage( ) );

@rzara
Copy link
Member Author

rzara commented Dec 4, 2025

My point is that the stack trace adds no useful information. SizeLimitExceededException is a specific case. The more general case for which less info is known in advance, FileUploadException will log the stack trace.

Besides, AppLogService.warn does not exist and your example would not log the stack trace anyway.
Lowering from Error to Warn would not prevent the logs from being polluted with this stack trace in the general case, because which site have a logger configured for only Error ?

@rzara
Copy link
Member Author

rzara commented Dec 4, 2025

To reiterate the point, SizeLimitExceededException is only ever emitted from one point in our case (from org.apache.commons.fileupload.FileUploadBase.FileItemIteratorImpl.FileItemIteratorImpl(FileUploadBase, RequestContext), which means the stack trace will always be the same and is thus irrelevant.

@ryahiaoui
Copy link
Member

I understand your point regarding the limited usefulness of the stack trace for SizeLimitExceededException, since it indeed always originates from the same place.
However, we should never “swallow” an exception: even if the case is specific, an exception must always be logged or properly propagated to avoid hiding unexpected or future issues.

Regarding the log level, we can certainly add a warning level to AppLogService, or use another appropriate level — but not error in this specific case. In my previous example, I simply forgot to pass the exception as an argument.

Furthermore, in a production environment, the recommended practice is to use the error level. This avoids polluting the logs and allows for much more efficient log analysis. Using a lower level systematically (info, …) can not only reduce log readability but also have performance implications.
AppLogService.warn( "Size limit error for upload: {}", e.getMessage( ), e );

@rzara
Copy link
Member Author

rzara commented Dec 4, 2025

This exception in this case would not be swallowed, but handled in two ways:

  • a log message indicating what limit was exceeded and by how much
  • A message displayed to the user indicating the same.
    I would maybe understand if you would argue to put the stack trace at the DEBUG level.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants