-
Notifications
You must be signed in to change notification settings - Fork 213
feat: disable error logging stacktrace in message #1705
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: master
Are you sure you want to change the base?
Conversation
d554ba6
to
d805557
Compare
d805557
to
243c6b1
Compare
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.
Pull Request Overview
Adds a new global option to disable automatic inclusion of error stacktraces in logs and updates the logger and tests to respect this setting.
- Introduce
disableErrorLoggingTraceback
inGlobalOptions
- Modify
entryFromArgs
to skip stacktrace when the option is enabled - Add tests for default stacktrace inclusion and disabling behavior
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/v2/options.ts | Add disableErrorLoggingTraceback to GlobalOptions with accompanying JSDoc |
src/logger/index.ts | Check getGlobalOptions().disableErrorLoggingTraceback before injecting stacktrace |
spec/logger.spec.ts | Tests for error logging behavior with and without the new option |
Comments suppressed due to low confidence (2)
src/v2/options.ts:241
- Fix the JSDoc wording to remove the double negative, e.g.
When true, the error message will not include the traceback of the error.
* When true, the error message will include not include the traceback of the error.
spec/logger.spec.ts:223
- Consider adding a test case where
logger.error
is called with an actualError
object to ensure that existing Error instances aren’t wrapped with a new stacktrace.
describe("error logging stacktrace", () => {
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.
Some minor changes/comments
src/v2/options.ts
Outdated
* @remarks | ||
* When true, the error message will include not include the traceback of the error. | ||
*/ | ||
disableErrorLoggingTraceback?: boolean; |
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 would use "stacktrace" instead of "traceback" as afaik traceback is mostly a python term?
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.
disableErrorStackTrace or suppressErrorStack or something?
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.
Just went with disableErrorLoggingStacktrace
, since it refers to logging. I feel like disableErrorStacktrace
isn't clear enough.
disableErrorLogStacktrace
could be an alternative?
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.
ya disableErrorLoggingStacktrace sounds good!
243c6b1
to
06e2743
Compare
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.
lgtm
/gemini review |
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.
Code Review
This pull request introduces a new global option disableErrorLoggingStacktrace
to control the automatic inclusion of stack traces in error logs. The implementation logic in the logger and the option definition are generally sound, but there are opportunities to improve test quality, code readability, and the default value of the new option.
beforeEach(() => { | ||
setGlobalOptions(defaultOptions); | ||
}); | ||
|
||
afterEach(() => { | ||
setGlobalOptions(defaultOptions); | ||
}); |
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.
* Defaults to false. | ||
* |
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.
Resolves #1681