[GitHub] [logging-log4j2] vy commented on a change in pull request #460: LOG4J2-2986: introduce filters.startFrames config element to specifc starting frame(s) during Throwable rendering

[GitBox <gi...@apache.org>]

vy commented on a change in pull request #460:
URL: https://github.com/apache/logging-log4j2/pull/460#discussion_r559778566



##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThrowableFormatOptions.java
##########
@@ -298,7 +321,28 @@ public static ThrowableFormatOptions newInstance(String[] options) {
                 }
             }
         }
-        return new ThrowableFormatOptions(lines, separator, packages, ansiRenderer, suffix);
+        return new ThrowableFormatOptions(lines, separator, filterPackages, filterStartFrames, ansiRenderer, suffix);
+    }
+
+    /**
+     * 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) {

Review comment:
       If you don't want to get @garydgregory on your neck, you better have `final` modifiers here. :stuck_out_tongue_winking_eye: 

##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThrowableFormatOptions.java
##########
@@ -298,7 +321,28 @@ public static ThrowableFormatOptions newInstance(String[] options) {
                 }
             }
         }
-        return new ThrowableFormatOptions(lines, separator, packages, ansiRenderer, suffix);
+        return new ThrowableFormatOptions(lines, separator, filterPackages, filterStartFrames, ansiRenderer, suffix);
+    }
+
+    /**
+     * 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) {
+        final String filterStr = option.substring(conversionWord.length(), option.length() - 1);
+        if (filterStr.length() > 0) {
+            final String[] array = filterStr.split(Patterns.COMMA_SEPARATOR);
+            if (array.length > 0) {
+                packages = new ArrayList<>(array.length);

Review comment:
       Doesn't this totally discard the initial content of the `packages`?

##########
File path: log4j-core/src/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() {
+        final ExtendedThrowablePatternConverter converter = ExtendedThrowablePatternConverter.newInstance(null, new String[] {
+            "filters.startFrames(org.apache.logging.log4j.core.pattern.ExtendedThrowablePatternConverterTest$ContrivedWebStack.vendorFramework)"});
+
+        Throwable thrown = null;
+        try {
+          new ContrivedWebStack().webContainerEntryPoint();
+        } catch (Throwable t) {
+            thrown = t;
+        }
+
+        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);

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

##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThrowableFormatOptions.java
##########
@@ -75,7 +75,9 @@
     /**
      * The list of packages to filter.
      */
-    private final List<String> ignorePackages;
+    private final List<String> filterPackages;
+
+    private final List<String> filterStartFrames;

Review comment:
       `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:
   
   1. Stick to the historical context and go with `filterStartFrames`.
   2. Try to improve the current state with the cost of some inconsistency in the syntax. `discardAfterPackages`?

##########
File path: log4j-core/src/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() {

Review comment:
       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.

##########
File path: log4j-core/src/test/java/org/apache/logging/log4j/core/pattern/ExtendedThrowablePatternConverterTest.java
##########
@@ -145,21 +194,25 @@ public void testFull() {
                 .setThrown(parent).build();
         final StringBuilder sb = new StringBuilder();
         converter.format(event, sb);
-        final StringWriter sw = new StringWriter();
-        final PrintWriter pw = new PrintWriter(sw);
-        parent.printStackTrace(pw);
         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);

Review comment:
       Why is the commented out `replaceAll`?

##########
File path: log4j-core/src/test/java/org/apache/logging/log4j/core/pattern/ExtendedThrowablePatternConverterTest.java
##########
@@ -145,21 +194,25 @@ public void testFull() {
                 .setThrown(parent).build();
         final StringBuilder sb = new StringBuilder();
         converter.format(event, sb);
-        final StringWriter sw = new StringWriter();
-        final PrintWriter pw = new PrintWriter(sw);
-        parent.printStackTrace(pw);
         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);
         assertEquals(expected, result);
     }
+
+    private String toString(Throwable ex) {

Review comment:
       Mind the `final` modifier, please.

##########
File path: 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,

Review comment:
       Missing whitespace.

##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThrowableFormatOptions.java
##########
@@ -298,7 +321,28 @@ public static ThrowableFormatOptions newInstance(String[] options) {
                 }
             }
         }
-        return new ThrowableFormatOptions(lines, separator, packages, ansiRenderer, suffix);
+        return new ThrowableFormatOptions(lines, separator, filterPackages, filterStartFrames, ansiRenderer, suffix);
+    }
+
+    /**
+     * 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) {
+        final String filterStr = option.substring(conversionWord.length(), option.length() - 1);
+        if (filterStr.length() > 0) {
+            final String[] array = filterStr.split(Patterns.COMMA_SEPARATOR);
+            if (array.length > 0) {
+                packages = new ArrayList<>(array.length);
+                for (String token : array) {
+                    token = token.trim();
+                    if (token.length() > 0) {
+                        packages.add(token);

Review comment:
       Maybe we should move the earlier initialization of `packages` here?

##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThrowableProxyRenderer.java
##########
@@ -148,11 +154,14 @@ private static void formatEntry(final ExtendedStackTraceElement extStackTraceEle
         textRenderer.render(lineSeparator, sb, "Text");
     }
 
-    private static boolean ignoreElement(final StackTraceElement element, final List<String> ignorePackages) {
-        if (ignorePackages != null) {
-            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) {

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




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org