feat(core): print cause of uncaught errors in their stack trace #13870
+93
−12
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Aug 9, 2024
|
There are 2 failing unit tests Thanks for your contribution! |
12 tasks
thibseisel
force-pushed
the
feature/exception-handler-error-cause
branch
2 times, most recently
from
e69f9ef
to
c3f54db
Compare
thibseisel
force-pushed
the
feature/exception-handler-error-cause
branch
from
c3f54db
to
4d37a0b
Compare
Pull Request Test Coverage Report for Build d4ce9edb-19ae-400b-bef2-ac4fb9aa2f87Details
💛 - Coveralls |
12 tasks
12 tasks
|
I've updated the test cases for the exception handler and submitted a PR for your review. I noticed an issue in the existing test code, so I made the necessary adjustments and included them in this PR as well. Currently, in the exception-handler.spec.ts file, the second test case seems to be designed for a scenario where the exception is not a nest/packages/core/test/errors/test/exception-handler.spec.ts Lines 28 to 32 in 38c0548 However, the To better match the intended test scenario, I have modified the exception variable to be an instance of @thibseisel Please take a look at my PR as well. Thanks. :) |
|
@jochongs I just accepted your PR, thank you! Also, I'd like to emphasize that unlike my initial implementation, this change no longer displays custom properties of an Error, only its chain of causes. This is because I decided to write a custom |
12 tasks
|
I’ve been thinking about the decision to print only the (I couldn’t find the original commit where |
rubiin
approved these changes
Aug 12, 2024
|
I'm somewhat leaning toward printing all attributes too. Is there any specific reason we should consider printing only the |
|
@jochongs @kamilmysliwiec I omitted custom properties essentially to reduce verbosity. String properties are rather short, but some deep objects could make the stack-trace unreasonably long (I'm pointing at you, Node.js streams!). I'm not against the idea though. Some custom errors have useful properties, for example |
|
Hello, I've been developing with Nest.js recently and had some thoughts that I wanted to share here. For smaller-scale servers, I think the default logger could work well even in production. Since it only logs unknown errors, it prevents excessive log accumulation and reduces development effort. Considering issue #13841, it seems reasonable to use the default logger in production environments as well. If the above premise holds, one potential concern is that the default logger might log all properties of error objects, which could be overwhelming for some errors with numerous properties, as shown in the example below. Example of an error log with all properties includedThis error occurred in the httpService that we frequently use. await this.httpService.axiosRef.post('https://axios-error.test/not-found');However, in development, logging all properties is highly convenient. How about using an environment variable like NODE_ENV to control this behavior? For instance, we could log all properties in development but limit it to just the stack trace in production. How about modifying the code in this link so it logs only in development mode? if (this.isExceptionObject(exception)) {
// Display all error properties only in development mode
if (process.env.NODE_ENV === 'development') {
return BaseExceptionFilter.logger.error(
exception.message,
inspect(exception, false, null),
);
}
return BaseExceptionFilter.logger.error(
exception.message,
combineStackTrace(exception),
);
}May I ask your thoughts on this? I primarily run Nest applications in container environments like ECS, where I review error logs directly in the terminal or via ECS Service log tab. In cases where a dedicated log management infrastructure like ELK or PLG isn't set up, I found the default logger to be quite sufficient for production as well. It would be great to have the option to use the default logger effectively even in production. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #13550
When an uncaught error is thrown, NestJS handles it using the configured
ExceptionFilter. If the default filter is not overridden, the error is logged under theExceptionsHandlertag with its message and stack trace.Since ECMAScript 2022, the
Errorconstructor accepts a newcauseparameter option. The cause of anErroris printed to the console when passed toconsole.error:But since NestJS only considers the caught error's stack trace, it does not include the root causes, making it hard to diagnose issues. The above example would only print something like the following:
What is the new behavior?
When an unexpected error occurs, NestJS now recursively logs error causes. This only focuses on error causes and does not display any custom property error objects may have.
The above example would generate the following stack trace:
Does this PR introduce a breaking change?
This change makes sure that stack trace of errors without a cause are logged exactly the same way as before.
The only impact is that logged error stack traces may be longer. Deeply nested errors may cause logs to be less readable.
Other information
None.