vy commented on PR #2691: URL: https://github.com/apache/logging-log4j2/pull/2691#issuecomment-2208951635
> @vy Actually `%rEx` and `%xEx` both operate on a `ThrowableProxy` in existing implementation. I think this is a ~feature~ bug. `%rEx` should operate on the `Throwable`, just like `%ex`. Hence, I prefer this to be fixed. > So only `%ex` needs to rely on `Throwable#printStackTrace()`. However, the output of `Throwable#printStackTrace()` looks like below: > ... > If we want to do filtering, we can only check if a line contains target package in `options.getIgnorePackages()`. For one, this won't be exactly correct. You will be matching against the stack trace line, not the package name of the associated `StackTraceElement`. But I am not concerned about this loose end. My main concern is, implementing `packages` filtering on a `StringBuilder` containing a stack trace won't be trivial. You not only need to filter lines, but also report them ala `ThrowableProxyRenderer.appendSuppressedCount()`. We will just be duplicating the effort that is already there for `ThrowableProxyRenderer`. Even though [I earlier supported the idea of _"sticking to `Throwable#printStackTrace()` wherever possible"_](https://github.com/apache/logging-log4j2/pull/2691#issuecomment-2204076668), coming to think about it again... If a user provides either `packages` or `maxLineCount`, it is certain that they won't get a standard stack trace anymore. Given this, I suggest both `%ex` and `%rEx` delegate to `ThrowableProxyRenderer` (just like `%xEx`) iff either `packages`, `maxLineCount`, `suffix`, or `lineSeparator` is provided. This approach will have the following pros/cons: 1. All stack trace dump mangling will be consolidated at one place – Great! 1. `%ex` and `%rEx` will report package information unnecessarily – Acceptable (I will explain a fix for this!) 1. Allocation cost will slightly increase – Acceptable Let me be more clear: 1. If either `packages`, `maxLineCount`, `suffix`, or `lineSeparator` is provided, 2. `ThrowablePatternConverter` will obtain a `ThrowableProxy`: ``` event.getThrownProxy() != null ? event.getThrownProxy() : new ThrowableProxy(event.getThrown()) ``` 3. `RootThrowablePatternConverter`, will find the root cause and wrap it with in `ThrowableProxy`: ``` Throwable rootCause = Throwables.getRootCause(event.getThrown()); new ThrowableProxy(rootCause); ``` 4. Both `ThrowablePatternConverter` and `RootThrowablePatternConverter` will use `ThrowableProxyRenderer` with the obtained `ThrowableProxy`. ### What about unsolicited package information? If we consolidate all stack trace mangling to `ThrowableProxyRenderer`, `%ex` and `%rEx` will report package information too. We can fix this by adding an extra `extendedClassInfoRendered` boolean parameter to its `formatWrapper()`. This flag needs to be passed all the way down to `ExtendedStackTraceElement#renderOn()`. ### Feedback @alan0428a, before starting to implement this, I really would like to hear your thoughts too. In the meantime, I will ask this to some other PMC members too. ### Thanks! @alan0428a, I can't thank you enough for this contribution and walking with us along this process. :bow: Please try to understand us for the thorough review process and see that it is not easy to come up with a good resolution, putting aside the difficulty of implementing that resolution. I am very very happy with interacting with you so far and please keep these contributions coming. It would be great to see you as a regular Log4j contributor! :heart_eyes: -- 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]
