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

Feature/logger #212

Merged
merged 9 commits into from
Jan 20, 2021
Merged

Feature/logger #212

merged 9 commits into from
Jan 20, 2021

Conversation

justinkaseman
Copy link
Member

@justinkaseman justinkaseman commented Jan 6, 2021

Description

Node operator's log files have been growing very large. The Coin Paprika adapter for example had a >100Gb logfile.

This PR removes the log emitted when a response is returned to a request. Logs will still be shown on error or when LOG_LEVEL="debug".

Additionally some other logs that should probably be better served to debug level logs are changed to be so.

Ticket

#176137608

@github-actions
Copy link
Contributor

github-actions bot commented Jan 6, 2021

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

@justinkaseman
Copy link
Member Author

Addresses #204 that was experienced when using the Node.js Error class:

throw new Error(xxxx)

external-adapter/src/requester.ts Outdated Show resolved Hide resolved
external-adapter/src/requester.ts Outdated Show resolved Hide resolved
bootstrap/src/index.ts Outdated Show resolved Hide resolved
bootstrap/src/index.ts Outdated Show resolved Hide resolved
@justinkaseman
Copy link
Member Author

@krebernisak Left some reasoning on why I made certain changes

@RodrigoAD
Copy link
Member

I've been playing with it, this is great. Just something I noticed, when there's an internal error, this is sent to the client as well. I don't know if this is the behavior we want

@justinkaseman
Copy link
Member Author

I've been playing with it, this is great. Just something I noticed, when there's an internal error, this is sent to the client as well. I don't know if this is the behavior we want

I was wondering about which behaviour we want there too. My thought process is that we are open source and want to empower people to debug on their own. @krebernisak thoughts? This is related to that first change that you requested.

@krebernisak
Copy link
Contributor

krebernisak commented Jan 13, 2021

My thoughts are that we should handle most/all of errors, output them with correct status codes and appropriate messages. We have work to do here.

Internal errors should be caught, mapped to a HTTP 500 status code and construct an appropriate message. Something like Internal Server Error: ${error.name}. Then if we are in dev env (NODE_ENV=development) or some other verbose/debug flag is set, we dump the whole internal error on the output, as an underlying cause to be investigated.

@justinkaseman
Copy link
Member Author

justinkaseman commented Jan 14, 2021

My thoughts are that we should handle most/all of errors, output them with correct status codes and appropriate messages. We have work to do here.

Internal errors should be caught, mapped to a HTTP 500 status code and construct an appropriate message. Something like Internal Server Error: ${error.name}. Then if we are in dev env (NODE_ENV=development) or some other verbose/debug flag is set, we dump the whole internal error on the output, as an underlying cause to be investigated.

@krebernisak Well said for a direction forward. Agreed that there is more work to do here with what response the user receives.

I reverted the code that had requested changes, leaving this PR to be:

  • adding the verbose/debug internal error dump
  • and setting most logs to be verbose/debug level to keep node operator log files small.

Copy link
Contributor

@krebernisak krebernisak left a comment

Choose a reason for hiding this comment

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

Thanks, @justinkaseman!

@justinkaseman justinkaseman merged commit 03c01b7 into develop Jan 20, 2021
@justinkaseman justinkaseman deleted the feature/logger branch January 20, 2021 17:39
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.

None yet

3 participants