Consolidate stack trace rendering in Pattern Layout

[alan0428a]

This PR

• Rewrites the stack rendering for Pattern Layout (i.e., %ex, %rEx, and %xEx converters)
  • Adds support for reporting circular references
• Reuses %ex to implement %rEx and %xEx. This effectively fixes:
  • LOG4J2-2170
  • LOG4J2-3627
  • ThrowablePatternConverter does not support "filters(packages)" #1729
  • ThrowableProxyHelper fails if a class in logged stack trace throws java.lang.Error from initializer #3137
• Rewrites all associated tests
• Deprecates ThrowableProxy and all its usages

[alan0428a]

Changes are based on the feedback in #1137.
However, as you might notice, there are similar methods in ThrowablePatternConverter and Throwableproxyrenderer, because of the package filtering added for %ex

Please let me know where to improve, thanks

[alan0428a]

Also I want to add more tests to cover the changes, but my Intellij IDEA shows below when I tried to run testException in ThrowableTest.

org.junit.jupiter.api.extension.ParameterResolutionException: No appender named List

	at org.apache.logging.log4j.core.test.junit.AppenderResolver.resolveParameter(AppenderResolver.java:50)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

Also my ./mvnw verify also failed.

[INFO] Apache Log4j Core .................................. FAILURE
....
[ERROR] Failed to execute goal biz.aQute.bnd:bnd-baseline-maven-plugin:7.0.0:baseline (check-api-compat) on project log4j-core: An error occurred while calculating the baseline: Baseline problems detected.

Could you let me know how to fix it?

[vy]

Great work @alan0428a! I have requested some changes.

See BUILDING.adoc explaining how a typical development cycle works, how to run tests, etc.

[vy]

@alan0428a, not to mention I am expecting to see several new tests. In particular, for %xEx{ [ "short" | depth]} and filter(packages).

[alan0428a]

Also add tests to cover the changes, please review when you have time.

[vy]

@alan0428a, thank you so much your great effort! 💯 It has been much appreciated! 🙇 I request more changes, but I think we are heading in the right direction. Please keep these contributions coming! 🚀

[vy]

@alan0428a, could you also create a src/changelog/.2.x.x/fix_throwable_converter_issues.xml, please? See other fix_*.xml files in the folder for the file structure. Please keep the description short, but still informative – imagine you will be reading that line (among hundreds of others) in the release notes.

[ppkarwasz]

Note about the test failures: unfortunately we have about a dozen of flaky tests in our project (see the Develocity statistics for a list).
The failures in those tests are not specific to this PR.

The failures in the Log4j Tag Library on the other hand seem connected to this PR and need to be investigated.

[alan0428a]

Just checked it's because before the PR, ThrowablePatternRender#formatOption serialize excpetion using Throwable#printStackTrace. But in the new implementation, I get the exception info using Throwable.getStackTrace and serialize using appendEntry, whose implementation is different from Throwable#printStackTrace.

The implementation in Throwable#printStackTrace is quite complicated, so I think it's better to get the exception info using Throwable#printStackTrace, instead of building our own implementation if we don't want to change the output format.

I am thinking the flow will be like(Just like the old way, but do not use String#split and add filter package)

1. Get exception info from `Throwable#printStackTrace`
2. Get `StringBuffer` from `StringWriter#getBuffer()`, which supports `indexOf()` since it extends `AbstractStringBuilder`
3. Use similar approach in `StringBuilders.truncateAfterDelimiter` but also append suffix, filter package and stop the loop when the line count reaches depth. 

Though the behavior is different from %rEx and %xEx(which processes through all lines then truncate), its "process and early stop" approach is same as the old implementation.

WDYT?

[rgoers]

Well shucks, while you were in there it would be nice to resolve LOG4J2-2170 and add in the module information.

[vy]

Well shucks, while you were in there it would be nice to resolve LOG4J2-2170 and add in the module information.

@rgoers, that ticket is implicitly resolved too. I added tests for that and updated the changelog entry.

[ppkarwasz]

@alan0428a and @vy, great job! 💯

This looks good to me, except the leading space that is added to each stack trace: Throwable.printStackTrace() does have a trailing new line, but does not have a leading space.

I know that the leading space was already there before, but it is quite unexpected. If a user asks for a %m%ex pattern, he should get just that: the message and the exception without anything inbetween.

I would remove the leading space and add it to the logic that automatically appends %xEx to a pattern:

• if a pattern does not contain an exception specifier and ends in %n or a whitespace character, then %xEx is appended,
• if a pattern does not contain an exception specifier and does not end in either %n nor whitespace, then %xEx is appended.

WDYT?

Note

I find the current algorithm that determines if a pattern contains an exception specifier extremely fragile.
The altorithm fails to detect any nested occurrence of %ex, e.g. %hightlight{%ex} or %notEmpty{ %ex}.

[vy]

This looks good to me, except the leading space that is added to each stack trace

@ppkarwasz, I implemented this, and it caused 99 test failures in log4j-core-test. I will implement this change in another PR.