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

chore(nns): reduce logging in tests #1367

Merged
merged 5 commits into from
Sep 10, 2024
Merged

Conversation

max-dfinity
Copy link
Contributor

@max-dfinity max-dfinity commented Sep 5, 2024

There are many logs that are helpful in mainnet that are creating too much noise in tests, causing in some cases the testing infrastructure to get overwhelmed and hang during cleanup.

Additionally, removing the extra logs reduces the test run time from ~40s to ~5s.

@github-actions github-actions bot added the chore label Sep 5, 2024
@max-dfinity max-dfinity force-pushed the msum/reduce-noisy-logs branch from 826ead0 to a4e3a60 Compare September 5, 2024 21:39
@max-dfinity max-dfinity force-pushed the msum/reduce-noisy-logs branch from a4e3a60 to c9f4a99 Compare September 5, 2024 22:01
@max-dfinity max-dfinity marked this pull request as ready for review September 5, 2024 22:16
@max-dfinity max-dfinity requested a review from a team as a code owner September 5, 2024 22:16
@max-dfinity max-dfinity disabled auto-merge September 5, 2024 22:56
Copy link
Contributor

@daniel-wong-dfinity-org daniel-wong-dfinity-org left a comment

Choose a reason for hiding this comment

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

NCR

In general, I do not like dropping data. Ofc, I also hate log spam. These concerns are somewhat in conflict, but I think we can usually satisfy both. The key is to be selective. Throwing out the bath water good. Throwing out the baby bad.

I wouldn't say all the deleted logging are super high value, but they aren't spam either.

I don't think the title matches. More precisely, some of the deleted logging is NOT only during tests. I like what the title claims more than what the code changes actually do.

Ideally (i.e. maybe not in this PR), we would have a generalized ability to filter out logs (and apply that to tests), rather than deleting logging entirely. At Google, there is this nice flag called --vmodule, which is kind of what I'm thinking of. You could use that to turn on "super debug" level logs by filename. Maybe what we want here is the opposite: be able to turn OFF some logs.

Maybe, we could do that like this:

#[cfg(???)]
use filter_out_some_logs_in_tests::println;

@anchpop
Copy link
Contributor

anchpop commented Sep 9, 2024

NCR

In general, I do not like dropping data. Ofc, I also hate log spam. These concerns are somewhat in conflict, but I think we can usually satisfy both. The key is to be selective. Throwing out the bath water good. Throwing out the baby bad.

I wouldn't say all the deleted logging are super high value, but they aren't spam either.

I don't think the title matches. More precisely, some of the deleted logging is NOT only during tests. I like what the title claims more than what the code changes actually do.

Ideally (i.e. maybe not in this PR), we would have a generalized ability to filter out logs (and apply that to tests), rather than deleting logging entirely. At Google, there is this nice flag called --vmodule, which is kind of what I'm thinking of. You could use that to turn on "super debug" level logs by filename. Maybe what we want here is the opposite: be able to turn OFF some logs.

Maybe, we could do that like this:

#[cfg(???)]
use filter_out_some_logs_in_tests::println;

I agree that some of these are useful and they're not just being disabled in tests. But fast tests > logs IMO. I wonder if it would be possible to buffer them to get the best of both worlds.

@max-dfinity
Copy link
Contributor Author

If it was just a case of fast tests or good production logs, it would be a bit more nuanced.

These logs were not printing in production, and some were only meant for tests in the first place, and they all created so much noise in tests that it broke our local test runners.

We all agree logs are good.

@max-dfinity max-dfinity changed the title chore(nns): reduce logging in tests that is only useful in production chore(nns): reduce logging in tests Sep 9, 2024
@max-dfinity max-dfinity added this pull request to the merge queue Sep 10, 2024
Merged via the queue into master with commit 7b8d005 Sep 10, 2024
24 checks passed
@max-dfinity max-dfinity deleted the msum/reduce-noisy-logs branch September 10, 2024 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants