Use more consistent time representation throughout the engine #8143

pierrechevalier83 · 2019-08-06T16:32:03Z

To represent timestamps and durations, we used to use a mix of various
strategies:

f64 representing a number of seconds
time::Timespec
protobuf::well_known_type::Timestamp

A span of time was sometimes represented as

a start and an end timestamp, or
a start and a duration

This lead to many little helper functions converting from one type of
time representation to another, which caused clutter.

There were valid reasons for alternating between different
representations. For instance, we sometimes wanted a timestamp that
could be serialized and an f64 filled that role.
We sometimes wanted a timestamp that was more strongly typed, and a
time::Timespec filled that role, despite coming from a deprecated
crate.

In general, we should use std::time for time operations, but these
types don't lend themselves to serialization as they explicitely avoid
exposing their internals through serialization or otherwise to avoid
misleading programmers into serializing a timestamp, checking it on a
different machine with a different timezone and ending up with broken
assumptions.

Since we do want to serialize durations (since EPOCH, so a timestamp or
since another duration), create a simple struct called
concrete_time::Duration that can seemlessly be converted from and into
a std::time::Duration and that can also be serialized.

For convenience, also create a TimeSpan that encapsulates the idea of
starting at some point and ending at some other point. Give it
constructors that make sense given the contexts in which we populate
timespans (from protobuf and from measurements).

Also change the ffi boundary to push the float representation of time as
far as possible, to the py_zipkin interface; so the python code can more
conveniently compare timestamps for equality in tests.

Note that brfs was purposefully omitted from the homogeneisation effort
as it only used time::Timespec without any conversions and that is
what the fuse API expects.

hrfuller

lgtm

hrfuller · 2019-08-06T17:34:49Z

src/python/pants/reporting/zipkin_reporter.py

      span.stop()
+
+def from_secs_and_nanos_to_float(secs, nanos):
+  return secs + nanos/NUM_NANOSECS_IN_SEC


feel free to ignore, but some parens around the division would make me feel good.

stuhood · 2019-08-06T18:25:27Z

src/python/pants/engine/native.py

+    c = self._ffi.from_handle(context_handle)
+    return c.to_value(u64)
+
+  @_extern_decl('Handle', ['ExternContext*', 'uint32_t'])


So, because python doesn't really differentiate between these types, there isn't really much advantage to having all of these methods. You could probably drop uint32_t and cast on the rust side to widen things to 64.

That's fair enough. I saw this as a tad more explicit, but it's true that it may be seen as unnecessary. I'll follow your advice.

stuhood · 2019-08-06T18:27:38Z

src/python/pants/reporting/zipkin_reporter.py

@@ -15,6 +15,7 @@

 logger = logging.getLogger(__name__)

+NUM_NANOSECS_IN_SEC = 1000000000.0


The _IN_SEC suffix here looks like we're describing the unit of this variable, which is a bit confusing. Maybe NANOSECONDS_PER_SECOND or something?

It might be cleaner just to define a const NANOSECOND = 10^-9 then you would multiply instead of divide.

I kind of like Stu's suggestion of NANOSECONDS_PER_SECONDS a bit better because the name is explicit. Will go with it.

pierrechevalier83 · 2019-08-07T10:11:54Z

Actioned the code review comment and fixed a python formatting error. (I edited the existing commits out of habit, although with our squash-on-merge strategy, I may as well have created new commits). The diff can be seen in the github UI by checking the "force-pushed" hyperlink.

blorente

LGTM, modulo the comments.

blorente · 2019-08-07T09:56:52Z

src/rust/engine/concrete_time/src/lib.rs

+/// A timespan
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize)]
+pub struct TimeSpan {
+  /// Duration since the UNIX_EPOCH


I found this comment confusing (in part because didn't read the names of the bound variables). I interpreted it as "Duration from UNIX_EPOCH until the end of this timespan".

Suggested change

/// Duration since the UNIX_EPOCH

/// Duration from the UNIX_EPOCH to the start of this TimeSpan.

Uhm. Not sure about this. Comments apply to the line directly below them, so I thing avoiding the repetition is better. If I mention "the start of this TimeSpan" and then rename start or TimeSpan for some reason, I'll have to edit the comment to keep in sync, which the compiler won't help me with.

blorente · 2019-08-07T10:51:59Z

src/rust/engine/concrete_time/src/lib.rs

+      duration: duration.into(),
+    });
+    if time_span.is_none() {
+      warn!(


Should this just crash? I'll look through the usages, but it doesn't look like returning None is what we want to do.

I think I'd prefer just crashing, or returning a Result<Timespan, String> or something like this.

Would also be okay to leave it as a TODO:

Suggested change

warn!(

// TODO Make this return a Result<> or panic instead of just warning.

warn!(

I see your point, and it may be worth doing in a separate PR. The reason I'm only warning here is because this change is a simple refactoring, so I'm not aiming to change the behaviour too significantly. You can find the old behaviour in src/rust/engine/process_execution/src/remote.rs (duplicated a few times).

I could return an Err from here and warn at the call site, though.

Addressed in an extra commit

pierrechevalier83 · 2019-08-07T11:24:00Z

LGTM, modulo the comments.

Thanks for the code review 😄 I addressed the comments

pierrechevalier83 · 2019-08-07T12:29:41Z

One of the CI failures is legit: forgot to run (and update) the tests after my last add-on commit. Fix coming.

illicitonion

Looks great, thanks!

Are we actually using the serde-ness anywhere? If not, may be worth skipping it until we need it (because compile times).

pierrechevalier83 · 2019-08-07T13:06:02Z

Are we actually using the serde-ness anywhere? If not, may be worth skipping it until we need it (because compile times).

Yes, we are. For instance, we serialize metadata to json in fs_util.

stuhood · 2019-08-07T18:33:04Z

The failures in travis look like a corrupted 3rdparty artifact in the cache. I've cleared the caches for this PR and will restart.

To represent timestamps and durations, we used to use a mix of various strategies: * `f64` representing a number of seconds * `time::Timespec` * `protobuf::well_known_type::Timestamp` A span of time was sometimes represented as * a start and an end timestamp, or * a start and a duration This lead to many little helper functions converting from one type of time representation to another, which caused clutter. There were valid reasons for alternating between different representations. For instance, we sometimes wanted a timestamp that could be serialized and an `f64` filled that role. We sometimes wanted a timestamp that was more strongly typed, and a `time::Timespec` filled that role, despite coming from a deprecated crate. In general, we should use `std::time` for time operations, but these types don't lend themselves to serialization as they explicitely avoid exposing their internals through serialization or otherwise to avoid misleading programmers into serializing a timestamp, checking it on a different machine with a different timezone and ending up with broken assumptions. Since we do want to serialize durations (since EPOCH, so a timestamp or since another duration), create a simple struct called `concrete_time::Duration` that can seemlessly be converted from and into a `std::time::Duration` and that can also be serialized. For convenience, also create a `TimeSpan` that encapsulates the idea of starting at some point and ending at some other point. Give it constructors that make sense given the contexts in which we populate timespans (from protobuf and from measurements). Also change the ffi boundary to push the float representation of time as far as possible, to the py_zipkin interface; so the python code can more conveniently compare timestamps for equality in tests. Note that brfs was purposefully omitted from the homogeneisation effort as it only used `time::Timespec` without any conversions and that is what the `fuse` API expects.

Let the caller decide how to deal with the error case (by warning)

stuhood · 2019-08-08T16:21:47Z

Huge number of network flakes. Cleared caches for those and restarted.

pierrechevalier83 · 2019-08-09T09:28:26Z

Huge number of network flakes. Cleared caches for those and restarted.

Thanks for this 👍

illicitonion requested review from illicitonion, hrfuller and blorente August 6, 2019 16:50

hrfuller approved these changes Aug 6, 2019

View reviewed changes

stuhood approved these changes Aug 6, 2019

View reviewed changes

pierrechevalier83 force-pushed the pchevalier/concrete_time branch from c92ee08 to 0403383 Compare August 7, 2019 10:09

blorente approved these changes Aug 7, 2019

View reviewed changes

pierrechevalier83 force-pushed the pchevalier/concrete_time branch from 2bd6a7f to 93ee5ca Compare August 7, 2019 12:32

illicitonion approved these changes Aug 7, 2019

View reviewed changes

stuhood mentioned this pull request Aug 7, 2019

tests/python/pants_test/base:exception_sink_integration is flaky #8127

Closed

pierrechevalier83 and others added 4 commits August 8, 2019 11:32

Changes in zipkin reporter related to concrete time in rust

2fe472f

TimeSpan: use result to express possible failure

7a597da

Let the caller decide how to deal with the error case (by warning)

Bump timeout for exception_sink_integration

a1ed999

pierrechevalier83 force-pushed the pchevalier/concrete_time branch from 93ee5ca to a1ed999 Compare August 8, 2019 10:33

stuhood merged commit 69f07f0 into pantsbuild:master Aug 8, 2019

pierrechevalier83 deleted the pchevalier/concrete_time branch August 9, 2019 09:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use more consistent time representation throughout the engine #8143

Use more consistent time representation throughout the engine #8143

pierrechevalier83 commented Aug 6, 2019

hrfuller left a comment

hrfuller Aug 6, 2019

stuhood Aug 6, 2019

pierrechevalier83 Aug 7, 2019

stuhood Aug 6, 2019

hrfuller Aug 6, 2019 •

edited

Loading

pierrechevalier83 Aug 7, 2019

pierrechevalier83 commented Aug 7, 2019

blorente left a comment

blorente Aug 7, 2019

pierrechevalier83 Aug 7, 2019

blorente Aug 7, 2019

pierrechevalier83 Aug 7, 2019

pierrechevalier83 Aug 7, 2019

pierrechevalier83 Aug 7, 2019

pierrechevalier83 commented Aug 7, 2019

pierrechevalier83 commented Aug 7, 2019

illicitonion left a comment

pierrechevalier83 commented Aug 7, 2019

stuhood commented Aug 7, 2019

stuhood commented Aug 8, 2019

pierrechevalier83 commented Aug 9, 2019

		@@ -15,6 +15,7 @@

		logger = logging.getLogger(__name__)

		NUM_NANOSECS_IN_SEC = 1000000000.0

	/// Duration since the UNIX_EPOCH
	/// Duration from the UNIX_EPOCH to the start of this TimeSpan.

	warn!(
	// TODO Make this return a Result<> or panic instead of just warning.
	warn!(

Use more consistent time representation throughout the engine #8143

Use more consistent time representation throughout the engine #8143

Conversation

pierrechevalier83 commented Aug 6, 2019

hrfuller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrfuller Aug 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierrechevalier83 commented Aug 7, 2019

blorente left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierrechevalier83 commented Aug 7, 2019

pierrechevalier83 commented Aug 7, 2019

illicitonion left a comment

Choose a reason for hiding this comment

pierrechevalier83 commented Aug 7, 2019

stuhood commented Aug 7, 2019

stuhood commented Aug 8, 2019

pierrechevalier83 commented Aug 9, 2019

hrfuller Aug 6, 2019 •

edited

Loading