-
Notifications
You must be signed in to change notification settings - Fork 143
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
feat: log(LogEntry) to handle different log types #2113
Conversation
504363d
to
d2ab544
Compare
d39bf7c
to
114e5ba
Compare
fvm/src/call_manager/default.rs
Outdated
fn log(&mut self, msg: String) { | ||
self.trace(ExecutionEvent::Log(msg)) | ||
fn log(&mut self, entry: LogEntry) { | ||
self.trace(ExecutionEvent::Log(match entry { |
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 was thinking we could add an entirely new event type (not just string logs). New event types aren't breaking (personally, I'd add "put" and "get" events).
The more breaking version would be to change the structure of the event log. Right now the log preserves the tree structure of the calls but not the tree structure of the syscalls, but we could also preserve the structure of the syscalls. But I don't think that's critical right now.
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.
note: I'd also just add these as separate methods? Maybe?
Or maybe we can avoid adding more methods to the kernel by recording put events at the blockstore level? But then we'd need to wrap the blockstore somehow which is... probably even more annoying.
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 was thinking we could add an entirely new event type (not just string logs)
OK, so this would involve changing the serialisation as it travels through FFI and would break existing clients. What's the general rule we've followed here for versioning? If we did that we would have to bump some number, is a semver-minor appropriate for that? What justifies a semver-major bump in here?
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.
Well:
- Adding new event types isn't a breaking change, users will just ignore the ones they don't understand.
- All FFI serialization is internal to the FFI anyways.
- We allow API breaking changes in the FVM in minor releases, patch releases are for releases with no breaking changes. This violates semver, but we use major versions for network upgrades. See FVM versioning and technical debt #1724 and ref-fvm version philosophy #358.
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.
OK, so maybe just an internally tagged enum serialization, ["syscall","some debug logging from sys::debug::log"]
and ["block_write",{"/":"bafy..."}]
for what I want, then we extend the enum as far as we want with additional stuff.
Thoughts on what other log items might be useful in execution traces?
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 wouldn't think hard about the serialization format. Well, that'll be an issue in the Lotus API but the execution trace has a completely different structure there.
I was thinking:
diff --git a/fvm/src/trace/mod.rs b/fvm/src/trace/mod.rs
index 370828b1..32fbd045 100644
--- a/fvm/src/trace/mod.rs
+++ b/fvm/src/trace/mod.rs
@@ -1,3 +1,4 @@
+use cid::Cid;
// Copyright 2021-2023 Protocol Labs
// SPDX-License-Identifier: Apache-2.0, MIT
use fvm_ipld_encoding::ipld_block::IpldBlock;
@@ -39,4 +40,15 @@ pub enum ExecutionEvent {
state: ActorState,
},
Log(String),
+ Ipld {
+ op: IpldOperation,
+ cid: Cid,
+ size: u64,
+ },
+}
+
+#[derive(Clone, Debug)]
+pub enum IpldOperation {
+ Get, // open
+ Put, // link
}
OK, introduced a |
Yeah, rust really expects exactly one owner. I'm not surprised.
We don't even use that right now. We added that for user-deployable actors but it isn't hooked up anywhere. |
In terms of the tests, it's dying with exit code 143 which apparently means "SIGTERM". |
The hidden error only visible in the raw logs is:
Of course, this is also visible to copilot so it was able to figure out the issue immediately.... |
Just toying around here to see if I can come up with an answer to #2101 (comment); @Stebalien I'm not sure what you mean by generalising the execution log but this is my take on it.