-
-
Notifications
You must be signed in to change notification settings - Fork 971
fix 4x exception logging #15564
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 7.1.x
Are you sure you want to change the base?
fix 4x exception logging #15564
Changes from all commits
83b22f6
e85c094
ce60438
d1ba16d
90e375e
024b9c8
7678cf4
2963cd9
c68360f
d5d70a6
cbf9f70
c67f460
9c7a7a3
870f9a3
e4c678a
2ba566d
b999949
627a1e9
1583d59
92eae8b
f5d28ec
e4b96a8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | |||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -33,6 +33,15 @@ | ||||||||||||||||||||||||||||||||
| */ | |||||||||||||||||||||||||||||||||
| public class DefaultStackTraceFilterer implements StackTraceFilterer { | |||||||||||||||||||||||||||||||||
| public static final String STACK_LOG_NAME = "StackTrace"; | |||||||||||||||||||||||||||||||||
| /** | |||||||||||||||||||||||||||||||||
| * Dedicated logger for exception stack traces. The filterer emits the unfiltered | |||||||||||||||||||||||||||||||||
| * trace to this logger as a side effect of {@link #filter(Throwable)} — before the | |||||||||||||||||||||||||||||||||
| * trace is trimmed in place — when {@link #logFullStackTraceOnFilter} is {@code true} | |||||||||||||||||||||||||||||||||
| * (the default). {@code GrailsExceptionResolver} also emits to this logger when | |||||||||||||||||||||||||||||||||
| * {@code grails.exceptionresolver.logFullStackTrace} is enabled. Exposed as a public | |||||||||||||||||||||||||||||||||
| * constant so that subclasses and logback configurations can reference the logger | |||||||||||||||||||||||||||||||||
| * name symbolically. | |||||||||||||||||||||||||||||||||
| */ | |||||||||||||||||||||||||||||||||
| public static final Log STACK_LOG = LogFactory.getLog(STACK_LOG_NAME); | |||||||||||||||||||||||||||||||||
|
|
|||||||||||||||||||||||||||||||||
| private static final String[] DEFAULT_INTERNAL_PACKAGES = new String[] { | |||||||||||||||||||||||||||||||||
|
|
@@ -58,6 +67,7 @@ public class DefaultStackTraceFilterer implements StackTraceFilterer { | ||||||||||||||||||||||||||||||||
| private List<String> packagesToFilter = new ArrayList<>(); | |||||||||||||||||||||||||||||||||
| private boolean shouldFilter; | |||||||||||||||||||||||||||||||||
| private String cutOffPackage = null; | |||||||||||||||||||||||||||||||||
| private boolean logFullStackTraceOnFilter = true; | |||||||||||||||||||||||||||||||||
|
|
|||||||||||||||||||||||||||||||||
| public DefaultStackTraceFilterer() { | |||||||||||||||||||||||||||||||||
| this(!Boolean.getBoolean(SYS_PROP_DISPLAY_FULL_STACKTRACE)); | |||||||||||||||||||||||||||||||||
|
|
@@ -77,36 +87,52 @@ public void setCutOffPackage(String cutOffPackage) { | ||||||||||||||||||||||||||||||||
| this.cutOffPackage = cutOffPackage; | |||||||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||||||
|
|
|||||||||||||||||||||||||||||||||
| /** | |||||||||||||||||||||||||||||||||
| * Controls whether {@link #filter(Throwable)} emits the unfiltered stack trace | |||||||||||||||||||||||||||||||||
| * to {@link #STACK_LOG} as a side effect before trimming the trace in place. | |||||||||||||||||||||||||||||||||
| * Defaults to {@code true} for backwards compatibility with pre-7.1 behaviour; | |||||||||||||||||||||||||||||||||
| * set to {@code false} to disable the side-effect emission. The exception | |||||||||||||||||||||||||||||||||
| * resolver wires this from {@code grails.exceptionresolver.logFullStackTraceOnFilter}. | |||||||||||||||||||||||||||||||||
| */ | |||||||||||||||||||||||||||||||||
| public void setLogFullStackTraceOnFilter(boolean logFullStackTraceOnFilter) { | |||||||||||||||||||||||||||||||||
| this.logFullStackTraceOnFilter = logFullStackTraceOnFilter; | |||||||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||||||
|
|
|||||||||||||||||||||||||||||||||
| public Throwable filter(Throwable source, boolean recursive) { | |||||||||||||||||||||||||||||||||
| if (recursive) { | |||||||||||||||||||||||||||||||||
| Throwable current = source; | |||||||||||||||||||||||||||||||||
| while (current != null) { | |||||||||||||||||||||||||||||||||
| current = filter(current); | |||||||||||||||||||||||||||||||||
| filter(current); | |||||||||||||||||||||||||||||||||
| current = current.getCause(); | |||||||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||||||
| return source; | |||||||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||||||
| return filter(source); | |||||||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||||||
|
|
|||||||||||||||||||||||||||||||||
| public Throwable filter(Throwable source) { | |||||||||||||||||||||||||||||||||
| if (shouldFilter) { | |||||||||||||||||||||||||||||||||
| StackTraceElement[] trace = source.getStackTrace(); | |||||||||||||||||||||||||||||||||
| List<StackTraceElement> newTrace = filterTraceWithCutOff(trace, cutOffPackage); | |||||||||||||||||||||||||||||||||
| if (!shouldFilter) { | |||||||||||||||||||||||||||||||||
| return source; | |||||||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||||||
| StackTraceElement[] trace = source.getStackTrace(); | |||||||||||||||||||||||||||||||||
| List<StackTraceElement> newTrace = filterTraceWithCutOff(trace, cutOffPackage); | |||||||||||||||||||||||||||||||||
|
|
|||||||||||||||||||||||||||||||||
| if (newTrace.isEmpty()) { | |||||||||||||||||||||||||||||||||
| // filter with no cut-off so at least there is some trace | |||||||||||||||||||||||||||||||||
| newTrace = filterTraceWithCutOff(trace, null); | |||||||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||||||
| if (newTrace.isEmpty()) { | |||||||||||||||||||||||||||||||||
| // filter with no cut-off so at least there is some trace | |||||||||||||||||||||||||||||||||
| newTrace = filterTraceWithCutOff(trace, null); | |||||||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||||||
|
|
|||||||||||||||||||||||||||||||||
| // Only trim the trace if there was some application trace on the stack | |||||||||||||||||||||||||||||||||
| // if not we will just skip sanitizing and leave it as is | |||||||||||||||||||||||||||||||||
| if (!newTrace.isEmpty()) { | |||||||||||||||||||||||||||||||||
| // We don't want to lose anything, so log it | |||||||||||||||||||||||||||||||||
| // Only trim the trace if there was some application trace on the stack | |||||||||||||||||||||||||||||||||
| // if not we will just skip sanitizing and leave it as is | |||||||||||||||||||||||||||||||||
| if (!newTrace.isEmpty()) { | |||||||||||||||||||||||||||||||||
| if (logFullStackTraceOnFilter) { | |||||||||||||||||||||||||||||||||
| // emit the unfiltered trace before mutating in place; once setStackTrace(clean) | |||||||||||||||||||||||||||||||||
| // runs the original frames are gone | |||||||||||||||||||||||||||||||||
| STACK_LOG.error(FULL_STACK_TRACE_MESSAGE, source); | |||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We discussed this and we think this should stay with a configuration setting.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consulted AI and it thinks it is a really bad idea: can you just introduce another config value and default it to false? It would work, but the mess comes back for opt-in users. Let me trace it. Setup
Output matrix for a 3-cause request exception
The bottom two rows are exactly the per-cause duplication the PR fixes. The user who opts into Non-resolver sanitize path
This is the only clean win. But it only kicks in when the flag is on, at which point the user is also paying the 4-entry penalty on Plus the layering problem
Option 3 is the cleanest plumbing-wise but means users opt in via JVM flag rather than Cleaner alternativeDocument one line in the upgrade guide for teams relying on non-resolver sanitize coverage: // before: relied on side-effect log
Throwable cleaned = GrailsUtil.sanitizeRootCause(ex);
log.error("job failed", cleaned);
// after: explicit
Throwable cleaned = GrailsUtil.sanitizeRootCause(ex);
DefaultStackTraceFilterer.STACK_LOG.error(StackTraceFilterer.FULL_STACK_TRACE_MESSAGE, cleaned);
log.error("job failed", cleaned); No new flag. No layering workaround. No duplicate-per-cause regression for users who happen to need non-resolver coverage. My call If you want a concrete non-resolver path you want covered, better the two-line migration. If the project wants blanket pre-PR compatibility, the second flag works but I'd go the system-property route (option 3) to avoid the layering mess, and I'd document clearly that turning it on without also turning logFullStackTrace off produces N+1 entries per exception. Honestly though: the second flag is solving the problem by re-exposing the old mechanism. The cleaner fix is the two-line code change at the actual call sites that want the behavior. That's explicit, discoverable at the code site, and doesn't involve tuning two interacting flags.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a config setting preserves both and we agreed in the weekly that this was the way forward, I am -1 on this without this |
|||||||||||||||||||||||||||||||||
| StackTraceElement[] clean = new StackTraceElement[newTrace.size()]; | |||||||||||||||||||||||||||||||||
| newTrace.toArray(clean); | |||||||||||||||||||||||||||||||||
| source.setStackTrace(clean); | |||||||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||||||
| StackTraceElement[] clean = new StackTraceElement[newTrace.size()]; | |||||||||||||||||||||||||||||||||
| newTrace.toArray(clean); | |||||||||||||||||||||||||||||||||
| source.setStackTrace(clean); | |||||||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||||||
| return source; | |||||||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||||||
|
|
|||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that
filter(source)is called again after the cause chain loop. Since the loop already filters thesourcethrowable (whencurrent == source), is the second call on line 89 necessary?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sanjana2505006 that's true about the old code, but after this PR, the early return makes the two calls mutually exclusive now.