Skip to content
This repository has been archived by the owner. It is now read-only.

feat(logs): log endpoint errors for better debugging #1627

Merged
merged 2 commits into from Jan 26, 2017
Merged

Conversation

@vladikoff
Copy link
Contributor

@vladikoff vladikoff commented Jan 26, 2017

No description provided.

@vladikoff
Copy link
Contributor Author

@vladikoff vladikoff commented Jan 26, 2017

@rfk r?

@vladikoff vladikoff requested a review from rfk Jan 26, 2017
@vladikoff
Copy link
Contributor Author

@vladikoff vladikoff commented Jan 26, 2017

Logs as:

{"Timestamp":1485407196029000000,"Logger":"fxa-auth-server","Type":"server.EndpointError","Severity":4,"Pid":17036,"EnvVersion":"2.0","Fields":{"op":"server.EndpointError","message":"127.0.0.1:8000 error: request timed out","reason":"socket hang up","method":"PUT"}}

You can see what the EndpointError provides here VS what we send back to the user:

image

function logEndpointErrors(response, log) {
// When requests to DB timeout and fail for unknown reason they are an 'EndpointError'.
// The error response hides error information from the user, but we log it here
// to better understand the DB timeouts.

This comment has been minimized.

@philbooth

philbooth Jan 26, 2017
Contributor

Purely a naming thing but, in light of this comment, would something like DatabaseError or TimeoutError be a more accurate name for these?

This comment has been minimized.

@vladikoff

vladikoff Jan 26, 2017
Author Contributor

hmm maybe, cases against the suggested though:

DatabaseError -> not exactly because customs server requests also use a pool, there is no "Database" there

TimeoutError -> not exactly because the pool can throw for different reasons.

It could be RequestError ?

This comment has been minimized.

@philbooth

philbooth Jan 26, 2017
Contributor

Leave it as it is in that case I reckon, I was just going from the comment. I don't think there's much to choose between EndpointError and RequestError. 👍

describe('lib/server', () => {
describe('trimLocale', () => {
it('trims given locale', () => {
assert.equal(server._trimLocale(' fr-CH, fr;q=0.9 '), 'fr-CH, fr;q=0.9')

This comment has been minimized.

@vbudhram

vbudhram Jan 26, 2017
Contributor

Mor coverage 👍

// log the DB attempt to understand the action
endpointLog.method = response.attempt.method
}
log.warn(endpointLog)

This comment has been minimized.

@vbudhram

vbudhram Jan 26, 2017
Contributor

Is warn the right level here, or should it be error?

This comment has been minimized.

@vladikoff

vladikoff Jan 26, 2017
Author Contributor

yea!

This comment has been minimized.

@vladikoff

vladikoff Jan 26, 2017
Author Contributor

what i mean is yea! error is probably better here...

@vladikoff vladikoff force-pushed the log-endpoint-err branch from c30bccb to 829f2b8 Jan 26, 2017
@vladikoff vladikoff merged commit 3719437 into master Jan 26, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@vladikoff vladikoff deleted the log-endpoint-err branch Jan 26, 2017
@rfk rfk added this to the FxA-0: quality milestone Feb 6, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants