You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2021/01/17 22:43:32 UTC
[GitHub] [logging-log4j2] ronosaurus opened a new pull request #460: LOG4J2-2986: introduce filters.startFrames config element to specifc starting frame(s) during Throwable rendering
ronosaurus opened a new pull request #460:
URL: https://github.com/apache/logging-log4j2/pull/460
- 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
----------------------------------------------------------------
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
[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
Posted by 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
[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
Posted by GitBox <gi...@apache.org>.
vy commented on a change in pull request #460:
URL: https://github.com/apache/logging-log4j2/pull/460#discussion_r559789496
##########
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) {
+ if (packagesOrStartFrames != null) {
+ String className = element.getClassName();
+ if (includeMethodName) { // support startFrames
+ className += "." + element.getMethodName();
+ }
+ for (final String item : packagesOrStartFrames) {
Review comment:
Prefer an index-based iteration instead if you want to avoid the iterator allocation at each call.
----------------------------------------------------------------
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
[GitHub] [logging-log4j2] rgoers commented on pull request #460: LOG4J2-2986: introduce filters.startFrames config element to specifc starting frame(s) during Throwable rendering
Posted by GitBox <gi...@apache.org>.
rgoers commented on pull request #460:
URL: https://github.com/apache/logging-log4j2/pull/460#issuecomment-974773547
Is anything happening with this?
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ronosaurus commented on pull request #460: LOG4J2-2986: introduce filters.startFrames config element to specifc starting frame(s) during Throwable rendering
Posted by GitBox <gi...@apache.org>.
ronosaurus commented on pull request #460:
URL: https://github.com/apache/logging-log4j2/pull/460#issuecomment-976105125
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.
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vy commented on pull request #460: LOG4J2-2986: introduce filters.startFrames config element to specifc starting frame(s) during Throwable rendering
Posted by GitBox <gi...@apache.org>.
vy commented on pull request #460:
URL: https://github.com/apache/logging-log4j2/pull/460#issuecomment-762443629
@ronosaurus, mind updating `changes.xml` and `layouts.xml.vm` too, please.
Keep in mind that this change set needs to be upstreamed to `master` as well. There one (you? :sweat_smile:) will need to deal with quite some merge conflicts.
----------------------------------------------------------------
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
[GitHub] [logging-log4j2] ronosaurus closed pull request #460: LOG4J2-2986: introduce filters.startFrames config element to specifc starting frame(s) during Throwable rendering
Posted by GitBox <gi...@apache.org>.
ronosaurus closed pull request #460:
URL: https://github.com/apache/logging-log4j2/pull/460
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vy edited a comment on pull request #460: LOG4J2-2986: introduce filters.startFrames config element to specifc starting frame(s) during Throwable rendering
Posted by GitBox <gi...@apache.org>.
vy edited a comment on pull request #460:
URL: https://github.com/apache/logging-log4j2/pull/460#issuecomment-762443629
@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? :sweat_smile:) will need to deal with quite some merge conflicts.
----------------------------------------------------------------
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
[GitHub] [logging-log4j2] rgoers commented on pull request #460: LOG4J2-2986: introduce filters.startFrames config element to specifc starting frame(s) during Throwable rendering
Posted by GitBox <gi...@apache.org>.
rgoers commented on pull request #460:
URL: https://github.com/apache/logging-log4j2/pull/460#issuecomment-974773547
Is anything happening with this?
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] rgoers commented on pull request #460: LOG4J2-2986: introduce filters.startFrames config element to specifc starting frame(s) during Throwable rendering
Posted by GitBox <gi...@apache.org>.
rgoers commented on pull request #460:
URL: https://github.com/apache/logging-log4j2/pull/460#issuecomment-761968243
@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.
----------------------------------------------------------------
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