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

feat(unstable): Add "Deno.osUptime()" API #17179

Merged
merged 4 commits into from Dec 26, 2022

Conversation

kamilogorek
Copy link
Contributor

@kamilogorek kamilogorek commented Dec 24, 2022

This PR adds support for Deno.uptime which reports number of seconds since os was booted.
It will allow us to be compatible with Node's os.uptime - https://nodejs.org/api/os.html#osuptime

There's also process.uptime, but I believe Deno.uptime should report stats for the OS itself, not the process, and we can always add Deno.processUptime or similar method when it'll become necessary.

Partially based on https://docs.rs/uptime_lib/latest/src/uptime_lib/lib.rs.html

@bartlomieju
Copy link
Member

bartlomieju commented Dec 24, 2022

Implementation wise - looks good. However when adding new APIs to Deno namespace, we also start with making them unstable (requiring --unstable flag to be present) and only gathering some feedback we stabilize the API. Could you change the PR in such a way to this API requires the unstable check and the types are only available in lib.deno_unstable.d.ts? EDIT: That way we can land and release this unstable API in the next patch release, without having to wait for next minor release.

There's also process.uptime, but I believe Deno.uptime should report stats for the OS itself, not the process, and we can always add Deno.processUptime or similar method when it'll become necessary.

Seems it's not needed - performance.now() returns a number of ms from the start of the process, so it could be used for that purpose.

* @tags allow-sys
* @category Runtime Environment
*/
export function uptime(): number;
Copy link
Member

Choose a reason for hiding this comment

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

Little bike-shedding maybe it should be Deno.osUptime()?

Copy link
Contributor

Choose a reason for hiding this comment

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

Deno.osUptime() is more clear and falls in line with the naming convention of Deno.osRelease(). I definitely prefer it over Deno.uptime().

I would also like to bikeshed another alternative being Deno.systemUptime() (which is in line with Deno.systemMemoryInfo(). I don't have strong feelings either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After checking few other places, I decided to go with osUptime, as:

the uptime is a kernel internal value, which ticks up every cycle, it starts counting when the kernel has initialized. That is, when the first cycle has ended. Even before anything is mounted, directly after the bootloader gives control to the kernel image.

which means that IMO it should be treated as Operating System info, where System info should be more "hardware-like" related, as in mem/cpu/peripherals etc.

@lino-levan
Copy link
Contributor

LGTM, nice work!

@bartlomieju bartlomieju changed the title feat(runtime): Add support for uptime operation feat(unstable): Add "Deno.osUptime()" API Dec 26, 2022
@bartlomieju bartlomieju self-requested a review December 26, 2022 01:20
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, nice work @kamilogorek, cleanly done!

@bartlomieju bartlomieju merged commit 7ce2b58 into denoland:main Dec 26, 2022
@kamilogorek kamilogorek deleted the deno-uptime branch December 27, 2022 09:16
bartlomieju pushed a commit that referenced this pull request Jan 5, 2023
This PR adds support for `Deno.osUptime` which reports number of seconds
since os was booted. It will allow us to be compatible with Node's `os.uptime` -
https://nodejs.org/api/os.html#osuptime

Partially based on
https://docs.rs/uptime_lib/latest/src/uptime_lib/lib.rs.html
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

3 participants