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

Deployer logs file #1611

Merged
merged 15 commits into from
Jan 26, 2023
Merged

Deployer logs file #1611

merged 15 commits into from
Jan 26, 2023

Conversation

vponline
Copy link
Contributor

Closes #1458

@vponline vponline requested a review from a team January 12, 2023 10:06
@vponline vponline self-assigned this Jan 12, 2023
Copy link
Contributor

@amarthadan amarthadan left a comment

Choose a reason for hiding this comment

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

This issue could have been discussed/specified more. Sorry, that's on me and @aTeoke.

First of all, we need to allow users to change where the files should be stored. There should be a CLI option for that. Also, I'm not really a fan of the file names and the fact that we're storing everything in one (two) files.

I would propose having a logs directory. Users will be able to specify where it should be (the default being config/logs). In the directory, we will store the log files. Not just the two of them but two (or rather potentially two) for each deployer run, suffixing the name with a timestamp. Something like this:

├── logs
│   ├── deployer-2023-01-16_09:35:23.error.log
│   ├── deployer-2023-01-16_09:35:23.log
│   └── deployer-2023-01-16_09:50:15.log

I'm not 100% convinced that we actually need separate output and error log files though. Imagine debugging an issue. What good does it do if I have two different files and need to manually match timestamps from those logs to see what happened prior to the error? I would probably log it into the same file.

I'm of course open to discussing these design changes, this is just how I think it would work best.

I also run into a few issues while testing this.
As mentioned in the issue description, we should not log any sensitive information. Currently, you can find in the logs for example Airnode wallet private key or HTTP gateway.

Also, the debug mode is not working correctly. When the debug mode is enabled (--debug switch) most of the logs are written in the file twice. When the debug mode is not enabled, the logs written in the file are written as when the debug mode is enabled. This may be on purpose, I understand the intent, it's just a bit weird. I haven't seen such behavior anywhere else.

packages/airnode-deployer/src/utils/infrastructure.ts Outdated Show resolved Hide resolved
@vponline
Copy link
Contributor Author

I think that covers everything so please feel free to re-review 😄

packages/airnode-deployer/src/cli/index.ts Outdated Show resolved Hide resolved
packages/airnode-deployer/src/utils/logger.ts Outdated Show resolved Hide resolved
packages/airnode-deployer/src/utils/logger.ts Outdated Show resolved Hide resolved
packages/airnode-deployer/src/utils/logger.ts Outdated Show resolved Hide resolved
packages/airnode-deployer/src/utils/logger.ts Show resolved Hide resolved
packages/airnode-deployer/src/utils/logger.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@amarthadan amarthadan left a comment

Choose a reason for hiding this comment

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

Hmm, a few more issues (sorry 😅)

There's a difference between what's written to the terminal and what's written to the log files. Not every output is using the logger and those are not in the logs (e.g. output of list or info commands). The question is whether we want those outputs there. Can they be considered sensitive information? I don't think so. I think we should include it in the logs.

Also, I would go with the debug mode as you had it before. Even though the debug mode is not enabled, we should write the logs to the file as if it is. That way, if something fails, the user already has the logs with all the info. Some issues may not be easily reproducible so asking them to enable the debug mode might not be a feasible option.

packages/airnode-deployer/src/utils/logger.ts Show resolved Hide resolved
packages/airnode-deployer/src/utils/logger.ts Outdated Show resolved Hide resolved
packages/airnode-deployer/src/utils/logger.ts Show resolved Hide resolved
@vponline vponline merged commit 2a66b8d into master Jan 26, 2023
@vponline vponline deleted the deployer-logs-file branch January 26, 2023 01:56
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.

Improve deployer error messages
2 participants