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

refactor: Use SystemTime for timestamps #5015

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dima74
Copy link
Contributor

@dima74 dima74 commented Aug 27, 2024

Context

Fixes #4729

Solution

Use std::time::SystemTime for time stamps instead of std::time::Duration.

  • Note that SystemTime requires std, so it will not be available inside WASM.
  • Alternatively chrono::DateTime might be used, but it will be more complex (in particular we will have to handle leap seconds), so I am not sure that it is worth it

Migration Guide (optional)

Review notes (optional)

Checklist

  • I've read CONTRIBUTING.md.
  • (optional) I've written unit tests for the code changes.
  • All review comments have been resolved.

@0x009922
Copy link
Contributor

0x009922 commented Sep 2, 2024

From SDK standpoint, it can be useful to clearly see the separation between durations and timestamps in the schema. Currently they are just *_ms: u64 fields.

@dima74
Copy link
Contributor Author

dima74 commented Sep 2, 2024

From SDK standpoint, it can be useful to clearly see the separation between durations and timestamps in the schema. Currently they are just *_ms: u64 fields.

One option I see here is to create DurationMs and TimestampMs newtype wrappers for u64 and use them correspondingly

@dima74 dima74 force-pushed the diralik/replace-duration-with-systemtime branch from 07ee499 to 6b9dbad Compare September 2, 2024 15:11
Comment on lines +257 to +258
#[cfg(feature = "std")]
pub fn creation_time(&self) -> SystemTime {
Copy link
Contributor

Choose a reason for hiding this comment

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

but how am I going to access it now in wasm?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think about adding separate methods specificaly for wasm?

#[cfg(not(feature = "std"))]
pub fn creation_time_ms(&self) -> u64 {

Copy link
Contributor

Choose a reason for hiding this comment

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

As a proposal, we could use jiff::Timestamp/chrono::DateTime as a main type instead of std::time::SystemTime. Both can work in no-std context.

Copy link
Contributor

@s8sato s8sato Sep 10, 2024

Choose a reason for hiding this comment

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

I think core::time::Duration was a painstaking effort to make it compatible with no_std. Is it worth having separate methods to achieve semantic consistency? A time span from the origin can be considered a timestamp

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to decide something. Either introducing other no_std compatible types or staying in core::time::Duration is acceptable to me

@s8sato s8sato self-assigned this Sep 9, 2024
@0x009922
Copy link
Contributor

0x009922 commented Sep 9, 2024

One option I see here is to create DurationMs and TimestampMs newtype wrappers for u64 and use them correspondingly

That would be helpful.

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.

Replace use of Duration for timestamps with an actual timestamp, e.g. chrono::DateTime
4 participants