Implement log initialisation inline in FVM runtime #1019
Conversation
| @@ -259,8 +259,6 @@ where | |||
| let state_cid = fvm::sself::root() | |||
| .map_err(|_| actor_error!(illegal_argument; "failed to get actor root state CID"))?; | |||
|
|
|||
| log::debug!("getting cid: {}", state_cid); | |||
There was a problem hiding this comment.
We can bring this kind of thing back as trace, if needed. But it looks like just noisy remnants of early development.
This makes no sense. The SDK already has a shared implementation that can be used by non builtin actors, why are we trying to duplicate this here? |
| @@ -552,14 +550,13 @@ where | |||
| /// 5a. In case of error, aborts the execution with the emitted exit code, or | |||
| /// 5b. In case of success, stores the return data as a block and returns the latter. | |||
| pub fn trampoline<C: ActorCode>(params: u32) -> u32 { | |||
| fvm::debug::init_logging(); | |||
There was a problem hiding this comment.
If you want to set the max log level to trace, you can do that here. init_logging is designed to be unopinionated and just initialize the logging framework.
There was a problem hiding this comment.
The v2 SDK initialises it to discard messages! As someone not deeply familiar with Rust logging configuration, this was extremely frustrating to discover. Also it still doesn't work, for reasons I don't understand yet.
The v3 SDK does initialise it to tracing. Maybe it'll work, and maybe we'll want to switch to it when we upgrade.
init_logging is opinionated about the format of the string that is logged, at least. We may well want to change it.
| @@ -552,14 +550,13 @@ where | |||
| /// 5a. In case of error, aborts the execution with the emitted exit code, or | |||
| /// 5b. In case of success, stores the return data as a block and returns the latter. | |||
| pub fn trampoline<C: ActorCode>(params: u32) -> u32 { | |||
| fvm::debug::init_logging(); | |||
There was a problem hiding this comment.
The v2 SDK initialises it to discard messages! As someone not deeply familiar with Rust logging configuration, this was extremely frustrating to discover. Also it still doesn't work, for reasons I don't understand yet.
The v3 SDK does initialise it to tracing. Maybe it'll work, and maybe we'll want to switch to it when we upgrade.
init_logging is opinionated about the format of the string that is logged, at least. We may well want to change it.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1019 +/- ##
==========================================
- Coverage 89.20% 89.12% -0.08%
==========================================
Files 92 92
Lines 19580 19327 -253
==========================================
- Hits 17466 17226 -240
+ Misses 2114 2101 -13
|
anorth
force-pushed
the
anorth/workbench
branch
3 times, most recently
from
77f5755
to
64f9f6d
Compare
anorth
force-pushed
the
anorth/workbench
branch
from
64f9f6d
to
908b3a0
Compare
I couldn't get logging to result in a syscall with the SDK (even with local patch to include filecoin-project/ref-fvm#1431)
This brings log initialisation for the builtin actors into their repo, which I think is entirely appropriate anyway. And I can verify that logging produces a syscall.