vy commented on PR #2691: URL: https://github.com/apache/logging-log4j2/pull/2691#issuecomment-2213522269
@rgoers, I very much liked the idea of retiring `ThrowableProxy`. I analyzed the possibility of this. I will share my findings below. ### What does Logback do? AFAIK, Log4j's `%xEx` is added to match the same functionality in Logback. I examined Logback documentation and its source code on this: 1. [Logback has disabled packaging data by default](https://logback.qos.ch/manual/configuration.html#packagingData) (since the `1.1.4` release published in February 2016, due to performance reasons) 2. [Logback reports packaging information doesn't play nice with Netbeans](https://jira.qos.ch/browse/LOGBACK-324) 3. [Logback deprecated `ClassPackagingData` in 2023-05-11](https://github.com/qos-ch/logback/commit/9e833ec858953a2296afdc3292f8542fc08f2a45#diff-8080e53d7f4a538f1d664631592abaa848b903e2c3e40a46f845647231c9bdad) ### What does `ThrowableProxy` do? * Helps with serialization – nobody would miss its absence * Helps with capturing the class loader context and does this efficiently – i.e., the `StackLocatorUtil.getCurrentStackTrace()` call. This bit is important, in particular, during asynchronous logging. That is, the class loader et al. needs to be captured _before_ the asynchronous barrier. But guess what, we don't do that! `LogEvent#getThrownProxy()` is called by the layout of the appender, i.e., _after_ the asynchronous barrier, and it gets populated on demand. Put another way, `ThrownProxy` is populated by the request from layout, after asynchronous logging scheduled the task away from the original user thread. ### Proposal: Create a new `ThrowableRenderer` class, rewire `ThrowableProxyRenderer` calls to this new class, and deprecate `ThrowableProxy` et al. 1. Create a new *internal* (package-private, etc.) `ThrowableRenderer<C>` class taking care of `Throwable` (not `ThrowableProxy`!) rendering such that: ``` package org.apache.logging.log4j.core.pattern; // Note package-private class/ctor/method definitions for encapsulation! class ThrowableRenderer<C> { ThrowableRenderer( Set<String> ignoredPackageNames, String stackTraceElementSuffix, String lineSeparator, int maxLineCount) { // Initialize package-private final fields... } // This is the method `Converter`s will be calling. // // Note the `final` modifier – subclasses should override the // `renderThrowable()` method accepting `C`. final void renderThrowable(StringBuilder buffer, Throwable throwable) { C context = createContext(throwable); renderThrowable(buffer, throwable, context); } // This will be overridden by `ExtendedThrowableRenderer` to pass the // map containing the `StackTraceElement`-to-packagingInformation cache. C createContext(Throwable throwable) { return null; } // This will be overridden by `RootThrowableRenderer` to // reverse the causal chain order. void renderThrowable(StringBuilder buffer, Throwable throwable, C context) { // Render `Throwable`... } // This will be overridden by `ExtendedThrowableRenderer` to // inject the packaging information cached in `context`. void renderStackTraceElement( StringBuilder buffer, StackTraceElement stackTraceElement, C context) { // Render `StackTraceElement`... } } ``` 2. Use an instance of `ThrowableRenderer<Void>` in `ThrowablePatternConverter` 3. Create the package-private `RootThrowableRenderer` class extending from `ThrowableRenderer<Void>` by overriding its `renderThrowable(Throwable,Void)` to reverse the causal chain rendering. 4. Use an instance of `RootThrowableRenderer` in `RootThrowablePatternConverter` 6. Create the package-private `ExtendedThrowableRenderer` class extending from `ThrowableRenderer<PackagingInfoCache>` such that: ``` class ExtendedThrowableRenderer extends ThrowableRenderer<PackagingInfoCache> { @Override PackagingInfoCache createContext(Throwable throwable) { // You need `StackLocatorUtil.getCurrentStackTrace()` and some other cache // structures used in `ThrowableProxyHelper.toExtendedStackTrace()`. } @Override void renderStackTraceElement( StringBuilder buffer, StackTraceElement element, PackagingInfoCache cache) { String location = cache.get(element); // Copy-paste from parent and inject `location` appropriately. } // Alternatively, instead of copy-pasting the body of `renderStackTraceElement()` // from the parent as described above, you can also make the parent logic reusable // by one more fine-grained (package-private static) method in the parent: static void renderStackTraceElement( StringBuilder buffer, StackTraceElement element, // You can pass location information in the suffix. // That is, `suffix = location + suffix`. String suffix, String newlineSeparator) { // ... } } ``` 7. Use an instance of `ExtendedThrowableRenderer` in `ExtendedThrowablePatternConverter` 8. Deprecate `LogEvent#getThrownProxy()` 9. Deprecate `ThrowableProxy` 10. Deprecate `ExtendedStackTraceElement` and `ExtendedClassInfo` 11. Remove `ThrowableProxyRenderer` 12. Double-check `ExtendedThrowableRenderer` performance (Refer to this recent related performance bug: #1214) 13. Revamp tests 14. Create a follow-up PR to 1. Cherry-pick changes from `2.x` 2. Remove all deprecated methods and classes from `main` Note that the proposed `ThrowableRender` doesn't use `TextRenderer` as its predecessor `ThrowableProxyRenderer` does. ### Feedback @alan0428a, @rgoers, @ppkarwasz, could you comment, please? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
