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

adds 'url' to log context for rendering errors #687

Merged
merged 2 commits into from
Sep 18, 2020
Merged

Conversation

mattoberle
Copy link
Contributor

@mattoberle mattoberle commented Sep 17, 2020

When Amphora throws an error in a location where req/res context is
available we don't currently include that context.

This commit includes url in the log context to make it easy to
reproduce errors.


According to the express docs res.locals is available by default and there is no risk of attempting to reference a property on an undefined object under normal use.

There may be more useful context available, but this seems like a good place to start.

For example, res.locals.user would log which user initiated an action (for internal requests using amphora-auth):
https://github.com/clay/amphora-auth/blob/94201d60a6043c968b507a66e7ab289ce4f0139b/index.js#L144-L153

@coveralls
Copy link

coveralls commented Sep 17, 2020

Pull Request Test Coverage Report for Build 2463

  • 4 of 4 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 2458: 0.0%
Covered Lines: 4197
Relevant Lines: 4197

💛 - Coveralls

Copy link
Member

@jjpaulino jjpaulino left a comment

Choose a reason for hiding this comment

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

🏄‍♂️

When Amphora throws an error in a location where req/res context is
available we don't currently include that context.

This commit includes 'url' in the log context to make it easy to
reproduce errors.
lib/responses.js Outdated
@@ -312,7 +312,7 @@ function unauthorized(res) {
* @param {object} res
*/
function clientError(err, res) {
log('error', err.message, { stack: err.stack });
log('error', err.message, { stack: err.stack, url: res.locals.url });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: Is a clientError really worthy of generating an error level log on the server side or should this be a warning?

Copy link
Contributor

@reubenson reubenson left a comment

Choose a reason for hiding this comment

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

lgtm, but would also be a bit safer to use lodash _get(res, 'locals.url')

@mattoberle
Copy link
Contributor Author

mattoberle commented Sep 18, 2020

lgtm, but would also be a bit safer to use lodash _get(res, 'locals.url')

@reubenson yeah I was thinking about that too.
It looks like Express sets res.locals so it should be there: https://expressjs.com/en/api.html#res.locals

The only situation I can think of where it wouldn't would be if someone added a middleware to wipe out that attribute or set it to a non-object (which seems like a mis-use of Express and maybe should result in a hard error?).

Edit: We already import lodash in these modules, might as well do the safer thing. It's probably worth keeping the side-effect code (ie. logging) as safe as possible.

While `res.locals` is an object created by Express, it's not impossible
to manually delete or override that attribute. Using `_.get(res,
'locals.url')` will provide some additional safety to the logging
mechanism in the event of misuse.
@mattoberle mattoberle merged commit 2401dd2 into master Sep 18, 2020
@mattoberle mattoberle deleted the error-context branch September 18, 2020 14:31
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.

5 participants