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.

Sign up for GitHub

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

Log expected api errors as warning instead of error #5285

Merged
merged 2 commits into from
Mar 21, 2023
Merged

Log expected api errors as warning instead of error #5285

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cd2ec2e
Log expected api errors as warning instead of error
nflaig Mar 20, 2023
3528945
Log all api errors as warnings
nflaig Mar 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion packages/beacon-node/src/api/rest/base.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,12 @@ export class RestApiServer {
if (err instanceof ErrorAborted || err instanceof NodeIsSyncing) return;

const {operationId} = res.context.config as RouteConfig;
this.logger.error(`Req ${req.id} ${operationId} error`, {}, err);

if (err instanceof ApiError && err.statusCode < 500) {
nflaig marked this conversation as resolved.
Show resolved Hide resolved
nflaig marked this conversation as resolved.
Show resolved Hide resolved
this.logger.warn(`Req ${req.id} ${operationId} failed`, {reason: err.message});
Copy link
Member Author

@nflaig nflaig Mar 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@wemeetagain I want to establish this pattern that for non-critical issues we use the terms "failed" and "reason" instead of "error" as based on my observations the term "error" scares a lot of users, especially less technical ones. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

👍 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

i think the stack trace is very useful, but we can move the log to debug will full trace, an additional warn log in this format is acceptable

Copy link
Member Author

Choose a reason for hiding this comment

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

but we can move the log to debug will full trace, an additional warn log in this format is acceptable

that's also what I was considering, sounds like a reasonable compromise. Not sure if it makes sense here though in case of ApiError, if someone feels strongly about having a stack trace for those we should probably log it as well to debug

} else {
this.logger.error(`Req ${req.id} ${operationId} error`, {}, err);
}
metrics?.errors.inc({operationId});
});

Expand Down