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 2022/11/07 21:02:10 UTC
[GitHub] [logging-log4j2] theit opened a new pull request, #1137: Fix for LOG4J2-3627: PatternLayout: %xEx{ [ "short" | depth]} not working
theit opened a new pull request, #1137:
URL: https://github.com/apache/logging-log4j2/pull/1137
This change restores the feature to limit the number of lines of a stack trace that is logged and adds a couple of test methods for the corresponding JUnit test class.
--
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] theit commented on a diff in pull request #1137: LOG4J2-3627: PatternLayout: %xEx{ [ "short" | depth ] } not working
Posted by GitBox <gi...@apache.org>.
theit commented on code in PR #1137:
URL: https://github.com/apache/logging-log4j2/pull/1137#discussion_r1016385846
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/ExtendedThrowablePatternConverter.java:
##########
@@ -71,8 +72,22 @@ public void format(final LogEvent event, final StringBuilder toAppendTo) {
if (len > 0 && !Character.isWhitespace(toAppendTo.charAt(len - 1))) {
toAppendTo.append(' ');
}
- proxy.formatExtendedStackTraceTo(toAppendTo, options.getIgnorePackages(),
+ final String trace = proxy.getExtendedStackTraceAsString(options.getIgnorePackages(),
Review Comment:
The change in `ExtendedThrowablePatternConverter` is similar to what is being done in `RootThrowablePatternConverter`. And having a look in the history, also basically the same as in commit 078f8c877d7454527f100f4331a5c132e70eeb0c...
Regarding your comment, then both classes should be changed and the code unified; well, at least as far as possible.
--
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
Re: [PR] LOG4J2-3627: PatternLayout: %xEx{ [ "short" | depth ] } not working (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on PR #1137:
URL: https://github.com/apache/logging-log4j2/pull/1137#issuecomment-1520828469
I am picking this up. I first need to wrap my mind around the changes.
--
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
Re: [PR] LOG4J2-3627: PatternLayout: %xEx{ [ "short" | depth ] } not working (logging-log4j2)
Posted by "hein4daddel (via GitHub)" <gi...@apache.org>.
hein4daddel commented on PR #1137:
URL: https://github.com/apache/logging-log4j2/pull/1137#issuecomment-1519908835
Will this fix be available in the next release? (haven't found it in 2.20.0)
--
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 a diff in pull request #1137: LOG4J2-3627: PatternLayout: %xEx{ [ "short" | depth ] } not working
Posted by GitBox <gi...@apache.org>.
vy commented on code in PR #1137:
URL: https://github.com/apache/logging-log4j2/pull/1137#discussion_r1017115243
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/ExtendedThrowablePatternConverter.java:
##########
@@ -71,8 +72,22 @@ public void format(final LogEvent event, final StringBuilder toAppendTo) {
if (len > 0 && !Character.isWhitespace(toAppendTo.charAt(len - 1))) {
toAppendTo.append(' ');
}
- proxy.formatExtendedStackTraceTo(toAppendTo, options.getIgnorePackages(),
+ final String trace = proxy.getExtendedStackTraceAsString(options.getIgnorePackages(),
Review Comment:
To be honest, I would appreciate a structural fix practicing some Boy Scout Rule: _"Always leave the campground cleaner than you found it."_ AFAIC, the entire `*Converter` family suffers from _"let me fix this particular issue for this particular converter"_ attempts. Please don't get me wrong, I am not pointing any fingers; I am as guilty as everybody else who is in a position to improve this situation. I also don't want to discourage you, I cannot emphasize enough how much I appreciate your effort. Long story short, I personally prefer a structural fix: moving this logic to a single place, testing it decently, and reusing it wherever needed. If you can provide one, that would be awesome. Otherwise, I will try to give it a shot myself, though I need to admit that my Log4j agenda is already pretty packed.
--
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
Re: [PR] LOG4J2-3627: PatternLayout: %xEx{ [ "short" | depth ] } not working (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #1137:
URL: https://github.com/apache/logging-log4j2/pull/1137#issuecomment-1449445602
This was closed automatically by Github because we renamed the `release-2.x` branch to `2.x`. Feel free to resubmit to the `2.x` branch.
--
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 a diff in pull request #1137: Fix for LOG4J2-3627: PatternLayout: %xEx{ [ "short" | depth]} not working
Posted by GitBox <gi...@apache.org>.
vy commented on code in PR #1137:
URL: https://github.com/apache/logging-log4j2/pull/1137#discussion_r1015911291
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/ExtendedThrowablePatternConverter.java:
##########
@@ -71,8 +72,22 @@ public void format(final LogEvent event, final StringBuilder toAppendTo) {
if (len > 0 && !Character.isWhitespace(toAppendTo.charAt(len - 1))) {
toAppendTo.append(' ');
}
- proxy.formatExtendedStackTraceTo(toAppendTo, options.getIgnorePackages(),
+ final String trace = proxy.getExtendedStackTraceAsString(options.getIgnorePackages(),
Review Comment:
`formatExtendedStackTrace` strives to perform a garbage-free dump. Your version switches the behaviour to allocate an entire `String` (and more) every time. Next to that, I would expect this _"stacktrace trimming"_ to be a functionality of `ThrowablePatternConverter`, which is extended by `ExtendedThrowablePatternConverter`, rather than duplicating it here.
In conclusion, I expect stacktrace trimming
* to be provided by `ThrowablePatternConverter` and
* to be discarded and `formatExtendedStackTraceTo()` used instead, if no trimming is requested
--
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] theit commented on a diff in pull request #1137: LOG4J2-3627: PatternLayout: %xEx{ [ "short" | depth ] } not working
Posted by GitBox <gi...@apache.org>.
theit commented on code in PR #1137:
URL: https://github.com/apache/logging-log4j2/pull/1137#discussion_r1017443269
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/ExtendedThrowablePatternConverter.java:
##########
@@ -71,8 +72,22 @@ public void format(final LogEvent event, final StringBuilder toAppendTo) {
if (len > 0 && !Character.isWhitespace(toAppendTo.charAt(len - 1))) {
toAppendTo.append(' ');
}
- proxy.formatExtendedStackTraceTo(toAppendTo, options.getIgnorePackages(),
+ final String trace = proxy.getExtendedStackTraceAsString(options.getIgnorePackages(),
Review Comment:
Don't panic, I already understood what you meant :-)
I'm still thinking about how to fix that, but didn't fully figure out where and what kind of changing is needed / reasonable.
--
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
Re: [PR] LOG4J2-3627: PatternLayout: %xEx{ [ "short" | depth ] } not working (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on PR #1137:
URL: https://github.com/apache/logging-log4j2/pull/1137#issuecomment-1538034225
@theit, sorry for the late response. I was just able to pick this up.
Both `ThrowablePatternConverter` (`%ex`) and `RootThrowablePatternConverter` (`%rEx`), which delegates back to `ThrowablePatternConverter` (`%xEx`), simply do a `String#split("\\n")` and retain the first `N` lines. On the other hand, your `ThrowableProxyRenderer` changes count and, if necessary, limit the number of lines along the rendering process. This diligent work (which I am thankful that you took time to implement) is pretty sophisticated (and error-prone?) compared to its `%ex` counterpart. I would be in favor of a simpler approach: simply **truncate the lines in `StringBuilder` just before the exit in package-private methods of `ThrowableProxyRenderer`**. If you can implement this, I would be more than happy to accept your PR.
Ideally, we should wrap the `StringBuilder` with a `LineLimitedStringBuilder` wrapper, and pass that to `Throwable` rendering routines. But that will probably necessitate changes in dozens of files. Hence, let's try to keep it simple for now.
--
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] theit commented on pull request #1137: LOG4J2-3627: PatternLayout: %xEx{ [ "short" | depth ] } not working
Posted by GitBox <gi...@apache.org>.
theit commented on PR #1137:
URL: https://github.com/apache/logging-log4j2/pull/1137#issuecomment-1363798548
Sorry the very late reply. I couldn't work earlier on the ticket because of $DAYJOB and several sick days... :-/
I reworked the patch and added the stack trace stripping as requested. This is basically done in the formatXXX methods in ThrowableProxyRenderer. Can you please have a look and review the changes?
--
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
Re: [PR] LOG4J2-3627: PatternLayout: %xEx{ [ "short" | depth ] } not working (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy closed pull request #1137: LOG4J2-3627: PatternLayout: %xEx{ [ "short" | depth ] } not working
URL: https://github.com/apache/logging-log4j2/pull/1137
--
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