matrei commented on code in PR #15564:
URL: https://github.com/apache/grails-core/pull/15564#discussion_r3224896626
##########
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:
It would have been nice with `grails.exceptionResolver.logFullStackTrace`,
but I agree with keeping consistency.
##########
grails-web-mvc/src/main/groovy/org/grails/web/errors/AuditorAwareLookup.java:
##########
Review Comment:
I found the logic a bit hard to grasp as the `resolve()` method always
returns `true`.
Using Groovy we could also abstract away some boilerplate:
```groovy
package org.grails.web.errors
import java.lang.reflect.Method
import groovy.transform.CompileStatic
import groovy.transform.PackageScope
import groovy.util.logging.Slf4j
import org.springframework.beans.BeansException
import org.springframework.context.ApplicationContext
import org.springframework.util.ClassUtils
/**
* Optional integration with the GORM {@code AuditorAware} bean. When the
audit API
* class is on the classpath and a bean is registered, {@link
AuditorAwareLookup#getCurrentAuditor()}
* returns its result so the exception resolver can reuse the same "current
user"
* resolution that {@code @CreatedBy} uses. The lookup is reflective so that
this
* module does not compile-time depend on {@code grails-datamapping-core}.
*
* <p>Any failure — missing class, missing bean, invocation error — resolves
to an
* empty {@code Optional}. Exception logging must never be blocked by a
broken
* auditor lookup.</p>
*/
@Slf4j
@PackageScope
@CompileStatic
class AuditorAwareLookup {
private static final String AUDITOR_AWARE_CLASS =
'org.grails.datastore.gorm.timestamp.AuditorAware'
private final ApplicationContext applicationContext
@Lazy volatile
private AuditorLookup auditorLookup = lookupAuditor()
AuditorAwareLookup(ApplicationContext applicationContext) {
this.applicationContext = applicationContext
}
Optional<?> getCurrentAuditor() {
auditorLookup.currentAuditor
}
private AuditorLookup lookupAuditor() {
try {
def classLoader = applicationContext?.classLoader
if (!classLoader || !ClassUtils.isPresent(AUDITOR_AWARE_CLASS,
classLoader)) {
return AuditorLookup.empty()
}
def type = ClassUtils.forName(AUDITOR_AWARE_CLASS, classLoader)
try {
return new AuditorLookup(
applicationContext.getBean(type),
type.getMethod('getCurrentAuditor')
)
}
catch (BeansException ignored) {
return AuditorLookup.empty()
}
}
catch (Throwable t) {
log.debug('AuditorAware integration unavailable', t)
return AuditorLookup.empty()
}
}
@CompileStatic
private static class AuditorLookup {
private final Object auditorAwareBean
private final Method getCurrentAuditorMethod
AuditorLookup(Object auditorAwareBean, Method
getCurrentAuditorMethod) {
this.auditorAwareBean = auditorAwareBean
this.getCurrentAuditorMethod = getCurrentAuditorMethod
}
static AuditorLookup empty() {
new AuditorLookup(null, null)
}
Optional<?> getCurrentAuditor() {
if (auditorAwareBean == null || getCurrentAuditorMethod == null)
{
return Optional.empty()
}
try {
def result = getCurrentAuditorMethod.invoke(auditorAwareBean)
return result instanceof Optional ? (Optional<?>) result :
Optional.empty()
}
catch (ReflectiveOperationException e) {
log.debug('AuditorAware#getCurrentAuditor invocation
failed', e)
return Optional.empty()
}
}
}
}
```
##########
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:
Do we need to resolve these on every invocation?
##########
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:
I think this should be disabled by default. Won't we risk logging email
addresses otherwise? Same GDPR problem as IP-addresses.
--
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]
