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

fix: undefined error property in json logging #1564

Merged
merged 1 commit into from
Dec 1, 2022
Merged

Conversation

dcroote
Copy link
Contributor

@dcroote dcroote commented Nov 30, 2022

Closes #1547. Relatively simple fix as the bug appears to just have been a typo of copying the wrong line i.e. the code block several lines above this change has the correct conditional.

@dcroote dcroote requested a review from a team November 30, 2022 05:51
Copy link
Contributor

@amarthadan amarthadan left a comment

Choose a reason for hiding this comment

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

It does fix the unhandled error but since the error occurred, it means that the logFull function was at some point called with level set to ERROR and options.error being null. This way we will just ignore that call, won't we?

@dcroote
Copy link
Contributor Author

dcroote commented Dec 1, 2022

It does fix the unhandled error but since the error occurred, it means that the logFull function was at some point called with level set to ERROR and options.error being null. This way we will just ignore that call, won't we?

There are some instances where we do log errors without an error, for example:

const log = logger.pend('ERROR', `${baseLogMsg} failed after ${durationMs}ms`);

But that is somewhat beside the point here as the changed code block corresponds to providing an additional log when there is a stack available. If you look one line above the changed line, the error will still be logged in the case where there isn't an error:

json(level, message, options);
if (level === 'ERROR' && (options as ErrorLogOptions).error?.stack) {
json('ERROR', (options as ErrorLogOptions).error!.stack!, options);

@amarthadan
Copy link
Contributor

Oh, right, must have missed that, sorry. Carry on 🙂

@dcroote dcroote merged commit 06d5d11 into master Dec 1, 2022
@dcroote dcroote deleted the dcroote/issue1547 branch December 1, 2022 06:57
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.

Unhandled error in logging when the API call fails
2 participants