Skip to content

Conversation

@theit
Copy link
Contributor

@theit theit commented Nov 7, 2022

This change restores the feature to limit the number of lines of a stack trace that is logged and adds a couple of test methods for the corresponding JUnit test class.

…king

This change restores the feature to limit the number of lines of a stack trace that is logged and adds a couple of test methods for the corresponding JUnit test class.
Copy link
Member

@vy vy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@theit! Thanks so much for the bug report and fix! Much appreciated! I have requested some changes, would you mind checking them out, please? Also a changes.xml entry would be handy too.

@theit theit changed the title Fix for LOG4J2-3627: PatternLayout: %xEx{ [ "short" | depth]} not working Fix for LOG4J2-3627: PatternLayout: %xEx{ [ "short" | depth ] } not working Nov 8, 2022
@theit theit changed the title Fix for LOG4J2-3627: PatternLayout: %xEx{ [ "short" | depth ] } not working LOG4J2-3627: PatternLayout: %xEx{ [ "short" | depth ] } not working Nov 8, 2022
@theit
Copy link
Contributor Author

theit commented Dec 23, 2022

Sorry the very late reply. I couldn't work earlier on the ticket because of $DAYJOB and several sick days... :-/
I reworked the patch and added the stack trace stripping as requested. This is basically done in the formatXXX methods in ThrowableProxyRenderer. Can you please have a look and review the changes?

@theit theit requested a review from vy January 4, 2023 06:03
@jvz jvz added the bug Incorrect, unexpected, or unintended behavior of existing code label Jan 14, 2023
@vy vy deleted the branch apache:release-2.x February 28, 2023 15:02
@vy vy closed this Feb 28, 2023
@ppkarwasz
Copy link
Contributor

This was closed automatically by Github because we renamed the release-2.x branch to 2.x. Feel free to resubmit to the 2.x branch.

@hein4daddel
Copy link

Will this fix be available in the next release? (haven't found it in 2.20.0)

@vy
Copy link
Member

vy commented Apr 24, 2023

I am picking this up. I first need to wrap my mind around the changes.

@vy vy self-assigned this Apr 24, 2023
@vy
Copy link
Member

vy commented May 8, 2023

@theit, sorry for the late response. I was just able to pick this up.

Both ThrowablePatternConverter (%ex) and RootThrowablePatternConverter (%rEx), which delegates back to ThrowablePatternConverter (%xEx), simply do a String#split("\\n") and retain the first N lines. On the other hand, your ThrowableProxyRenderer changes count and, if necessary, limit the number of lines along the rendering process. This diligent work (which I am thankful that you took time to implement) is pretty sophisticated (and error-prone?) compared to its %ex counterpart. I would be in favor of a simpler approach: simply truncate the lines in StringBuilder just before the exit in package-private methods of ThrowableProxyRenderer. If you can implement this, I would be more than happy to accept your PR.

Ideally, we should wrap the StringBuilder with a LineLimitedStringBuilder wrapper, and pass that to Throwable rendering routines. But that will probably necessitate changes in dozens of files. Hence, let's try to keep it simple for now.

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

Labels

bug Incorrect, unexpected, or unintended behavior of existing code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants