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

Pass the error object to log #1246

Merged
merged 3 commits into from
Aug 4, 2024
Merged

Pass the error object to log #1246

merged 3 commits into from
Aug 4, 2024

Conversation

ikonst
Copy link
Contributor

@ikonst ikonst commented Jul 18, 2024

The current code uses suppressStack to determine what is logged, while I think suppressStack was only intended to control whether we expose internals to the caller.

It would be preferable if the logging mechanism could see the original error for what it is, since then it could use the appropriate logging mechanism in the code base; for example, pass the error object to Sentry, or log it in a structured way (name and stack separately) for Datadog to consume, etc.

@w666
Copy link
Collaborator

w666 commented Jul 19, 2024

Hi @ikonst,

Could you please add a test for this? Change looks good me.

@w666 w666 self-requested a review July 19, 2024 08:08
@ikonst
Copy link
Contributor Author

ikonst commented Jul 19, 2024

@w666 take a look ^

@ikonst ikonst force-pushed the pass-error-to-log branch from 4e9d23b to 35e4a6a Compare July 20, 2024 22:07
test/server-test.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@w666 w666 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor comment, otherwise looks good to me.

@w666
Copy link
Collaborator

w666 commented Jul 21, 2024

Approved, thanks for the improvement, I will include it into next release.

Please let me know if you need these changes asap, will see if I can make a release earlier.

@ikonst
Copy link
Contributor Author

ikonst commented Jul 21, 2024

Thank you. There's no urgency.

@w666 w666 merged commit 8e0853a into vpulim:master Aug 4, 2024
1 check passed
@ikonst ikonst deleted the pass-error-to-log branch August 5, 2024 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants