Change janus_messages::Duration and Time to be in units of TimePrecision #4219

jcjones · 2025-12-16T15:56:37Z

Resolves #4019

The rest of the owl:

    /\_/\
   ( o o )
   (  V  )
  /--m-m--\

This small change consists of a few commits which refactor janus_messages::Time and ::Duration to be in janus_messages::TimePrecision units. After the change to the janus_messages module, the first series of commits were done with regexes to get everything to compile. Then I progressively fixed tests, until they all passed. Then I've been performing cleanup, resolving issues from self-review and otherwise.

I have filed follow-on issues for things that seemed bigger but not intrinsic to this change:

I did include two other notable fixes/refactors in this PR which arguably could have gone into separate ones, and still could:

Fix Interval::EMPTY round-trip asymmetry, which came up while I was fixing tests but seems like a non-issue in production
Use IntervalExt::merged_with instead of manual interval construction, which is mostly that we had a slick way to make client_timestamp_interval one place using fold and IntervalExt::merged_with which didn't produce zero-width intervals, and then we had the other way that needed explicit add_duration(Duration::from_seconds(1), &time_precision). This seemed worthwhile to take here.

Resolves #4019

…regation_artifacts test to use fold/merged_with

… ref two more issues to follow-ups

…d_with

…k test - Fix the time_bucket_start_opt for add_report, which I had broken. - Then also ensure that we ask for buckets, not timestamps

…TIME_PRECISION) to from_time_precision_units(0)

FromSql: PostgreSQL empty range is decoded as Interval::EMPTY, which is good. However, ToSql: Interval::EMPTY encodes to [1970-01-01 00:00:00, 1970-01-01 00:00:00), which bit me while I was fixing the rest of this. We shouldn't ever hit this! But we might as well fix it while we're here. Also, these timestamps in PostgreSQL are microseconds, not millis.

…hange

jcjones · 2025-12-16T23:06:21Z

aggregator/src/aggregator/aggregation_job_creator.rs

+        let now_time = clock.now().to_time(&time_precision);
+
+        let report_time_2 = now_time.sub_duration(&batch_time_window_size).unwrap();
+        let report_time_1 = report_time_2.sub_duration(&batch_time_window_size).unwrap();
+
+        // Compute the bucketed time bucket starts for querying outstanding batches
+        let batch_window_units = batch_time_window_size.as_time_precision_units();
+        let bucket_time_1 = Time::from_time_precision_units(
+            (report_time_1.as_time_precision_units() / batch_window_units) * batch_window_units,
+        );
+        let bucket_time_2 = Time::from_time_precision_units(
+            (report_time_2.as_time_precision_units() / batch_window_units) * batch_window_units,
+        );


This is just not ergonomic. I did it this way because to_batch_interval_start() is just odd to reason through hanging off of DateTimeExt -- for this purpose anyway. It's also a bit weird to hang off Time because then we need basically two time_precision arguments.

I'm going to open a follow-on to tackle this better. For now, this may be dumb but it works.

tgeoghegan

I still need to look at the stuff in janus_aggregator_core::datastore but I'm going a little cross eyed and it's getting dark outside, so here's a first set of comments.

messages/src/lib.rs

tgeoghegan · 2025-12-16T21:47:01Z

messages/src/taskprov.rs

The other day I observed to Chris Patton that I'm no longer sure exactly which draft of taskprov we implement. Since you've been looking at this closely and are up to speed on related changes to taskprov itself, do you think you could document the implemented taskprov draft version in the "Draft versions and release branches" table in the top-level README.md? I think it's OK if many of the rows of the taskprov draft columns are "?" so long as we start tracking what version we implement in main / release/0.9 onward.

interop_binaries/src/commands/janus_interop_collector.rs

integration_tests/src/janus.rs

core/src/time.rs

aggregator/src/binaries/aggregator.rs

aggregator/src/aggregator/upload_tests.rs

tgeoghegan · 2025-12-17T00:58:40Z

aggregator/src/aggregator/batch_creator.rs

+                        (report.client_timestamp().as_time_precision_units() / batch_window_units)
+                            * batch_window_units,


It really looks like / batch_window_units * batch_window_units should cancel out, and then Time::from_time_precision_units(report.client_timestamp().as_time_precision_units()) is just report.client_timestamp(). But I think what's happening is that report.client_timestamp() is in terms of the task's time precision but batch_time_window_size is in terms of a different time precision used by the batch creator? Would a method on Time akin to Interval::to_time_precision help make it clear that we're doing a conversion here?

Yeah, that's the issue exactly, the possibility for them being a factor of each other. I'm going to rework that in #4225.

Co-authored-by: Tim Geoghegan <timg@divviup.org>

…nterval

tgeoghegan

More comments on the datastore stuff. Please re-request review when you're done addressing things and I will do another pass.

tgeoghegan · 2025-12-17T23:32:40Z

aggregator_core/src/datastore/models.rs

-    pub fn remaining_lease_duration(&self, current_time: &Time, skew_seconds: u64) -> StdDuration {
+    pub fn remaining_lease_duration(
+        &self,
+        current_time: &DateTime<Utc>,


Lease::lease_expiry_time is NaiveDateTime. Is there a reason current_time can't be the same type? Would that simplify the arithmetic here?

tgeoghegan · 2025-12-17T23:42:07Z

aggregator_core/src/datastore/models.rs

    }
+
+    /// Convert an interval from task time precision to SQL time precision.
+    pub fn from_task_interval(


task_interval doesn't sound right. from_dap_time_interval?

tgeoghegan · 2025-12-17T23:52:19Z

aggregator_core/src/datastore/models.rs

+        let start_sql_usec = time_to_sql_timestamp(*self.0.start(), &SQL_UNIT_TIME_PRECISION)
+            .map_err(|_| "microsecond timestamp of Interval start overflowed")?;
+        let end_sql_usec = time_to_sql_timestamp(self.0.end(), &SQL_UNIT_TIME_PRECISION)


Hmm. I dislike that we have start() -> &Time end() -> Time, especially since Time is Copy and it's just a u64, so we shouldn't bother with references anyway. Ah well, that's not a new issue.

… we can.

jcjones · 2025-12-18T03:11:00Z

More comments on the datastore stuff. Please re-request review when you're done addressing things and I will do another pass.

Will do, but at this point, I'm out! I've resolved everything that I've ... resolved with commits. The unresolved comments will be there for me when we return! See you in 2026!

jcjones added 30 commits December 15, 2025 17:01

Change janus_messages::Duration and Time to be in units of TimePrecision

0e18481

Resolves #4019

Add panics

b418130

Task Duration is supposed to be a Duration, whoops

33c3548

test fix

26ee57a

Fix aggregator_job_creator tests

4f4939f

Fix datastore time precision conversions for intervals

fe72947

Fix upload_tests

1d6710a

Fix taskprov tests

0b6ed32

Fix garbage_collector

ed1bdfd

last unit tests

13fe92b

Use Duration::from_time_precision_units(n) where possible

2b5aaeb

Fix end-to-end tests

532b30f

Update janus_cli and sample configs

8c2c399

The validate_collect_identifier test is no longer valid

8d98bb1

Fix remaining Aggregator API unit tests

c50000d

Start refactoring the datastore tests

2c7a49b

first datastore fixes

2877b40

Down to 5 datastore test failures

f2a67c2

Fix three more tests

594f1bd

Fix taskprov

6c29110

Change calculation of client_timestamp_interval in delete_expired_agg…

7670afe

…regation_artifacts test to use fold/merged_with

Resolve all the TKTKs except for key_rotator

0d3b87f

Resolve the key_rotator TKTK by ref to Issue #4216

250c422

Cleanups of spurious comments, fix some more stray TimePrecision(1)s,…

0f435f9

… ref two more issues to follow-ups

Use IntervalExt::merged_with instead of manual interval construction

4070d0a

Use my Duration::ONE constant

de3b6b4

Time::from_time_precision_units is my friend

0053b45

Some more cleanups

1937dba

Change remaining client_timestamp_interval calculations to fold/merge…

ce188f5

…d_with

Fixups in the collection job cases

b91b184

jcjones added 6 commits December 15, 2025 19:38

Fix the create_aggregation_jobs_for_leader_selected_time_bucketed_tas…

2e40adf

…k test - Fix the time_bucket_start_opt for add_report, which I had broken. - Then also ensure that we ask for buckets, not timestamps

Convert all remaining instances of Time::from_seconds_since_epoch(0, …

61a5211

…TIME_PRECISION) to from_time_precision_units(0)

Cleanups for docker tests

87b8c45

validate_collection_identifier needs no task anymore

953703c

clippy

fe8cde5

jcjones requested a review from a team as a code owner December 16, 2025 15:56

jcjones added 2 commits December 16, 2025 12:29

Merge remote-tracking branch 'origin/main' into 4019-time-precision-c…

6338003

…hange

doc update

b49974a

jcjones commented Dec 16, 2025

View reviewed changes

tgeoghegan reviewed Dec 17, 2025

View reviewed changes

jcjones and others added 7 commits December 17, 2025 11:10

Update messages/src/lib.rs

8013e33

Co-authored-by: Tim Geoghegan <timg@divviup.org>

Review updates pt. 1

dd359ca

Review updates pt. 2

79e201b

Rename Interval::single to Interval::minimal

24c0513

Refactor DAP time types into messages/src/time.rs

c8a5b1c

Refactor Interval::to_time_precision into SqlInterval::to/from_task_i…

c182170

…nterval

Refactor out add_sub/time_precision methods

50b25d0

tgeoghegan reviewed Dec 17, 2025

View reviewed changes

Implement DateTimeExt::is_before and use is_before and is_after where…

705d062

… we can.

		(report.client_timestamp().as_time_precision_units() / batch_window_units)
		* batch_window_units,

Change janus_messages::Duration and Time to be in units of TimePrecision #4219

Are you sure you want to change the base?

Change janus_messages::Duration and Time to be in units of TimePrecision #4219

Uh oh!

Conversation

jcjones commented Dec 16, 2025

Uh oh!

jcjones Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

tgeoghegan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tgeoghegan Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tgeoghegan Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcjones Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

tgeoghegan left a comment

Choose a reason for hiding this comment

Uh oh!

tgeoghegan Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

tgeoghegan Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

tgeoghegan Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

jcjones commented Dec 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tgeoghegan Dec 17, 2025 •

edited

Loading