Skip to content

fix: Improve error logging for axios errors#120

Merged
dipasqualew merged 6 commits intomasterfrom
ENG-115-improve-error-logging-for-axios-errors
Dec 14, 2020
Merged

fix: Improve error logging for axios errors#120
dipasqualew merged 6 commits intomasterfrom
ENG-115-improve-error-logging-for-axios-errors

Conversation

@dipasqualew
Copy link
Copy Markdown
Contributor

@dipasqualew dipasqualew commented Dec 14, 2020

Axios errors are really verbose and local development terminals get spammed, especially if more than one test fail because of HTTP errors.

While handling an error, lambda wrapper should recognise axios errors and trim down the information to the request config, response.status and response.data

See:

@dipasqualew dipasqualew self-assigned this Dec 14, 2020
Comment thread src/Wrapper/LambdaWrapper.js Outdated
// if the axios error is incomplete
// for any reason, we don't want
// error handling to fail
errorBody = error;
Copy link
Copy Markdown
Contributor

@tomisaacmarshall tomisaacmarshall Dec 14, 2020

Choose a reason for hiding this comment

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

It's pretty common for axios errors to not have.response e.g. when there's a network error or timeout. These errors will have .request but not .response. So would probs be good to handle those cases without just using the full error.

Also, would be good to keep error.message too maybe?

Copy link
Copy Markdown
Contributor

@tomisaacmarshall tomisaacmarshall Dec 14, 2020

Choose a reason for hiding this comment

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

Oh and maybe some of the request info can be kept, might be useful in development? E.g. url, payload, headers, query parameters etc

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh and maybe some of the request info can be kept, might be useful in development? E.g. url, payload, headers, query parameters etc

They come with error.config

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's pretty common for axios errors to not have.response e.g. when there's a network error or timeout. These errors will have .request but not .response. So would probs be good to handle those cases without just using the full error.

Also, would be good to keep error.message too maybe?

Good points. On it!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice one. Good to know re .config

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Axios errors are really verbose and local development terminals get spammed, especially if more than one test fail because of HTTP errors.

While handling an error, lambda wrapper should recognise axios errors and trim down the information to the request config, response.status and response.data

See:
- [ENG-115]
@dipasqualew dipasqualew force-pushed the ENG-115-improve-error-logging-for-axios-errors branch from b08c322 to 11ac8d2 Compare December 14, 2020 13:43
@dipasqualew dipasqualew merged commit 2f4d5db into master Dec 14, 2020
@dipasqualew dipasqualew deleted the ENG-115-improve-error-logging-for-axios-errors branch December 14, 2020 14:01
@lebaz20
Copy link
Copy Markdown
Contributor

lebaz20 commented Dec 14, 2020

🎉 This PR is included in version 1.2.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants