Log errors in RestResponse regardless of error_trace parameter #101066
Conversation
Currently we only log exceptions in RestResponse when the request parameter "error_trace" isn't set or set to "false". If the client requests traces in the rest response, we skip the server side logging. In order to improve visibility of those errors in our logs this changes the logging behaviour to only depend on the "rest.exception.stacktrace.skip" which is only used internally. Closes elastic#100884
cbuescher
added
>enhancement
:Core/Infra/Core
|
Hi @cbuescher, I've created a changelog YAML for you. |
|
Pinging @elastic/es-search (Team:Search) |
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
fyi I left the "rest.exception.stacktrace.skip" condition in there around the logging block because I'm unsure how we internally use it today. Some research makes me think that we only ever change the global default of "rest.exception.stacktrace.skip == true" in overwrites to the XContent rendering parameter for certain object, e.g. like in . If that understanding is correct we might as well remove the logic here and in tests as well and only keep the parameter around for xcontent rendering purposes. |
There was a problem hiding this comment.
Left one question, an additional one if we go ahead with this change (and I think we should) is whether the logger should still be called rest.suppressed. it seems like we are changing its behaviour significantly that it would deserve a rename as a follow-up, or at least a general discussion about the long-term direction.
| @@ -115,7 +115,7 @@ public RestResponse(RestChannel channel, Exception e) throws IOException { | |||
| @SuppressWarnings("this-escape") | |||
| public RestResponse(RestChannel channel, RestStatus status, Exception e) throws IOException { | |||
| this.status = status; | |||
| ToXContent.Params params = paramsFromRequest(channel.request()); | |||
| ToXContent.Params params = channel.request(); | |||
| if (params.paramAsBoolean(REST_EXCEPTION_SKIP_STACK_TRACE, REST_EXCEPTION_SKIP_STACK_TRACE_DEFAULT) && e != null) { | |||
There was a problem hiding this comment.
where does this param come from? isn't its default true?
There was a problem hiding this comment.
It is used in ElasticsearchExceptionL391 to decide whether stack traces are rendered as part of the xContent output of an exception. Some xContent Object use it to overwrite the default with "false", e.g. ExceptionWatchRecordL318, ILMHistoryItemL113 and maybe others (I didn't do a full search). The important thing I think that it should never be set through a Rest request but might be switched from the default "true" to "false" in several places programmatically.
There was a problem hiding this comment.
Do we need still need this if check here?
params.paramAsBoolean(REST_EXCEPTION_SKIP_STACK_TRACE, REST_EXCEPTION_SKIP_STACK_TRACE_DEFAULT)
Do we want to check it only if status.getStatus() < 500 ?
There was a problem hiding this comment.
Reading this again more closely, REST_EXCEPTION_SKIP_STACK_TRACE is only ever set to "false" in GetFeatureUpgradeStatusResponse, ExceptionWatchRecord and ILMHistoryItem because it seems someone decided to always render exception stack traces inside those objects. That also means it should never be set here, so you are right this check can be removed. The presence of an exception is still something we'd need to check I think, also we want to log both 400/500s here so I don't see why we should condition on status here. I will push some changes.
| @@ -131,6 +131,9 @@ public RestResponse(RestChannel channel, RestStatus status, Exception e) throws | |||
| SUPPRESSED_ERROR_LOGGER.warn(messageSupplier, e); | |||
| } | |||
| } | |||
| if (params.paramAsBoolean("error_trace", false) && status != RestStatus.UNAUTHORIZED) { | |||
There was a problem hiding this comment.
I know you are just rearranging code for this case, but (if you understand it) could you add a code comment of what this is for and what it is doing? It's not clear to me.
|
@javanna @quux00 thanks for the additional review, I removed conditioning on the REST_EXCEPTION_SKIP_STACK_TRACE parameter in the RestResponse logging now since it cannot be different from the default value at least in this location. Its still used (and changed) for xcontent rendering so I'd like to keep it there for now. If we decide so we can also remove it there in a follow up. Let me know what you think. |
…ic#101066) Currently we only log exceptions in RestResponse when the request parameter "error_trace" isn't set or set to "false". If the client requests traces in the rest response, we skip the server side logging. In order to improve visibility of those errors in our logs this changes the logging behaviour to only depend on the "rest.exception.stacktrace.skip" which is only used internally. Closes elastic#100884
Currently, setting the "error_trace" request parameter to "true" leads to error stack traces being
included in the rest response, but will prevent logging them in the "rest.suppressed" logger in the
RestResponseclass.In order to improve visibility of those errors in our logs, this PR changes the logging behavior be
independent of the "error_trace" request parameter and only take the "rest.exception.stacktrace.skip"
parameter into account, which is only used internally.
Closes #100884