Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Fix logging from inside the WASM runtime #7355

Merged
merged 2 commits into from
Oct 20, 2020
Merged

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Oct 19, 2020

When using RuntimeLogger to log something from the runtime, we didn't
set any logging level. So, we actually did not log anything from the
runtime as logging is disabled by default. This pr fixes that by setting
the logging level to TRACE. It also adds a test to ensure this does
not break again ;)

When using `RuntimeLogger` to log something from the runtime, we didn't
set any logging level. So, we actually did not log anything from the
runtime as logging is disabled by default. This pr fixes that by setting
the logging level to `TRACE`. It also adds a test to ensure this does
not break again ;)
@bkchr bkchr added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Oct 19, 2020
@bkchr bkchr requested a review from tomusdrw October 19, 2020 15:06
@@ -185,6 +193,7 @@ impl log::Log for RuntimeLogger {
}

fn log(&self, record: &log::Record) {
debug(&"Hey2");
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose this is not intended.

Copy link
Member Author

Choose a reason for hiding this comment

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

🤦🤦

//
// If we don't set any level, logging is disabled
// completly.
log::set_max_level(log::LevelFilter::Trace);
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIR it shouldn't matter. We already return true in log::Log::enabled which should be responsible for filtering like that? Are you sure it's actually filtered somewhere internally in log crate?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I checked that. Remove the line and the test fails ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

https://docs.rs/log/0.4.11/log/#implementing-a-logger

They also mention it here that you need to call it.

Copy link
Member Author

Choose a reason for hiding this comment

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

https://docs.rs/log/0.4.11/src/log/macros.rs.html#46

Here they call the max_level function that checks the value that is set with set_max_value

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough!

frame/support/src/debug.rs Outdated Show resolved Hide resolved
//
// If we don't set any level, logging is disabled
// completly.
log::set_max_level(log::LevelFilter::Trace);
Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough!

@tomusdrw tomusdrw added A8-mergeoncegreen and removed A0-please_review Pull request needs code review. labels Oct 20, 2020
@bkchr bkchr merged commit 743981a into master Oct 20, 2020
@bkchr bkchr deleted the bkchr-fix-wasm-runtime-logging branch October 20, 2020 10:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants