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

Jump to bottom

Fix request on error logger #45

Merged
merged 4 commits into from
Jan 28, 2018
Merged

Fix request on error logger #45

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2a4ba8a
Fix response 'finished' error logger
crimzo Jan 25, 2018
85f54a1
Modify test
crimzo Jan 25, 2018
e0b8036
Add support for responses emitting error event
natanavra Jan 25, 2018
200a75a
Change request error to be only server errors 5xx
Jan 28, 2018
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
6 changes: 3 additions & 3 deletions logger.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,16 @@ function pinoLogger (opts, stream) {
return loggingMiddleware

function onResFinished (err) {
this.removeListener('finish', onResFinished)
this.removeListener('error', onResFinished)
this.removeListener('finish', onResFinished)

var log = this.log
var responseTime = Date.now() - this[startTime]

if (err) {
if (err || this.err || this.statusCode >= 400) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be >= 500, not 400.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that you'd want to have logs of issues like: bad request, unauthorized, forbidden access

And besides, we want to log the request / response, and a 4xx is a request (client) error.

Copy link
Member

Choose a reason for hiding this comment

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

Non of them are errors.
This is a flow for error conditions.

@jsumners what do you think?

Copy link
Contributor Author

@natanavra natanavra Jan 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

One more point on my side, if I'd look at a logs dashboard, I'd be looking to see 4xx responses as errors. These can indicate a broken client side library, broken flow somewhere, etc.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like it got resolved before I came around. I'm fine with the current solution.

log.error({
res: this,
err: err,
err: err || this.err || new Error('failed with status code' + this.statusCode),
responseTime: responseTime
}, 'request errored')
return
Expand Down
16 changes: 16 additions & 0 deletions test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,22 @@ test('responseTime for errored request', function (t) {
var dest = split(JSON.parse)
var logger = pinoHttp(dest)

function handle (req, res) {
logger(req, res)
setTimeout(function () {
res.err = new Error('Some error')
res.emit('finished')
res.end()
}, 100)
}

expectResponseTime(t, dest, logger, handle)
})

test('responseTime for request emitting error event', function (t) {
var dest = split(JSON.parse)
var logger = pinoHttp(dest)

function handle (req, res) {
logger(req, res)
setTimeout(function () {
Expand Down