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 stat for clock runtime #121

Merged
merged 4 commits into from
Feb 7, 2022
Merged

Add stat for clock runtime #121

merged 4 commits into from
Feb 7, 2022

Conversation

kination
Copy link
Contributor

@kination kination commented Jan 29, 2022

Following #76
(please ignore branch/commit name...)

Copy link
Contributor

@cratelyn cratelyn left a comment

Choose a reason for hiding this comment

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

nice work @djKooks! i have just a small pair of nitpicks about variable names. otherwise, this looks great 🙂

@@ -228,7 +229,7 @@ impl ExecuteCtx {
remote: IpAddr,
) -> Result<(), ExecutionError> {
info!("handling request {} {}", req.method(), req.uri());

let now = Instant::now();
Copy link
Contributor

Choose a reason for hiding this comment

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

how about renaming this variable ever so slightly, to something like...

Suggested change
let now = Instant::now();
let start_timestamp = Instant::now();

@@ -276,11 +277,15 @@ impl ExecuteCtx {
.expect("`memory` is exported")
.size(&store);

let distant = Instant::now().duration_since(now);
Copy link
Contributor

Choose a reason for hiding this comment

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

...and similarly, a slight name tweak

Suggested change
let distant = Instant::now().duration_since(now);
let request_duration = Instant::now().duration_since(start_timestamp);

info!(
"request completed using {} of WebAssembly heap",
bytesize::ByteSize::kib(heap_pages as u64 * 64)
);

info!("request completed in {:?}", distant);
Copy link
Contributor

Choose a reason for hiding this comment

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

same rename as above, just applying it here so you can fix this with the click of a mouse 🖱️

Suggested change
info!("request completed in {:?}", distant);
info!("request completed in {:?}", request_duration);

@kination
Copy link
Contributor Author

kination commented Feb 2, 2022

@cratelyn updated 🙏

@cratelyn
Copy link
Contributor

cratelyn commented Feb 2, 2022

this looks like it's almost ready to go @djKooks! there's currently a hiccup in CI here that was addressed in #123. if you rebase this branch on the current main we should be able to fix that. (note: happy to help walk you through that if you run into any questions!)

@kination
Copy link
Contributor Author

kination commented Feb 3, 2022

@cratelyn got it. Updated~

Copy link
Contributor

@cratelyn cratelyn left a comment

Choose a reason for hiding this comment

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

thank you so much @djKooks! this looks great 🙂

@cratelyn cratelyn merged commit 573ebd3 into fastly:main Feb 7, 2022
@kination kination deleted the update/emit-ram-cpu-stats branch February 8, 2022 00:13
@cratelyn cratelyn mentioned this pull request Feb 8, 2022
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.

2 participants