LOG4J2-2986: introduce filters.startFrames config element to specifc starting frame(s) during Throwable rendering #460

ronosaurus · 2021-01-17T22:43:22Z

Allow frame (package + method name) specific starting point on stack traces to suppress boilerplate web container, vendor framework, etc frames
filters.startFrames(frame1,frame2) name selected because its related to filters(package1,package2)
Suppress web container frames beneath the servlet entry point using filters.startFrame(javax.servlet.http.HttpServlet.service)
Updated various packages, ignorePackages, hasPackages variables to more descriptive and consistent filterPackages
Change a few signatures to accept new filterStartFrames parameter alongside filterPackages
- Future: combine both Lists into some kind of FilterConfig object to support future filters and reduce arguments to methods

Keeping fork up to date

…starting frame(s) during Throwable rendering

rgoers · 2021-01-18T04:15:41Z

@ronosaurus I am really not sure how you can do something wrong in a pull request, except maybe commit code for the wrong project or something. Sorry, I am in a busy period at work again and just haven't had much time to look at stuff.

vy · 2021-01-18T19:53:24Z

@ronosaurus, mind updating changes.xml and layouts.adoc too, please.

Keep in mind that this change set needs to be backported to release-2.x as well. (Right?) There one (you? 😅) will need to deal with quite some merge conflicts.

vy · 2021-01-18T20:06:08Z

log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThrowableFormatOptions.java

+    private final List<String> filterPackages;
+
+    private final List<String> filterStartFrames;


filterPackages suppresses the stack frames, whereas filterStartFrames truncates the stringified stack trace after any occurrence of the given package name. Correct me if I am wrong.

IMHO, filter here is already used in the wrong context. It is generally used for selecting something, not excluding it, e.g., Stream#filter(). I would have called it discardPackages. That said, I am probably more than a decade late to share this great wisdom. But maybe we can do something better for filterStartFrames. Here I see two options:

Stick to the historical context and go with filterStartFrames.

Try to improve the current state with the cost of some inconsistency in the syntax. discardAfterPackages?

vy · 2021-01-18T20:16:12Z

log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThrowableFormatOptions.java

+        if (filterStr.length() > 0) {
+            final String[] array = filterStr.split(Patterns.COMMA_SEPARATOR);
+            if (array.length > 0) {
+                packages = new ArrayList<>(array.length);


Doesn't this totally discard the initial content of the packages?

vy · 2021-01-18T20:17:04Z

log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThrowableFormatOptions.java

+     * Common code for extracting packages from "filters(com.example)" and "filters.startFrames(com.example)"
+     *
+     */
+    private static List<String> extractFilters(List<String> packages, String option, String conversionWord) {


If you don't want to get @garydgregory on your neck, you better have final modifiers here. 😜

vy · 2021-01-18T20:17:58Z

log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThrowableFormatOptions.java

+                for (String token : array) {
+                    token = token.trim();
+                    if (token.length() > 0) {
+                        packages.add(token);


Maybe we should move the earlier initialization of packages here?

vy · 2021-01-18T20:23:11Z

log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThrowableProxyRenderer.java

-            final String className = element.getClassName();
-            for (final String pkg : ignorePackages) {
-                if (className.startsWith(pkg)) {
+    private static boolean elementStartsWith(final StackTraceElement element, final List<String> packagesOrStartFrames, boolean includeMethodName) {


Sorry, I am allergic to these kind of unions. Why not simply having two arguments: packages and startFrames? (Please mind the final modifiers.)

vy · 2021-01-18T20:24:38Z

log4j-core/src/test/java/org/apache/logging/log4j/core/impl/ThrowableProxyTest.java

@@ -287,7 +287,7 @@ public void testSeparator_getExtendedStackTraceAsString() throws Exception {
        final ThrowableProxy proxy = new ThrowableProxy(throwable);

        final String separator = " | ";
-        final String extendedStackTraceAsString = proxy.getExtendedStackTraceAsString(null,
+        final String extendedStackTraceAsString = proxy.getExtendedStackTraceAsString(null,null,


Missing whitespace.

vy · 2021-01-18T20:27:32Z

...c/test/java/org/apache/logging/log4j/core/pattern/ExtendedThrowablePatternConverterTest.java

@@ -132,6 +132,55 @@ public void testFiltering() {
        assertTrue(result.contains(" suppressed "), "No suppressed lines");
    }

+    @Test
+    public void testFiltersStartFrames() {


This test indeed checks that the given filter pattern should not be found in the output. Though I would also check if the filter is not provided, the pattern indeed can be found.

vy · 2021-01-18T20:28:40Z

...c/test/java/org/apache/logging/log4j/core/pattern/ExtendedThrowablePatternConverterTest.java

+        final LogEvent event = Log4jLogEvent.newBuilder() //
+                .setLoggerName("testLogger") //
+                .setLoggerFqcn(this.getClass().getName()) //
+                .setLevel(Level.DEBUG) //
+                .setMessage(new SimpleMessage("test exception")) //
+                .setThrown(thrown).build();
+        final StringBuilder sb = new StringBuilder();
+        converter.format(event, sb);
+        final String parentToString = toString(thrown);
+        // System.out.println(parentToString);
+        final String result = sb.toString();
+        // System.out.println(result);


Would you mind removing these empty comment line starts and commented out lines, please?

vy · 2021-01-18T20:29:33Z

...c/test/java/org/apache/logging/log4j/core/pattern/ExtendedThrowablePatternConverterTest.java

        String result = sb.toString();
        result = result.replaceAll(" ~?\\[.*\\]", Strings.EMPTY);
-        final String expected = sw.toString(); //.replaceAll("\r", Strings.EMPTY);
+        final String expected = toString(parent); //.replaceAll("\r", Strings.EMPTY);


Why is the commented out replaceAll?

vy · 2021-01-18T20:30:06Z

...c/test/java/org/apache/logging/log4j/core/pattern/ExtendedThrowablePatternConverterTest.java

        assertEquals(expected, result);
    }
+
+    private String toString(Throwable ex) {


Mind the final modifier, please.

vy · 2021-01-18T20:49:45Z

log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThrowableProxyRenderer.java

+            if (includeMethodName) { // support startFrames
+                className += "." + element.getMethodName();
+            }
+            for (final String item : packagesOrStartFrames) {


Prefer an index-based iteration instead if you want to avoid the iterator allocation at each call.

rgoers · 2021-11-21T08:17:27Z

Is anything happening with this?

ronosaurus · 2021-11-23T02:18:26Z

Didn't realize this PR is 11 months old. I'm going to close, review the feedback, then eventually submit another PR from more current code.

ronosaurus added 2 commits January 9, 2021 16:14

Merge pull request #1 from apache/master

958ff76

Keeping fork up to date

LOG4J2-2986: introduce filters.startFrames config element to specifc …

1ce4c3d

…starting frame(s) during Throwable rendering

vy requested changes Jan 18, 2021

View reviewed changes

vy reviewed Jan 18, 2021

View reviewed changes

ronosaurus closed this Nov 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LOG4J2-2986: introduce filters.startFrames config element to specifc starting frame(s) during Throwable rendering #460

LOG4J2-2986: introduce filters.startFrames config element to specifc starting frame(s) during Throwable rendering #460

ronosaurus commented Jan 17, 2021

rgoers commented Jan 18, 2021

vy commented Jan 18, 2021 •

edited

vy Jan 18, 2021

vy Jan 18, 2021

vy Jan 18, 2021

vy Jan 18, 2021

vy Jan 18, 2021

vy Jan 18, 2021

vy Jan 18, 2021

vy Jan 18, 2021

vy Jan 18, 2021

vy Jan 18, 2021

vy Jan 18, 2021

rgoers commented Nov 21, 2021

ronosaurus commented Nov 23, 2021

		private final List<String> filterPackages;

		private final List<String> filterStartFrames;

LOG4J2-2986: introduce filters.startFrames config element to specifc starting frame(s) during Throwable rendering #460

LOG4J2-2986: introduce filters.startFrames config element to specifc starting frame(s) during Throwable rendering #460

Conversation

ronosaurus commented Jan 17, 2021

rgoers commented Jan 18, 2021

vy commented Jan 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgoers commented Nov 21, 2021

ronosaurus commented Nov 23, 2021

vy commented Jan 18, 2021 •

edited