Skip to content
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

Add ability to use passed in Errors for callstacks and adjust how deeply you want to look for information #1269

Merged
merged 10 commits into from Sep 29, 2022

Conversation

ZachHaber
Copy link
Contributor

@ZachHaber ZachHaber commented Jun 17, 2022

closes #1268

New behavior:

logger.info(new Error()) will parse the error instead of creating a new one.

logger.callStackLinesToSkip = 1 will look one level deeper than the base for Error values.

logger.setParseCallStackFunction(undefined) will reset it to default (instead of throwing an error the next time it logs).

logger.setParseCallStackFunction('') will throw an error immediately, rather than throw the next time you log.

@lamweili
Copy link
Contributor

lamweili commented Jun 18, 2022

In e7ea403, deca3fb and b4e8a27, it is hard to identify and review the surgical changes.
Those style/lint fixes in those three commits cloud the changes and will also bear the seemingly non-related commit message.

In the future, please try to separate the style/lint issues a commit by itself (probably the last commit) for a hastier merge. 🤗

I would need some time to review through, or if you could split/rebase them, it will be greatly appreciated!

@ZachHaber
Copy link
Contributor Author

Sorry about that...It just sort of happens because I have prettier set up on save to make my life easier normally.
I won't have access to my computer until Monday to make the change.

@lamweili
Copy link
Contributor

lamweili commented Jun 22, 2022

Hi @ZachHaber, I merged your PR #1271 that addressed the prettier.

You can rebase and do a force push to this same branch to update the commits for this PR.
That will make the commit diff/changes more surgical and make review much easier. Thanks!

@ZachHaber
Copy link
Contributor Author

@peteriman, this branch is now rebased! Definitely a much smaller set of changes :)

@ZachHaber
Copy link
Contributor Author

@peteriman, where did we land on this, along with the logfacesHTTP adapter checking only the first data value to see if it's an error?

We could instead adjust this to loop through to find the first Error object instead, if we do that, I think it would make sense to surface that in the base LoggingEvent as LoggingEvent.error even without enableCallStack to prevent having to do that lookup multiple times depending on the adapter.

@lamweili
Copy link
Contributor

lamweili commented Jun 23, 2022

@ZachHaber Hi there! My apologies that I haven't been available to take a look at this yet.
I'm not exactly sure about the current behaviour to be in a position to make a judgement on the new behaviour.

But certainly, what you suggested sounds logical and great. Don't let me hold you back!

@ZachHaber
Copy link
Contributor Author

I'll split that out into a separate branch/pr so we can decide on behavior afterwards more easily :)

use the first data value if an Error to get callstack info. Only 1 line will be stripped from it by default

Refs: log4js-node#1268
`logger.callStackLinesToSkip = 2` will make it skip 2 extra lines above the base.

You can't skip negative lines

Refs: log4js-node#1268
if set to an invalid value, it'll throw an error immediately, instead of later.
@lamweili
Copy link
Contributor

Rebased to be merged.

@lamweili lamweili merged commit 916eef1 into log4js-node:master Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

useCallStack and Logging an Error object
2 participants