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

improves logging for explicitly handled errors #694

Merged
merged 7 commits into from
Feb 23, 2021

Conversation

mattoberle
Copy link
Contributor

This pull request incorporates elements of #693 with a few additional changes:

  1. I rebased the branch against origin/master to handle merge conflicts for existing logging updates.
  2. I removed logging in the composer.js when an error has a .status (leaving the logging up to the error handler in responses.js).
  3. I added a method to log errors with an explicit .status property at the warn (4xx) or error (5xx) level.

Change (1) pulls in a previous commit that added URLs to response logs.
Change (2/3) will allow dynamic routes to return 404s/400s without logging at the error level.

Tiffachow and others added 5 commits February 23, 2021 12:47
Amphora provides a mechanism to supply an error status code and message
in a response. This is accomplished by setting the following properties
on an **Error**:

```js
const err = new Error('Invalid Request');

err.status = 400;
err.message = 'Invalid Request';

throw err;
```

Errors thrown in a model's `render` or `save` will make their way to
the main error handler defined in `lib/responses.js`.

Prior to this commit, those errors were not logged, leaving the logging
up to the implementer.
@mattoberle mattoberle requested a review from jpope19 February 23, 2021 18:40
@mattoberle
Copy link
Contributor Author

@Tiffachow PR I incorporated your commits from #693 and combined them with some additional logging changes that were requested here. Since #693 was opened from an older fork there were conflicts with earlier changes that added url context to each response log.

lib/responses.js Outdated Show resolved Hide resolved
lib/responses.js Outdated Show resolved Hide resolved
lib/bootstrap.test.js Outdated Show resolved Hide resolved
Copy link
Contributor

@Tiffachow Tiffachow left a comment

Choose a reason for hiding this comment

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

Thanks matt looks good to me

This commit is unrelated to the primary purpose of this branch, but the
unused variable is causing the linter to fail.
@mattoberle mattoberle merged commit 97386a1 into master Feb 23, 2021
@mattoberle mattoberle deleted the logging-improvements branch February 23, 2021 21:44
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.

4 participants