Allow disabling logging on the command line

[athei]

Fixes #8611
Same for Polkadot: paritytech/polkadot#2901
bench-bot: paritytech/bench-bot#14

Currently, there is no way to compile out logging instructions from the runtime without modifying Cargo.toml files. This caused the situation in the linked issue.

In order to prevent such problems in the future we allow disabling logging on the command line by forwarding the feature from the node-cli to the runtime.

This means in order to run benchmarks properly the following command line needs to be supplied:

cargo run --release --features runtime-benchmarks,disable-logging -- benchmark

Please note that we cannot disable logging it automatically when benchmarking because our CI test supplies the runtime-benchmark feature in order to test the benchmarking code while at the same time relying on logging output.

[kianenigma]

if correct, it should be fixed in polkadot runtimes as well.

[athei]

if correct, it should be fixed in polkadot runtimes as well.

Already on it.

[kianenigma]

Then this is making a regression in all weights, right?

[athei]

Then this is making a regression in all weights, right?

I don't think so. It was only for one specific dispatchable in pallet_contracts which had a lot of trace! stuff in a tight loop. But of course there are potentially more regressions which will be fixed by this.

[kianenigma]

Then this is making a regression in all weights, right?

I don't think so. It was only for one specific dispatchable in pallet_contracts which had a lot of trace! stuff in a tight loop. But of course there are potentially more regressions which will be fixed by this.

phew, I thought we had artificially f-ed the weights in runtime v30, first version to have this feature.

[athei]

@bkchr I moved the disable-logging flag to the frame-benchmarking crate (see latest changes in this PR) and it worked for me (the benchmarks do no longer log). Did you do something different here?

Re-requested reviews. This is now a one line PR which should make the polkadot one obsolet.

[bkchr]

Yeah this works, but this also disables the logging for the node without benchmarking

[athei]

Yeah this works, but this also disables the logging for the node without benchmarking

frame-benchmarking is an optional dependency which is only there if the runtime-benchmarking feature is set. So it should only disable logging when benchmarking is requested. Or am I missing something?

[bkchr]

As I said in the polkadot pr, cargo and feature resolution is broken, since always

[athei]

Yeah you are right. Just verified that behaviour. Rolling back the PR. I guess this is the best we can do right now.

[athei]

The failing test parses logging output of the node. Because all tests are run with the runtime-benchmarks feature enabled those fail because of missing output.

So automatically activating this feature on benchmarking does not seem to be a viable path, right?

[shawntabrizi]

Not a fan of the new feature flag.

Should not possible that someone can compile their node for benchmarking and forget to disable logging such that their results are wrong.

Basti says:

I think we can optimize this by introducing a new host function that returns the max log level. Than we could prevent so call into the host for every log line, which is very likely rejected on the host

[athei]

Not sure how to progress without removing those tests then.

[bkchr]

#8655 this should fix it.

[athei]

Superseded by #8655