Skip to content
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

Implement span_begin and span_end syscalls #1816

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sistemd
Copy link

@sistemd sistemd commented Jul 15, 2023

Part of #1566. Implements new syscalls for beginning and ending spans, and exposes them through the SDK. Adds new execution events for spans. Implements a new TraceClock abstraction to obtain nanosecond timestamps from the Machine, which can later be used to add timestamps to all ExecutionEvents for tracing.

Tests the added functionality in an integration test.

What's not included in this PR:

  • Emitting span events whenever a method call happens.
  • Including timestamps on all execution events.

I felt like the PR was already quite large, so I decided to leave these for separate PRs.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

(initial review, more coming)

Comment on lines +47 to +50
/// CID of the currently executing method's code.
pub code: Cid,
/// Number of the currently executing method.
pub method: MethodNum,
Copy link
Member

Choose a reason for hiding this comment

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

  1. For the code CID, I'm adding a special event when we invoke an actor so we shouldn't need it here (PR WIP).
  2. With the method number, we should be able to derive that from the current Call.

/// The ID of the span that is ending.
pub id: SpanId,
/// The timestamp when this event ocurred, in nanoseconds.
pub timestamp: u64,
Copy link
Member

Choose a reason for hiding this comment

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

What if, instead, we recorded a timestamp every time we entered/exited wasm? I.e.:

  • InvokeActor(cid, timestamp)
  • ExitActor(timestamp)
  • BeginSyscall(module, function, timestamp)
  • EndSyscall(timestamp)

This would also help with gas timing.

Copy link
Author

Choose a reason for hiding this comment

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

To make sure I understand correctly: would you like to do this instead of the span functionality, or would you like to do this as well as the span functionality?

I believe InvokeActor and ExitActor are equivalent to what we have today with Call, CallReturn, and CallError.

Just for the record, I think that it would be useful to include the timestamp on every event. I think a change like this deserves a separate issue/PR though.

Copy link
Author

Choose a reason for hiding this comment

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

Also, regarding your previous comment:

For the code CID, I'm adding a special event when we invoke an actor so we shouldn't need it here (PR WIP).

are you already adding InvokeActor?

Copy link
Member

Choose a reason for hiding this comment

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

To make sure I understand correctly: would you like to do this instead of the span functionality, or would you like to do this as well as the span functionality?

In addition to spans. The idea is to split "spans" from "timestamps".

But you're right, doing it in a second PR makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

are you already adding InvokeActor?

@fridrik01 is likely going to do it in a few days as he'll need it for some tracing work he's doing in lotus.

Copy link
Contributor

@fridrik01 fridrik01 left a comment

Choose a reason for hiding this comment

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

Did a quick review of the PR and implementation looks fine to me but lets wait on Steb's further comments

@codecov-commenter
Copy link

Codecov Report

Merging #1816 (5ed1fa8) into master (124f444) will decrease coverage by 20.81%.
The diff coverage is 80.43%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1816       +/-   ##
===========================================
- Coverage   75.32%   54.51%   -20.81%     
===========================================
  Files         149      112       -37     
  Lines       14553     9549     -5004     
===========================================
- Hits        10962     5206     -5756     
- Misses       3591     4343      +752     
Files Changed Coverage Δ
fvm/src/call_manager/mod.rs 81.81% <ø> (ø)
fvm/src/machine/boxed.rs 0.00% <0.00%> (ø)
fvm/src/machine/mod.rs 76.56% <ø> (ø)
fvm/src/trace/mod.rs 8.33% <0.00%> (-91.67%) ⬇️
fvm/src/syscalls/debug.rs 50.00% <96.00%> (+34.84%) ⬆️
fvm/src/call_manager/default.rs 89.14% <100.00%> (+0.57%) ⬆️
fvm/src/kernel/default.rs 54.09% <100.00%> (+0.98%) ⬆️
fvm/src/machine/default.rs 74.28% <100.00%> (+3.95%) ⬆️
fvm/src/syscalls/mod.rs 97.86% <100.00%> (+0.02%) ⬆️

... and 93 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants