-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Disable logging in runtime when enabling runtime-benchmarks
#2901
Conversation
@@ -102,6 +102,7 @@ std = [ | |||
"xcm/std", | |||
] | |||
runtime-benchmarks = [ | |||
"sp-api/disable-logging", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need to enable this in other places as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The common runtime is a dependency of all other runtimes. AFAIK this is the only place where we need to at it. It does not seem to be applicable to bridge and xcm runtimes because those don't depend on sp-api
. Maybe someone else can step in with more knowledge about those runtimes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like we could have some kind of test to make sure we don't make this mistake again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean having logging enabled during benchmarking by accident?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but it is probably a PITA
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea on how to construct such a test other than some benchmark which looks for regressions. And something like that would be really nice to have anyways. After each benchmarks of the contracts pallet I need to manually look through a ton of benchmarks and scan for regressions. Would be nice to have this automated.
Do we need to re-run any benchmarks? |
Every benchmark which was ran since paritytech/substrate#8128 was merged is potentially flawed when it emits a substantial amount of log messages. That said, for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if we could try to enable the disable-logging
feature for sp-api
in the benchmarking crate, to not having the requirement to add this everywhere
@bkchr can we do this by simply adding |
Yeah, this is what I wanted to express with my message xD |
Ok I got it. Would be more appropriate to discuss this in the substrate version of this PR, though. I will update that one with your suggestion. |
I tested it and it doesn't work... Cargo feature resolution is just the best piece of software I know... :( |
I can confirm that. So we just keep this PR the way it is. |
Superseded by paritytech/substrate#8655 |
Same PR as paritytech/substrate#8638
This is different from the substrate PR in that here the logs are automatically disabled whenever the
runtime-benchmarks
feature is supplied. On substrate this is not possible because of tests which parse log output.