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

Add CPU time to output for requests #175

Closed
wants to merge 1 commit into from
Closed

Conversation

mgattozzi
Copy link
Contributor

Fastly uses CPU time to denote how much time is spent actually running the WASM code given to us. Given how asynchronous code works, it means we only run the code when it's being polled. This means we can't just slap Instant::now() in front of the call to the main function of the WASM in Viceroy. We instead construct a future that will check how long each call to poll was and add it to a Duration. When we are done polling we will have the result of the request made to Viceroy and how long that took! We then report it with the other metrics for customers to look at so that they can approximate how much time they spend in their module. While this is by no means a 1 to 1 in terms of how much CPU time is actually used as a customer's local environment is not the same as our POPs it should help guide them in terms of optimizing their code so that requests are fast.

Sample output when running now for stats:

Sep 13 13:22:40.291  INFO request{id=14}: request completed using 1.1 MB of WebAssembly heap
Sep 13 13:22:40.291  INFO request{id=14}: request completed in 5ms
Sep 13 13:22:40.291  INFO request{id=14}: CPU Time 3ms

You can see that it takes 5ms for the whole request to complete, but only 3ms of which is spent working on the actual request using the customer's code.

Fastly uses CPU time to denote how much time is spent *actually running*
the WASM code given to us. Given how asynchronous code works, it means
we only run the code when it's being polled. This means we can't just
slap `Instant::now()` in front of the call to the main function of the
WASM in Viceroy. We instead construct a future that will check how long
each call to `poll` was and add it to a `Duration`. When we are done
polling we will have the result of the request made to Viceroy and how
long that took! We then report it with the other metrics for customers
to look at so that they can approximate how much time they spend in
their module. While this is by no means a 1 to 1 in terms of how much
CPU time is actually used as a customer's local environment is not the
same as our POPs it should help guide them in terms of optimizing their
code so that requests are fast.

Sample output when running now for stats:

```
Sep 13 13:22:40.291  INFO request{id=14}: request completed using 1.1 MB of WebAssembly heap
Sep 13 13:22:40.291  INFO request{id=14}: request completed in 5ms
Sep 13 13:22:40.291  INFO request{id=14}: CPU Time 3ms
```

You can see that it takes 5ms for the whole request to complete, but only
3ms of which is spent working on the actual request using the customer's
code.
@mgattozzi mgattozzi requested review from a team and awortman-fastly and removed request for a team September 13, 2022 17:27
Copy link

@iximeow iximeow left a comment

Choose a reason for hiding this comment

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

dropped a couple thoughts in the review, the most important one i think is using consistent phrasing for the resource usage lines ("WebAssembly heap" and "CPU"). the rest is more just my 2c here

Comment on lines +376 to +377
let poll_time = Instant::now().duration_since(now);
self.duration += poll_time;
Copy link

Choose a reason for hiding this comment

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

Suggested change
let poll_time = Instant::now().duration_since(now);
self.duration += poll_time;
self.duration += now.elapsed();

take it or leave it, but i'm like 95% sure this is a correct suggestion...

@@ -302,6 +303,7 @@ impl ExecuteCtx {
);

info!("request completed in {:.0?}", request_duration);
info!("CPU Time {:.0?}", cpu_time);
Copy link

Choose a reason for hiding this comment

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

probably good to have a message more consistent with the other info text, something like...

Suggested change
info!("CPU Time {:.0?}", cpu_time);
info!("request completed using {:.0?} of CPU time", cpu_time);

additionally moving this up a line or two would give output like:

request completed using ... of WebAssembly heap
request completed using ... of CPU time
request completed in ...

which leaves the first two grouped together as they're both talking about resource use for the request in question. that seems preferable to the WebAssembly heap/request completed/CPU used sandwich we'd get here?

@mgattozzi
Copy link
Contributor Author

This PR is on hold until we get some confirmation from product regarding this.

The issue is that Viceroy is more of a correctness implementation. You run it in CI and then can check things like did my request go to the origin as expected etc. It's not an exact copy of a production environment and while we can show some timings like request total time it's just not going to run at the same speeds or interact with things the same as on our POPs. Viceroy only mimics a part of how Fastly works.

What we don't want is to guarantee that Viceroy is the same as a prod environment, especially for guiding optimizations. It's a good approximation, but it's just not the same as running it in production and it would be better for a customer to run things in a staging environment for tuning perf while using Viceroy to make sure their code runs as expected.

Until we talk to product about how to handle this bit of trickiness or what we want to guarantee I'm going to close this PR for now and reopen it if we wish to proceed with this.

@mgattozzi mgattozzi closed this Sep 16, 2022
@pchickey pchickey deleted the mgattozzi/cpu-time branch July 14, 2023 17:02
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.

None yet

2 participants