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

chore(errors): amend errors so user-input derived issues are info level logged, the error is logged in full, and other refactoring #2767

Draft
wants to merge 39 commits into
base: main
Choose a base branch
from

Conversation

iainsproat
Copy link
Contributor

@iainsproat iainsproat commented Aug 27, 2024

Description & motivation

All errors classes extended from BaseError define an http status code. This is defined regardless of whether the error is used in handling an http request as it allows the correct log level to be selected.

All generic throw new Error (in non-test, non-mock) is replaced with a more specific error class, extended from BaseError.

The graphql error logger has been updated to use the statusCode to define which error level is used in logging.

Changes:

  • the default error handler for express now ensures the error is included in the request context. The pino-http logger now ensures that the error from the request context is included in the logged message.
  • logging of graphql errors now determine which level an error should be logged at. User-input derived errors are logged as 'info', and internal system errors as 'error'.
  • status codes are added to all errors extended from BaseError.
  • fixes error in logging, where graphql_operation_value was expected but was not a parameter available in the structured log. Replaces with graphql_operation_name.
  • Moves common code between graphql and graphql subscriptions to a function shouldLogAsInfoLevel
  • Errors where stream, projects, invites, commits etc. cannot be found should result in a X_NOT_FOUND error code, which should be logged as info level.
  • introduces a ObjectNotFoundError. (TODO, complete this list)
  • replaces throw new Error with a specific class.
  • configuration of S3 uses the envHelpers.
  • Where error messages include dynamic context, these values are added to the options.info parameter when the error is constructed.

To-do before merge:

Screenshots:

Requests now log the full error message, whether logging as info or error:
Screenshot 2024-08-29 at 12 33 37

Validation of changes:

Checklist:

  • My pull request follows the guidelines in the Contributing guide?
  • My pull request does not duplicate any other open Pull Requests for the same update/change?
  • My commits are related to the pull request and do not amend unrelated code or documentation.
  • My code follows a similar style to existing code.
  • I have added appropriate tests.
  • I have updated or added relevant documentation.

References

@iainsproat iainsproat marked this pull request as ready for review August 28, 2024 09:05
@iainsproat iainsproat changed the title chore(graphql): amend errors so user-input derived issues are info level logged and 4XX status coded chore(graphql): amend errors so user-input derived issues are info level logged Aug 28, 2024
@iainsproat iainsproat marked this pull request as draft August 28, 2024 12:33
- a more appropriate error is used if possible
- FIXME and other notes added
- logging as info is determined on statusCode assigned to error
- info object on Error constructor is used where possible
- S3 config should use envHelper
packages/server/logging/graphqlError.ts Outdated Show resolved Hide resolved
packages/server/modules/auth/rest/index.js Outdated Show resolved Hide resolved
packages/server/modules/auth/rest/index.js Outdated Show resolved Hide resolved
packages/server/modules/blobstorage/services.js Outdated Show resolved Hide resolved
packages/server/modules/core/errors/user.ts Outdated Show resolved Hide resolved
packages/server/modules/pwdreset/rest/index.ts Outdated Show resolved Hide resolved
…ging metadata

- this allows us to remove try/catch blocks around Express endpoints which were handling logging of errors
@iainsproat iainsproat changed the title chore(graphql): amend errors so user-input derived issues are info level logged chore(errors): amend errors so user-input derived issues are info level logged, the error is logged in full, and other refactoring Aug 29, 2024
@iainsproat
Copy link
Contributor Author

didEncounterErrors method continues to log error level for graphql errors which should be info logged:
Screenshot 2024-09-07 at 11 07 13

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.

1 participant