codeconsole commented on code in PR #15564:
URL: https://github.com/apache/grails-core/pull/15564#discussion_r3245668955
##########
grails-web-mvc/src/main/groovy/org/grails/web/errors/AuditorAwareLookup.java:
##########
Review Comment:
Good catch — the `boolean` return on `resolve()` was dead. Changed it to
`void` and gated the caller on `bean == null` instead, which removes the
misleading `!resolve() || bean == null` check. See f5d28ec50b.
Appreciate the Groovy rewrite — left it as Java since the rest of
`grails-web-mvc/.../errors/*.java` is Java and the reflective lookup doesn't
really benefit from `@Lazy` / Groovy sugar here. Happy to revisit if you'd
rather standardize on Groovy in this package.
##########
grails-core/src/main/groovy/grails/config/Settings.groovy:
##########
@@ -292,6 +292,41 @@ interface Settings {
* The parameters to exclude from logging
*/
String SETTING_EXCEPTION_RESOLVER_PARAM_EXCLUDES =
'grails.exceptionresolver.params.exclude'
+ /**
+ * Whether the exception resolver should also emit the exception on the
separate
+ * {@code StackTrace} logger in addition to its own request-context log
entry.
+ * Defaults to {@code false}; set to {@code true} to restore the
historical two-logger
+ * behaviour, which allows routing the trace to a separate appender via
logback config.
+ */
+ String SETTING_LOG_FULL_STACKTRACE =
'grails.exceptionresolver.logFullStackTrace'
Review Comment:
Yeah, agreed — I went lowercase to match the existing
`grails.exceptionresolver.*` keys on this interface
(`grails.exceptionresolver.params.exclude`,
`grails.exceptionresolver.logRequestParameters`), so the four new keys
(`logFullStackTrace`, `logAuditor`, `logRemoteAddr`,
`logFullStackTraceOnFilter`) stay consistent with that prefix. Happy to look at
a repo-wide camelCase normalization in a separate PR if we want it.
##########
grails-doc/src/en/guide/conf/config/logging/loggingFullStackTraces.adoc:
##########
@@ -0,0 +1,212 @@
+////
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+https://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+////
+
+When a request-handling exception reaches `GrailsExceptionResolver`, Grails
emits a single log record to the
+`org.grails.web.errors.GrailsExceptionResolver` logger at `ERROR` level. That
record contains the HTTP method and
+request URI alongside a _filtered_ stack trace — framework internals such as
`java.lang.reflect`, `jakarta.servlet`,
+`org.codehaus.groovy.runtime`, and similar dispatch/plumbing frames are
trimmed out so that application code is the
+first thing visible on the trace.
+
+For most operational use the filtered trace is what you want: it surfaces the
application frames that actually matter
+and keeps the log readable. For cases that need the untrimmed trace — routing
the raw frames to a separate audit
+file, correlating with an APM tool, or debugging dispatcher internals — Grails
exposes an opt-in:
+
+[source, yaml]
+.grails-app/conf/application.yml
+----
+grails:
+ exceptionresolver:
+ logFullStackTrace: true
+----
+
+The setting defaults to `false`.
+
+When enabled, Grails emits an additional log record to a dedicated logger
named `StackTrace`, containing the
+_unfiltered_ stack trace as it was captured at throw time. This record is
written _before_ the filter step runs,
+so no frames are lost. Each request-handling exception therefore produces two
log records:
+
+* A `StackTrace` logger record with the full, unfiltered trace (header `Full
Stack Trace:`).
+* A `GrailsExceptionResolver` logger record with the filtered trace and the
request-context headline
+(`<ExceptionType> occurred when processing request: [<METHOD>] <uri>`).
+
+The two records carry different trace content — that is the value of the
opt-in. The `StackTrace` record includes
+every reflection, dispatch, and servlet frame; the `GrailsExceptionResolver`
record shows only application code.
+
+==== Routing the StackTrace Logger To a Separate Appender
+
+Enabling `logFullStackTrace` alone will cause the unfiltered trace to appear
on whatever appender the root logger
+is configured with — typically the console — in addition to the filtered trace
from the resolver logger. If that is
+not what you want, pair the opt-in with a Logback configuration that routes
the `StackTrace` logger to its own
+appender and disables additivity so it does not bubble up to the root:
+
+[source, xml]
+.grails-app/conf/logback.xml
+----
+<appender name="STACK_FILE" class="ch.qos.logback.core.FileAppender">
+ <file>logs/stacktraces.log</file>
+ <encoder>
+ <pattern>%date %msg%n%ex%n</pattern>
+ </encoder>
+</appender>
+
+<logger name="StackTrace" level="ERROR" additivity="false">
+ <appender-ref ref="STACK_FILE"/>
+</logger>
+----
+
+With this configuration the unfiltered trace is written only to
`logs/stacktraces.log`, which you can rotate and
+retain independently. The console continues to show the concise, filtered
resolver record as before.
+
+==== Suppressing the StackTrace Logger
+
+Setting the `StackTrace` logger level to `OFF` in Logback makes the opt-in a
no-op, regardless of the
+`logFullStackTrace` property value:
+
+[source, xml]
+.grails-app/conf/logback.xml
+----
+<logger name="StackTrace" level="OFF"/>
+----
+
+This is useful if the property is enabled in a shared `application.yml` but a
particular environment needs to
+silence the extra record.
+
+==== Side-Effect Emission From the Filterer
+
+In addition to the resolver-driven emission described above,
`DefaultStackTraceFilterer.filter(Throwable)` emits
+the unfiltered stack trace to the `StackTrace` logger as a side effect
_before_ trimming the trace in place.
+This preserves the pre-7.1 behaviour where any caller of the filterer —
`GrailsExceptionResolver`,
+`GroovyPageView.deepSanitize`, `GrailsUtil.sanitizeRootCause`, or custom
plugin code — produced a `StackTrace`
+log record. It means non-resolver code paths (for example, a scheduled job
that calls
+`GrailsUtil.sanitizeRootCause(ex)` before logging via its own logger) continue
to populate the `StackTrace`
+appender without an explicit emission call.
+
+The behaviour is enabled by default. To disable the side-effect emission and
rely solely on
+`logFullStackTrace` for resolver-driven output, set:
+
+[source, yaml]
+.grails-app/conf/application.yml
+----
+grails:
+ exceptionresolver:
+ logFullStackTraceOnFilter: false
+----
+
+When the side-effect emission is enabled and `logFullStackTrace` is also
enabled, a request exception produces
+both — one resolver-driven record (top-level exception with the full cause
chain) and one record per throwable
+visited by the recursive filter walk. This matches pre-7.1 multiplicity. Pick
the combination that suits your
+log-routing setup:
+
+[cols="1,1,3"]
+|===
+|`logFullStackTrace` |`logFullStackTraceOnFilter` |Behaviour for a request
exception with N causes
+
+|`false` (default)
+|`true` (default)
+|N `StackTrace` records (one per throwable in the chain) + 1 resolver record.
Matches pre-7.1 (minus a former
+duplicate "condensed" record).
+
+|`true`
+|`false`
+|1 `StackTrace` record (top-level with full chain) + 1 resolver record.
Cleanest output; recommended for new
+deployments that don't depend on per-cause emission.
+
+|`true`
+|`true`
+|N+1 `StackTrace` records + 1 resolver record. Highest fidelity, most verbose.
+
+|`false`
+|`false`
+|1 resolver record only. Silent on `StackTrace`.
+|===
+
+==== Including Per-Request Context in the Exception Log
+
+The resolver can append a small parenthesised clause of per-request context —
the current user and the remote
+client address — to the exception headline:
+
+----
+RuntimeException occurred when processing request: [GET] /admin/users/42 (ip:
198.51.100.42, user: alice)
+Stacktrace follows:
+----
+
+When both pieces of context apply they share a single clause (`(ip: …, user:
…)`); when only one applies the
+clause collapses to that one entry. When neither applies, the headline is
unchanged.
+
+===== Current User
+
+When the application has registered an `AuditorAware` bean (see the GORM audit
timestamps section), the resolver
+calls `getCurrentAuditor()` on that bean and includes ` user: <value>` in the
clause, reusing the same auditor
+resolution that populates `@CreatedBy` and `@LastModifiedBy` fields. This
avoids registering a second "current
+user" bean purely for logging.
+
+The appended value is the `toString()` of whatever the `AuditorAware`
implementation returns — typically a
+username string or a numeric id, depending on the type parameter chosen for
the bean. If no `AuditorAware` bean
+is registered, or if it returns `Optional.empty()` for the current request
(for example, an unauthenticated
+request), no `user:` entry is emitted.
+
+Enabled by default. To keep user identifiers out of exception logs — for
example to satisfy a PII separation
Review Comment:
Good call. Flipped the default to `false` in e4b96a83bc — same PII reasoning
as `logRemoteAddr`. Apps that want the appended ` user: <value>` clause now opt
in with:
```yaml
grails:
exceptionresolver:
logAuditor: true
```
Updated `loggingFullStackTraces.adoc` ("Disabled by default…") and the
section 2.12 blurb in `upgrading71x.adoc` to match. With both `logAuditor` and
`logRemoteAddr` off-by-default, the headline collapses to its pre-7.1 form
unless the app explicitly opts in.
##########
grails-web-mvc/src/main/groovy/org/grails/web/errors/GrailsExceptionResolver.java:
##########
@@ -261,6 +266,70 @@ protected void logStackTrace(Exception e,
HttpServletRequest request) {
LOG.error(getRequestLogMessage(e, request), e);
}
+ /**
+ * When the {@code grails.exceptionresolver.logFullStackTrace} property is
enabled,
+ * emits the unfiltered stack trace to the dedicated {@code StackTrace}
logger.
+ * Must be invoked <em>before</em> {@link #filterStackTrace(Exception)} —
once the
+ * filterer calls {@code setStackTrace(clean)}, the original frames are
gone and
+ * this method can only log the already-trimmed trace.
+ */
+ protected void logFullStackTraceIfEnabled(Exception e) {
+ if (shouldLogFullStackTrace()) {
+
DefaultStackTraceFilterer.STACK_LOG.error(StackTraceFilterer.FULL_STACK_TRACE_MESSAGE,
e);
+ }
+ }
+
+ protected boolean shouldLogFullStackTrace() {
+ Config config = grailsApplication != null ?
grailsApplication.getConfig() : null;
+ return config != null &&
config.getProperty(Settings.SETTING_LOG_FULL_STACKTRACE, Boolean.class, false);
+ }
+
+ protected boolean shouldLogAuditor() {
+ Config config = grailsApplication != null ?
grailsApplication.getConfig() : null;
+ return config != null &&
config.getProperty(Settings.SETTING_LOG_AUDITOR, Boolean.class, true);
+ }
+
+ protected boolean shouldLogRemoteAddr() {
+ Config config = grailsApplication != null ?
grailsApplication.getConfig() : null;
+ return config != null &&
config.getProperty(Settings.SETTING_LOG_REMOTE_ADDR, Boolean.class, false);
+ }
Review Comment:
Fair — config doesn't change at runtime so re-reading it per resolved
exception is wasteful. Resolved in e4b96a83bc: added a private
`resolveLogFlags()` (double-checked, `volatile boolean logFlagsResolved`) that
reads the three properties on first use and caches them into `boolean` fields.
The three `shouldLog*` predicates now just call `resolveLogFlags()` and return
the cached value, so subsequent exceptions pay only a volatile read.
Kept the methods as `protected` so subclasses overriding any `shouldLog*`
predicate bypass the cache entirely, same as before.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
