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

Use Single Log File in E2E #11810

Merged
merged 5 commits into from
Dec 23, 2022
Merged

Use Single Log File in E2E #11810

merged 5 commits into from
Dec 23, 2022

Conversation

nisdas
Copy link
Member

@nisdas nisdas commented Dec 22, 2022

What type of PR is this?

Cleanup

What does this PR do? Why is it needed?

  • Simplifies our component startup to only write stderr to a file. This prevents us from having stdout logs in our artifacts which do not provide any new information.
  • Remove debug flag on bootnode component to prevent unnecessary noisy logs from being saved to our E2E artifacts.

Which issues(s) does this PR fix?

N.A

Other notes for review

@nisdas nisdas added Ready For Review E2E Tests End-To-End testing labels Dec 22, 2022
@nisdas nisdas requested a review from a team as a code owner December 22, 2022 06:28
@prestonvanloon
Copy link
Member

Can you elaborate a bit more on the purpose of this PR? Were the stdout logs empty or just not useful?

@prestonvanloon
Copy link
Member

What about piping stdout and stderr to the same log file? The validator does have some logging to stdout because it is not using the proper logger when importing keystores (the progress bar goes to stdout)

@kasey
Copy link
Contributor

kasey commented Dec 23, 2022

What about piping stdout and stderr to the same log file? The validator does have some logging to stdout because it is not using the proper logger when importing keystores (the progress bar goes to stdout)

If you do this, depending on the size of log lines and exactly how writes are buffered (line-wise vs flush-when-full), you may wind up getting some interleaved log lines, like... .0001% of the time. Odds of that happening could be orders of magnitude higher in programs that write to stdout a lot. But ours don't, so it would be very rare. We shouldn't ever be writing meaningful information to stdout - stdout is for data to move between programs in pipes, stderr is for human readable things, like logs.

That said, IMO we either want stdout, in which case we should update this PR to only change the behavior of bootnode (which already writes both out&err to the same file), or we don't care about stdout, in which case we should stop writing it to disk. I vote for dropping it, because it adds no value and dropping it makes things simpler - less files, less lines of code, and no one will ever have to look at this code and wonder "are these going to the same file intentionally? which log lines are coming from stdout and which from stderr?".

Here's what we would lose:

$ ls -al | grep stdout
-rw-r--r-- 1 kasey kasey        0 Dec 22 18:01 beacon_node_0_stdout.log
-rw-r--r-- 1 kasey kasey        0 Dec 22 18:01 beacon_node_1_stdout.log
-rw-r--r-- 1 kasey kasey      169 Dec 22 18:01 validator_0_stdout.log
-rw-r--r-- 1 kasey kasey      169 Dec 22 18:01 validator_1_stdout.log

The validator log is... a progress bar:

$ cat validator_1_stdout.log 
Adding optimizations for validator slashing protection 100% [=========]  [0s:0s]

@prestonvanloon
Copy link
Member

prestonvanloon commented Dec 23, 2022 via email

@prylabs-bulldozer prylabs-bulldozer bot merged commit 4aa075a into develop Dec 23, 2022
@delete-merged-branch delete-merged-branch bot deleted the useSingleLogFile branch December 23, 2022 03:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E2E Tests End-To-End testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants