[local_aggregation] Cache results of civil time conversions
The ObservationGenerator needs to convert time points
to civil times with respect to specific time zones,
with a lot of repeated inputs within a given run of
of GenerateObservationsOnce(). This adds caching of
civil time conversion across all metrics.
Bug: 92131
Change-Id: I0496538ddc08a5331696c9a8c052b978f48b951e
Reviewed-on: https://fuchsia-review.googlesource.com/c/cobalt/+/673673
Reviewed-by: Cameron Dale <camrdale@google.com>
Commit-Queue: Laura Peskin <pesk@google.com>
diff --git a/src/lib/util/datetime_util.cc b/src/lib/util/datetime_util.cc
index 1a9360b..a4b080d 100644
--- a/src/lib/util/datetime_util.cc
+++ b/src/lib/util/datetime_util.cc
@@ -220,7 +220,7 @@
return (day_index + 4) / kDaysPerWeek;
}
-uint32_t DayIndexToHourId(uint32_t day_index) { return day_index * kHourIdsPerDay; }
+uint32_t DayIndexToHourIdNoDst(uint32_t day_index) { return day_index * util::kHourIdsPerDay + 1; }
uint32_t HourIdToDayIndex(uint32_t hour_id) { return (hour_id / kHourIdsPerDay); }
uint32_t CalendarDateToWeekIndex(const CalendarDate& calendar_date) {
@@ -278,14 +278,14 @@
TimeInfo TimeInfo::FromHourId(uint32_t hour_id) {
TimeInfo data;
data.hour_id = hour_id;
- data.day_index = util::HourIdToDayIndex(data.hour_id);
+ data.day_index = HourIdToDayIndex(data.hour_id);
return data;
}
TimeInfo TimeInfo::FromDayIndex(uint32_t day_index) {
TimeInfo data;
data.day_index = day_index;
- data.hour_id = util::DayIndexToHourId(data.day_index);
+ data.hour_id = DayIndexToHourIdNoDst(day_index);
return data;
}
diff --git a/src/lib/util/datetime_util.h b/src/lib/util/datetime_util.h
index 1da8504..bfb5966 100644
--- a/src/lib/util/datetime_util.h
+++ b/src/lib/util/datetime_util.h
@@ -149,8 +149,9 @@
// containing that date.
uint32_t DayIndexToWeekIndex(uint32_t day_index);
-// Given a day index, returns the hour identifier for the start of that day.
-uint32_t DayIndexToHourId(uint32_t day_index);
+// Given a day index, returns the hour identifier for the start of that day, assuming that DST is
+// not in force.
+uint32_t DayIndexToHourIdNoDst(uint32_t day_index);
// Given an hour identifier, returns the index of the Cobalt day containing that hour.
uint32_t HourIdToDayIndex(uint32_t hour_id);
diff --git a/src/lib/util/datetime_util_test.cc b/src/lib/util/datetime_util_test.cc
index a6eecbd..47bda4c 100644
--- a/src/lib/util/datetime_util_test.cc
+++ b/src/lib/util/datetime_util_test.cc
@@ -738,6 +738,26 @@
ASSERT_FALSE(TimeInfo::FromTimePoint(time, converter, metric).ok());
}
+TEST(DatetimeUtilTest, TimeInfoFromDayIndex) {
+ const uint32_t day_index = 100;
+ TimeInfo info = TimeInfo::FromDayIndex(day_index);
+ EXPECT_EQ(info.day_index, day_index);
+ // The hour ID should be day_index * util::kHourIdsPerDay + 1, since this function assumes that
+ // DST is not in force.
+ EXPECT_EQ(info.hour_id, 4801u);
+}
+
+TEST(DatetimeUtilTest, TimeInfoFromHourId) {
+ const uint32_t hour_id_dst = 4800;
+ const uint32_t hour_id_nodst = 4801;
+ TimeInfo info_dst = TimeInfo::FromHourId(hour_id_dst);
+ TimeInfo info_nodst = TimeInfo::FromHourId(hour_id_nodst);
+ EXPECT_EQ(info_dst.day_index, 100u);
+ EXPECT_EQ(info_dst.hour_id, hour_id_dst);
+ EXPECT_EQ(info_nodst.day_index, 100u);
+ EXPECT_EQ(info_nodst.hour_id, hour_id_nodst);
+}
+
TEST(DatetimeUtilTest, TimeInfoFromTimePointDeprecated) {
const uint32_t day_index = 100;
auto time = FromUnixSeconds(static_cast<int64_t>(day_index) * util::kNumUnixSecondsPerDay);
diff --git a/src/local_aggregation_1_1/BUILD.gn b/src/local_aggregation_1_1/BUILD.gn
index 964911b..e2730de 100644
--- a/src/local_aggregation_1_1/BUILD.gn
+++ b/src/local_aggregation_1_1/BUILD.gn
@@ -29,6 +29,7 @@
testonly = true
deps = [
":backfill_manager",
+ ":civil_time_manager",
":local_aggregation",
":observation_generator",
":proto",
@@ -43,6 +44,7 @@
"$cobalt_root/src/logger:internal_metrics",
"$cobalt_root/src/logger:logger_test_utils",
"$cobalt_root/src/logger:project_context_factory",
+ "$cobalt_root/src/public/lib:clock_interfaces",
"$cobalt_root/src/public/lib:registry_identifiers",
"$cobalt_root/src/public/lib/statusor",
"$cobalt_root/src/registry/testing",
@@ -59,6 +61,7 @@
]
sources = [
"backfill_manager_test.cc",
+ "civil_time_manager_test.cc",
"local_aggregation_test.cc",
"observation_generator_test.cc",
]
@@ -94,6 +97,7 @@
"backfill_manager.h",
]
deps = [
+ ":civil_time_manager",
"$cobalt_root/src/lib/util:datetime_util",
"aggregation_procedures",
]
@@ -103,6 +107,23 @@
]
}
+source_set("civil_time_manager") {
+ visibility = []
+ visibility = [
+ ":backfill_manager",
+ ":observation_generator",
+ ":tests",
+ ]
+
+ sources = [ "civil_time_manager.h" ]
+ deps = [ "$cobalt_root/src/lib/util:datetime_util" ]
+ public_deps = [
+ "$cobalt_root/src/public/lib:clock_interfaces",
+ "$cobalt_root/src/public/lib:registry_identifiers",
+ "$cobalt_root/src/public/lib/statusor",
+ ]
+}
+
source_set("observation_generator") {
sources = [
"observation_generator.cc",
@@ -110,6 +131,7 @@
]
deps = [
":backfill_manager",
+ ":civil_time_manager",
"$cobalt_root/src/lib/util:clock",
"$cobalt_root/src/lib/util:datetime_util",
"$cobalt_root/src/logger:project_context_factory",
diff --git a/src/local_aggregation_1_1/backfill_manager.cc b/src/local_aggregation_1_1/backfill_manager.cc
index 042661a..b6744b5 100644
--- a/src/local_aggregation_1_1/backfill_manager.cc
+++ b/src/local_aggregation_1_1/backfill_manager.cc
@@ -22,17 +22,15 @@
: backstop_days_(total_backfill_window) {}
lib::statusor::StatusOr<std::vector<TimeInfo>> BackfillManager::CalculateBackfill(
- TimeInfo last_time_info, time_point end_time,
- const util::CivilTimeConverterInterface& civil_time_converter, const MetricDefinition& metric,
+ TimeInfo last_time_info, CivilTimeManager* civil_time_mgr, const MetricDefinition& metric,
bool is_daily, bool is_expedited) const {
// Calculate the end day index and hour ID with respect to the time zone specified in `metric`.
- CB_ASSIGN_OR_RETURN(TimeInfo end_info,
- TimeInfo::FromTimePoint(end_time, civil_time_converter, metric));
+ CB_ASSIGN_OR_RETURN(TimeInfo end_info, civil_time_mgr->GetTimeInfo(0, metric));
// Calculate the backstop day index with respect to the current UTC time. It is not useful to send
// observations for earlier day indices, since the shuffler's lookback period is calculated with
// respect to UTC. Observations with smaller day indices will be dropped.
- uint32_t backstop_day_index = util::TimePointToDayIndexUtc(end_time) - backstop_days_;
+ uint32_t backstop_day_index = civil_time_mgr->GetInitialUtc().day_index - backstop_days_;
std::vector<TimeInfo> backfill;
// If calculating backfill for a daily report, select every day index since the last one (or the
@@ -52,15 +50,13 @@
backfill.push_back(end_info);
}
} else {
- time_point time = end_time - util::kOneHour;
- while (time >= time_point::min()) {
- CB_ASSIGN_OR_RETURN(TimeInfo time_info,
- TimeInfo::FromTimePoint(time, civil_time_converter, metric));
- if (time_info.hour_id <= last_time_info.hour_id || time_info.day_index < backstop_day_index) {
- break;
- }
+ int past_hours = 1;
+ CB_ASSIGN_OR_RETURN(TimeInfo time_info, civil_time_mgr->GetTimeInfo(past_hours, metric));
+ while (time_info.hour_id > last_time_info.hour_id &&
+ time_info.day_index >= backstop_day_index) {
backfill.push_back(time_info);
- time -= util::kOneHour;
+ past_hours++;
+ CB_ASSIGN_OR_RETURN(time_info, civil_time_mgr->GetTimeInfo(past_hours, metric));
}
std::reverse(backfill.begin(), backfill.end());
}
diff --git a/src/local_aggregation_1_1/backfill_manager.h b/src/local_aggregation_1_1/backfill_manager.h
index 293496c..d8ab383 100644
--- a/src/local_aggregation_1_1/backfill_manager.h
+++ b/src/local_aggregation_1_1/backfill_manager.h
@@ -6,6 +6,7 @@
#define COBALT_SRC_LOCAL_AGGREGATION_1_1_BACKFILL_MANAGER_H_
#include "src/local_aggregation_1_1/aggregation_procedures/aggregation_procedure.h"
+#include "src/local_aggregation_1_1/civil_time_manager.h"
#include "src/local_aggregation_1_1/local_aggregation.pb.h"
#include "src/public/lib/clock_interfaces.h"
#include "src/public/lib/statusor/statusor.h"
@@ -27,15 +28,14 @@
// observations have not yet been generated.
//
// |last_time_info| The last `TimeInfo` for which an observation has been generated.
- // |end_time| The time at which observation generation was most recently requested.
- // |civil_time_converter| Converts time points into civil times for a given time zone.
+ // |civil_time_mgr| Converts the end time of the observation generation interval, as well
+ // as earlier hourly time points, into civil times for a given time zone.
// |metric| The metric definition associated with the observations.
// |is_daily| If true, we should generate daily TimeInfos, otherwise generate hourly.
// |is_expedited| If true, generate a TimeInfo for the day of |end_time|.
[[nodiscard]] lib::statusor::StatusOr<std::vector<util::TimeInfo>> CalculateBackfill(
- util::TimeInfo last_time_info, std::chrono::system_clock::time_point end_time,
- const util::CivilTimeConverterInterface& civil_time_converter, const MetricDefinition& metric,
- bool is_daily, bool is_expedited) const;
+ util::TimeInfo last_time_info, CivilTimeManager* civil_time_mgr,
+ const MetricDefinition& metric, bool is_daily, bool is_expedited) const;
private:
// The number of days in the past for which we should generate observations.
diff --git a/src/local_aggregation_1_1/backfill_manager_test.cc b/src/local_aggregation_1_1/backfill_manager_test.cc
index 15f3b74..3bae89b 100644
--- a/src/local_aggregation_1_1/backfill_manager_test.cc
+++ b/src/local_aggregation_1_1/backfill_manager_test.cc
@@ -12,6 +12,7 @@
#include "src/lib/util/clock.h"
#include "src/lib/util/datetime_util.h"
#include "src/local_aggregation_1_1/aggregation_procedures/aggregation_procedure.h"
+#include "src/local_aggregation_1_1/civil_time_manager.h"
#include "src/public/lib/statusor/status_macros.h"
namespace cobalt::local_aggregation {
@@ -50,14 +51,16 @@
util::kHourIdsPerHour);
std::chrono::system_clock::time_point end_time =
start_time + std::chrono::hours(kNumHoursToGenerate + 1);
+ CivilTimeManager civil_time_mgr(end_time, converter);
CB_ASSERT_OK_AND_ASSIGN(TimeInfo last_time_info,
TimeInfo::FromTimePoint(start_time, converter, metric));
ASSERT_EQ(last_time_info.hour_id, 10001);
CB_ASSERT_OK_AND_ASSIGN(std::vector<TimeInfo> backfill,
- manager.CalculateBackfill(last_time_info, end_time, converter, metric,
+ manager.CalculateBackfill(last_time_info, &civil_time_mgr, metric,
/*is_daily=*/false, /*is_expedited=*/false));
+
EXPECT_EQ(ToHourIds(backfill), std::vector<uint32_t>({10003, 10005, 10007, 10009, 10011}));
}
@@ -71,9 +74,10 @@
TimeInfo start = TimeInfo::FromHourId(0);
std::chrono::system_clock::time_point end_time(
std::chrono::hours(896489 / util::kHourIdsPerHour + 1));
+ CivilTimeManager civil_time_mgr(end_time, converter);
CB_ASSERT_OK_AND_ASSIGN(std::vector<TimeInfo> backfill,
- manager.CalculateBackfill(start, end_time, converter,
+ manager.CalculateBackfill(start, &civil_time_mgr,
MetricWithTimeZonePolicy(MetricDefinition::UTC),
/*is_daily=*/false, /*is_expedited=*/false));
@@ -91,10 +95,11 @@
// Set the end time to the first hour of the next day.
std::chrono::system_clock::time_point end_time(
std::chrono::hours(util::kNumHoursPerDay * (kStartDayIndex + 2)));
+ CivilTimeManager civil_time_mgr(end_time, converter);
CB_ASSERT_OK_AND_ASSIGN(
std::vector<TimeInfo> backfill,
- manager.CalculateBackfill(TimeInfo::FromHourId(kStartHourId), end_time, converter, metric,
+ manager.CalculateBackfill(TimeInfo::FromHourId(kStartHourId), &civil_time_mgr, metric,
/*is_daily=*/false, /*is_expedited=*/false));
std::vector<uint32_t> hour_sequence = ToHourIds(backfill);
@@ -111,10 +116,11 @@
// Set the end time to 3 hours before the end of the same day.
std::chrono::system_clock::time_point end_time(
std::chrono::hours(util::kNumHoursPerDay * (kStartDayIndex + 1) - 3));
+ CivilTimeManager civil_time_mgr(end_time, converter);
CB_ASSERT_OK_AND_ASSIGN(
std::vector<TimeInfo> backfill,
- manager.CalculateBackfill(TimeInfo::FromHourId(kStartHourId), end_time, converter,
+ manager.CalculateBackfill(TimeInfo::FromHourId(kStartHourId), &civil_time_mgr,
MetricWithTimeZonePolicy(MetricDefinition::UTC), /*is_daily=*/false,
/*is_expedited=*/false));
@@ -131,12 +137,14 @@
const uint32_t kNumDaysToGenerate = 5;
std::chrono::system_clock::time_point end_time(
std::chrono::hours(util::kNumHoursPerDay * (kStartDayIndex + kNumDaysToGenerate + 1)));
+ CivilTimeManager civil_time_mgr(end_time, converter);
CB_ASSERT_OK_AND_ASSIGN(
std::vector<TimeInfo> backfill,
- manager.CalculateBackfill(TimeInfo::FromDayIndex(kStartDayIndex), end_time, converter,
+ manager.CalculateBackfill(TimeInfo::FromDayIndex(kStartDayIndex), &civil_time_mgr,
MetricWithTimeZonePolicy(MetricDefinition::UTC), /*is_daily=*/true,
/*is_expedited=*/false));
+
EXPECT_EQ(ToDayIndices(backfill), std::vector<uint32_t>({10002, 10003, 10004, 10005, 10006}));
}
@@ -148,11 +156,13 @@
const uint32_t kNumPastDays = 2;
std::chrono::system_clock::time_point end_time(
std::chrono::hours(util::kNumHoursPerDay * (kStartDayIndex + kNumPastDays + 1)));
+ CivilTimeManager civil_time_mgr(end_time, converter);
CB_ASSERT_OK_AND_ASSIGN(
std::vector<TimeInfo> backfill,
- manager.CalculateBackfill(TimeInfo::FromDayIndex(kStartDayIndex), end_time, converter,
- MetricWithTimeZonePolicy(MetricDefinition::UTC), /*is_daily=*/true,
+ manager.CalculateBackfill(TimeInfo::FromDayIndex(kStartDayIndex), &civil_time_mgr,
+ MetricWithTimeZonePolicy(MetricDefinition::UTC),
+ /*is_daily=*/true,
/*is_expedited=*/true));
// Expect day indices for the 2 past days in addition to the day index of `end_time`.
@@ -168,10 +178,11 @@
const uint32_t kLastDayIndex = 120;
std::chrono::system_clock::time_point end_time(
std::chrono::hours(util::kNumHoursPerDay * (kLastDayIndex + 1)));
+ CivilTimeManager civil_time_mgr(end_time, converter);
CB_ASSERT_OK_AND_ASSIGN(
std::vector<TimeInfo> backfill,
- manager.CalculateBackfill(TimeInfo::FromDayIndex(kStartDayIndex), end_time, converter,
+ manager.CalculateBackfill(TimeInfo::FromDayIndex(kStartDayIndex), &civil_time_mgr,
MetricWithTimeZonePolicy(MetricDefinition::UTC), /*is_daily=*/true,
/*is_expedited=*/false));
@@ -193,14 +204,16 @@
start_time + std::chrono::hours(kNumHoursToGenerate + 1);
util::FakeCivilTimeConverter converter(/*start_utc_offset=*/0, /*start_isdst=*/false);
+ CivilTimeManager civil_time_mgr(end_time, converter);
CB_ASSERT_OK_AND_ASSIGN(TimeInfo start_time_info,
TimeInfo::FromTimePoint(start_time, converter, metric));
ASSERT_EQ(start_time_info.hour_id, 10001);
CB_ASSERT_OK_AND_ASSIGN(std::vector<TimeInfo> backfill,
- manager.CalculateBackfill(start_time_info, end_time, converter, metric,
+ manager.CalculateBackfill(start_time_info, &civil_time_mgr, metric,
/*is_daily=*/false, /*is_expedited=*/false));
+
EXPECT_EQ(ToHourIds(backfill), std::vector<uint32_t>({10003, 10005, 10007, 10009, 10011}));
}
@@ -216,14 +229,16 @@
start_time + std::chrono::hours(kNumHoursToGenerate + 1);
util::FakeCivilTimeConverter converter(/*start_utc_offset=*/8, /*start_isdst=*/false);
+ CivilTimeManager civil_time_mgr(end_time, converter);
CB_ASSERT_OK_AND_ASSIGN(TimeInfo start_time_info,
TimeInfo::FromTimePoint(start_time, converter, metric));
ASSERT_EQ(start_time_info.hour_id, 10017);
CB_ASSERT_OK_AND_ASSIGN(std::vector<TimeInfo> backfill,
- manager.CalculateBackfill(start_time_info, end_time, converter, metric,
+ manager.CalculateBackfill(start_time_info, &civil_time_mgr, metric,
/*is_daily=*/false, /*is_expedited=*/false));
+
EXPECT_EQ(ToHourIds(backfill), std::vector<uint32_t>({10019, 10021, 10023, 10025, 10027}));
}
@@ -243,8 +258,9 @@
ASSERT_EQ(util::TimePointToDayIndexUtc(end_time), 101);
util::FakeCivilTimeConverter converter(/*start_utc_offset=*/-23, /*start_isdst=*/false);
+ CivilTimeManager civil_time_mgr(end_time, converter);
- // All times between `start_time` and `end_time`-3h have day index 99 with respect to `metric`'s
+ // All times between `start_time` and `end_time`-3h have day index 99 with respect to `metric`'s
// time zone. Only the hours `end_time`-2h and `end_time`-1h (hour IDs 4801 and 4803) fall within
// the backfill period.
CB_ASSERT_OK_AND_ASSIGN(TimeInfo start_time_info,
@@ -264,8 +280,9 @@
ASSERT_EQ(end_time_info.day_index, 100);
CB_ASSERT_OK_AND_ASSIGN(std::vector<TimeInfo> backfill,
- manager.CalculateBackfill(start_time_info, end_time, converter, metric,
+ manager.CalculateBackfill(start_time_info, &civil_time_mgr, metric,
/*is_daily=*/false, /*is_expedited=*/false));
+
EXPECT_EQ(ToHourIds(backfill), std::vector<uint32_t>({4801, 4803}));
}
@@ -282,14 +299,16 @@
start_time + std::chrono::hours(kNumHoursToGenerate + 1);
util::FakeCivilTimeConverter converter(/*start_utc_offset=*/0, /*start_isdst=*/true);
+ CivilTimeManager civil_time_mgr(end_time, converter);
CB_ASSERT_OK_AND_ASSIGN(TimeInfo start_time_info,
TimeInfo::FromTimePoint(start_time, converter, metric));
ASSERT_EQ(start_time_info.hour_id, 10000);
CB_ASSERT_OK_AND_ASSIGN(std::vector<TimeInfo> backfill,
- manager.CalculateBackfill(start_time_info, end_time, converter, metric,
+ manager.CalculateBackfill(start_time_info, &civil_time_mgr, metric,
/*is_daily=*/false, /*is_expedited=*/false));
+
EXPECT_EQ(ToHourIds(backfill), std::vector<uint32_t>({10002, 10004, 10006, 10008, 10010}));
}
@@ -311,14 +330,16 @@
/*start_utc_offset=*/0, /*start_isdst=*/false,
/*end_utc_offset=*/1, /*end_isdst=*/true,
/*threshold=*/start_time + 2 * util::kOneHour);
+ CivilTimeManager civil_time_mgr(end_time, converter);
CB_ASSERT_OK_AND_ASSIGN(TimeInfo start_time_info,
TimeInfo::FromTimePoint(start_time, converter, metric));
ASSERT_EQ(start_time_info.hour_id, 10001);
CB_ASSERT_OK_AND_ASSIGN(std::vector<TimeInfo> backfill,
- manager.CalculateBackfill(start_time_info, end_time, converter, metric,
+ manager.CalculateBackfill(start_time_info, &civil_time_mgr, metric,
/*is_daily=*/false, /*is_expedited=*/false));
+
// The first hour ID in the backfill period is odd (not DST) and the remainder are even (DST).
EXPECT_EQ(ToHourIds(backfill), std::vector<uint32_t>({10003, 10006, 10008, 10010, 10012}));
}
@@ -341,14 +362,16 @@
/*start_utc_offset=*/1, /*start_isdst=*/true,
/*end_utc_offset=*/0, /*end_isdst=*/false,
/*threshold=*/start_time + std::chrono::hours(2));
+ CivilTimeManager civil_time_mgr(end_time, converter);
CB_ASSERT_OK_AND_ASSIGN(TimeInfo start_time_info,
TimeInfo::FromTimePoint(start_time, converter, metric));
ASSERT_EQ(start_time_info.hour_id, 10002);
CB_ASSERT_OK_AND_ASSIGN(std::vector<TimeInfo> backfill,
- manager.CalculateBackfill(start_time_info, end_time, converter, metric,
+ manager.CalculateBackfill(start_time_info, &civil_time_mgr, metric,
/*is_daily=*/false, /*is_expedited=*/false));
+
// The first hour ID in the backfill period is even (DST) and the remainder are odd (not DST).
EXPECT_EQ(ToHourIds(backfill), std::vector<uint32_t>({10004, 10005, 10007, 10009, 10011}));
}
@@ -370,11 +393,12 @@
/*start_utc_offset=*/0, /*start_isdst=*/false,
/*end_utc_offset=*/1, /*end_isdst=*/true,
/*threshold=*/start_time + 2 * util::kOneHour);
+ CivilTimeManager civil_time_mgr(end_time, converter);
CB_ASSERT_OK_AND_ASSIGN(TimeInfo last_time_info,
TimeInfo::FromTimePoint(start_time, converter, metric));
CB_ASSERT_OK_AND_ASSIGN(std::vector<TimeInfo> backfill,
- manager.CalculateBackfill(last_time_info, end_time, converter, metric,
+ manager.CalculateBackfill(last_time_info, &civil_time_mgr, metric,
/*is_daily=*/false, /*is_expedited=*/false));
// Generate all possible hour IDs from the start to end of the time window.
@@ -407,11 +431,12 @@
/*start_utc_offset=*/1, /*start_isdst=*/true,
/*end_utc_offset=*/0, /*end_isdst=*/false,
/*threshold=*/start_time + std::chrono::hours(2));
+ CivilTimeManager civil_time_mgr(end_time, converter);
CB_ASSERT_OK_AND_ASSIGN(TimeInfo last_time_info,
TimeInfo::FromTimePoint(start_time, converter, metric));
CB_ASSERT_OK_AND_ASSIGN(std::vector<TimeInfo> backfill,
- manager.CalculateBackfill(last_time_info, end_time, converter, metric,
+ manager.CalculateBackfill(last_time_info, &civil_time_mgr, metric,
/*is_daily=*/false, /*is_expedited=*/false));
// Generate all possible hour IDs from the start to end of the time window.
@@ -436,13 +461,14 @@
const uint32_t kNumDaysToGenerate = 5;
std::chrono::system_clock::time_point end_time(
std::chrono::hours(util::kNumHoursPerDay * (kStartDayIndex + kNumDaysToGenerate + 1)));
+ CivilTimeManager civil_time_mgr(end_time, converter);
CB_ASSERT_OK_AND_ASSIGN(
std::vector<TimeInfo> backfill,
- manager.CalculateBackfill(TimeInfo::FromDayIndex(kStartDayIndex), end_time, converter,
+ manager.CalculateBackfill(TimeInfo::FromDayIndex(kStartDayIndex), &civil_time_mgr,
MetricWithTimeZonePolicy(MetricDefinition::OTHER_TIME_ZONE),
- /*is_daily=*/true,
- /*is_expedited=*/false));
+ /*is_daily=*/true, /*is_expedited=*/false));
+
EXPECT_EQ(ToDayIndices(backfill), std::vector<uint32_t>({10002, 10003, 10004, 10005, 10006}));
}
@@ -458,6 +484,7 @@
const uint32_t kNumDaysToGenerate = 5;
std::chrono::system_clock::time_point end_time(
std::chrono::hours(util::kNumHoursPerDay * (kStartDayIndex + kNumDaysToGenerate + 1)));
+ CivilTimeManager civil_time_mgr(end_time, converter);
// The day index of `end_time` in `metric`'s time zone is 10006 (rather than 10007 in UTC).
CB_ASSERT_OK_AND_ASSIGN(uint32_t end_day_index,
@@ -466,8 +493,9 @@
CB_ASSERT_OK_AND_ASSIGN(
std::vector<TimeInfo> backfill,
- manager.CalculateBackfill(TimeInfo::FromDayIndex(kStartDayIndex), end_time, converter, metric,
+ manager.CalculateBackfill(TimeInfo::FromDayIndex(kStartDayIndex), &civil_time_mgr, metric,
/*is_daily=*/true, /*is_expedited=*/false));
+
// Since the day index of `end_time` is 10006, only return 4 day indices (instead of 5 as would be
// expected for UTC).
EXPECT_EQ(ToDayIndices(backfill), std::vector<uint32_t>({10002, 10003, 10004, 10005}));
@@ -485,6 +513,7 @@
const uint32_t kNumDaysToGenerate = 5;
std::chrono::system_clock::time_point end_time(
std::chrono::hours(util::kNumHoursPerDay * (kStartDayIndex + kNumDaysToGenerate + 1)));
+ CivilTimeManager civil_time_mgr(end_time, converter);
// The day index of `end_time` in `metric`'s time zone is 10006.
CB_ASSERT_OK_AND_ASSIGN(uint32_t end_day_index,
@@ -493,10 +522,43 @@
CB_ASSERT_OK_AND_ASSIGN(
std::vector<TimeInfo> backfill,
- manager.CalculateBackfill(TimeInfo::FromDayIndex(kStartDayIndex), end_time, converter, metric,
+ manager.CalculateBackfill(TimeInfo::FromDayIndex(kStartDayIndex), &civil_time_mgr, metric,
/*is_daily=*/true, /*is_expedited=*/false));
+
// Only the previous day index, 10005, is permitted by the 2-day backfill window.
EXPECT_EQ(ToDayIndices(backfill), std::vector<uint32_t>({10005}));
}
+// Test that the BackfillManager gives the expected result when TimeInfos for past hours are read
+// from the CivilTimeManager's cache.
+TEST(BackfillManager, HourlyBackfillWithCaching) {
+ const uint32_t kNumHoursToGenerate = 5;
+ BackfillManager manager(2);
+ MetricDefinition metric = MetricWithTimeZonePolicy(MetricDefinition::OTHER_TIME_ZONE);
+ metric.set_other_time_zone("other_tz");
+
+ std::chrono::system_clock::time_point start_time(
+ std::chrono::hours(10001 / util::kHourIdsPerHour));
+ std::chrono::system_clock::time_point end_time =
+ start_time + std::chrono::hours(kNumHoursToGenerate + 1);
+
+ util::FakeCivilTimeConverter converter(/*start_utc_offset=*/0, /*start_isdst=*/true);
+ CivilTimeManager civil_time_mgr(end_time, converter);
+
+ CB_ASSERT_OK_AND_ASSIGN(TimeInfo start_time_info,
+ TimeInfo::FromTimePoint(start_time, converter, metric));
+
+ CB_ASSERT_OK_AND_ASSIGN(std::vector<TimeInfo> backfill,
+ manager.CalculateBackfill(start_time_info, &civil_time_mgr, metric,
+ /*is_daily=*/false, /*is_expedited=*/false));
+ EXPECT_EQ(ToHourIds(backfill), std::vector<uint32_t>({10002, 10004, 10006, 10008, 10010}));
+
+ // When we compute backfill for an hourly metric with the same time zone policy, the
+ // CivilTimeManager fetches the TimeInfo for each past hour from its cache.
+ CB_ASSERT_OK_AND_ASSIGN(std::vector<TimeInfo> backfill_2,
+ manager.CalculateBackfill(start_time_info, &civil_time_mgr, metric,
+ /*is_daily=*/false, /*is_expedited=*/false));
+ EXPECT_EQ(ToHourIds(backfill_2), ToHourIds(backfill));
+}
+
} // namespace cobalt::local_aggregation
diff --git a/src/local_aggregation_1_1/civil_time_manager.h b/src/local_aggregation_1_1/civil_time_manager.h
new file mode 100644
index 0000000..683281a
--- /dev/null
+++ b/src/local_aggregation_1_1/civil_time_manager.h
@@ -0,0 +1,68 @@
+// Copyright 2022 The Fuchsia Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COBALT_SRC_LOCAL_AGGREGATION_1_1_CIVIL_TIME_MANAGER_H_
+#define COBALT_SRC_LOCAL_AGGREGATION_1_1_CIVIL_TIME_MANAGER_H_
+
+#include "src/lib/util/datetime_util.h"
+#include "src/public/lib/clock_interfaces.h"
+#include "src/public/lib/statusor/status_macros.h"
+#include "src/public/lib/statusor/statusor.h"
+#include "src/registry/metric_definition.pb.h"
+
+namespace cobalt::local_aggregation {
+
+// A CivilTimeManager converts a specific initial time point to `TimeInfo`s for given time
+// zones. It also provides and caches the `TimeInfo`s corresponding to time points at hourly
+// increments before the initial time.
+class CivilTimeManager {
+ public:
+ CivilTimeManager(std::chrono::system_clock::time_point initial_time,
+ const util::CivilTimeConverterInterface& civil_time_converter)
+ : initial_time_(initial_time), civil_time_converter_(civil_time_converter) {
+ cache_["UTC"][0] = util::TimeInfo::FromTimePointUtc(initial_time);
+ }
+
+ // Returns the `TimeInfo` at the initial time with respect to UTC.
+ util::TimeInfo GetInitialUtc() { return cache_["UTC"][0]; }
+
+ // Returns the `TimeInfo` at the initial time minus `past_hours`, with respect to the time zone
+ // policy of `metric`.
+ lib::statusor::StatusOr<util::TimeInfo> GetTimeInfo(int past_hours,
+ const MetricDefinition& metric) {
+ std::string time_zone_id = GetTimeZoneId(metric);
+ if (cache_[time_zone_id].count(past_hours) == 0) {
+ CB_ASSIGN_OR_RETURN(util::TimeInfo time_info,
+ util::TimeInfo::FromTimePoint(initial_time_ - util::kOneHour * past_hours,
+ civil_time_converter_, metric));
+ cache_[time_zone_id].try_emplace(past_hours, time_info);
+ return time_info;
+ }
+ return cache_[time_zone_id][past_hours];
+ }
+
+ private:
+ static std::string GetTimeZoneId(const MetricDefinition& metric) {
+ switch (metric.time_zone_policy()) {
+ case MetricDefinition::UTC:
+ return "UTC";
+ case MetricDefinition::LOCAL:
+ return "LOCAL";
+ case MetricDefinition::OTHER_TIME_ZONE:
+ return metric.other_time_zone();
+ default:
+ return "";
+ }
+ }
+
+ std::chrono::system_clock::time_point initial_time_;
+ const util::CivilTimeConverterInterface& civil_time_converter_;
+ // The top-level keys are time zone identifiers. For each time zone id, the entry at index `i`
+ // is the TimeInfo corresponding to the hour ID of `initial_time_` minus `i` hours.
+ std::map<std::string, std::map<int, util::TimeInfo>> cache_;
+};
+
+} // namespace cobalt::local_aggregation
+
+#endif // COBALT_SRC_LOCAL_AGGREGATION_1_1_CIVIL_TIME_MANAGER_H_
diff --git a/src/local_aggregation_1_1/civil_time_manager_test.cc b/src/local_aggregation_1_1/civil_time_manager_test.cc
new file mode 100644
index 0000000..e488d6c
--- /dev/null
+++ b/src/local_aggregation_1_1/civil_time_manager_test.cc
@@ -0,0 +1,172 @@
+// Copyright 2022 The Fuchsia Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "src/local_aggregation_1_1/civil_time_manager.h"
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+#include "src/lib/util/clock.h"
+#include "src/lib/util/datetime_util.h"
+#include "src/public/lib/clock_interfaces.h"
+#include "src/public/lib/statusor/status_macros.h"
+#include "src/public/lib/statusor/statusor.h"
+
+namespace cobalt::local_aggregation {
+namespace {
+
+using lib::statusor::StatusOr;
+using ::testing::DefaultValue;
+using util::TimeInfo;
+
+constexpr std::chrono::system_clock::time_point kInitialTime(std::chrono::hours(10000));
+
+class MockCivilTimeConverter : public util::CivilTimeConverterInterface {
+ public:
+ MOCK_METHOD(lib::statusor::StatusOr<std::tm>, civil_time,
+ (std::chrono::system_clock::time_point time, const std::string& time_zone),
+ (const, override));
+};
+
+TEST(CivilTimeManager, GetInitialUtc) {
+ MockCivilTimeConverter converter;
+ CivilTimeManager manager(kInitialTime, converter);
+ TimeInfo initial_info = manager.GetInitialUtc();
+ TimeInfo expected_info = TimeInfo::FromTimePointUtc(kInitialTime);
+
+ EXPECT_EQ(initial_info.day_index, expected_info.day_index);
+ EXPECT_EQ(initial_info.hour_id, expected_info.hour_id);
+ EXPECT_CALL(converter, civil_time).Times(0);
+}
+
+TEST(CivilTimeManager, GetTimeInfoUtc) {
+ MockCivilTimeConverter converter;
+ CivilTimeManager manager(kInitialTime, converter);
+
+ MetricDefinition metric;
+ metric.set_time_zone_policy(MetricDefinition::UTC);
+
+ // Get some UTC TimeInfos. This should not invoke the civil time converter.
+ EXPECT_CALL(converter, civil_time).Times(0);
+ CB_ASSERT_OK_AND_ASSIGN(TimeInfo info_0, manager.GetTimeInfo(0, metric));
+ CB_ASSERT_OK_AND_ASSIGN(TimeInfo info_1, manager.GetTimeInfo(1, metric));
+ CB_ASSERT_OK_AND_ASSIGN(TimeInfo info_3, manager.GetTimeInfo(3, metric));
+
+ // Get the same UTC TimeInfos again, this time from the cache.
+ CB_ASSERT_OK_AND_ASSIGN(TimeInfo cached_0, manager.GetTimeInfo(0, metric));
+ CB_ASSERT_OK_AND_ASSIGN(TimeInfo cached_1, manager.GetTimeInfo(1, metric));
+ CB_ASSERT_OK_AND_ASSIGN(TimeInfo cached_3, manager.GetTimeInfo(3, metric));
+
+ // Check that the TimeInfos are correct.
+ TimeInfo expected_0 = TimeInfo::FromTimePointUtc(kInitialTime);
+ TimeInfo expected_1 = TimeInfo::FromTimePointUtc(kInitialTime - util::kOneHour);
+ TimeInfo expected_3 = TimeInfo::FromTimePointUtc(kInitialTime - 3 * util::kOneHour);
+
+ EXPECT_EQ(info_0.day_index, expected_0.day_index);
+ EXPECT_EQ(info_0.hour_id, expected_0.hour_id);
+ EXPECT_EQ(info_1.day_index, expected_1.day_index);
+ EXPECT_EQ(info_1.hour_id, expected_1.hour_id);
+ EXPECT_EQ(info_3.day_index, expected_3.day_index);
+ EXPECT_EQ(info_3.hour_id, expected_3.hour_id);
+
+ EXPECT_EQ(cached_0.day_index, expected_0.day_index);
+ EXPECT_EQ(cached_0.hour_id, expected_0.hour_id);
+ EXPECT_EQ(cached_1.day_index, expected_1.day_index);
+ EXPECT_EQ(cached_1.hour_id, expected_1.hour_id);
+ EXPECT_EQ(cached_3.day_index, expected_3.day_index);
+ EXPECT_EQ(cached_3.hour_id, expected_3.hour_id);
+}
+
+TEST(CivilTimeManager, GetTimeInfoLocal) {
+ MockCivilTimeConverter converter;
+ CivilTimeManager manager(kInitialTime, converter);
+
+ MetricDefinition metric;
+ metric.set_time_zone_policy(MetricDefinition::LOCAL);
+
+ // Get some TimeInfos in local time. This should not invoke the civil time converter.
+ EXPECT_CALL(converter, civil_time).Times(0);
+ CB_ASSERT_OK_AND_ASSIGN(TimeInfo info_0, manager.GetTimeInfo(0, metric));
+ CB_ASSERT_OK_AND_ASSIGN(TimeInfo info_1, manager.GetTimeInfo(1, metric));
+ CB_ASSERT_OK_AND_ASSIGN(TimeInfo info_3, manager.GetTimeInfo(3, metric));
+
+ // Get the same TimeInfos again, this time from the cache.
+ CB_ASSERT_OK_AND_ASSIGN(TimeInfo cached_0, manager.GetTimeInfo(0, metric));
+ CB_ASSERT_OK_AND_ASSIGN(TimeInfo cached_1, manager.GetTimeInfo(1, metric));
+ CB_ASSERT_OK_AND_ASSIGN(TimeInfo cached_3, manager.GetTimeInfo(3, metric));
+
+ // Check that the TimeInfos are correct.
+ CB_ASSERT_OK_AND_ASSIGN(TimeInfo expected_0,
+ TimeInfo::FromTimePoint(kInitialTime, converter, metric));
+ CB_ASSERT_OK_AND_ASSIGN(
+ TimeInfo expected_1,
+ TimeInfo::FromTimePoint(kInitialTime - util::kOneHour, converter, metric));
+ CB_ASSERT_OK_AND_ASSIGN(
+ TimeInfo expected_3,
+ TimeInfo::FromTimePoint(kInitialTime - 3 * util::kOneHour, converter, metric));
+
+ EXPECT_EQ(info_0.day_index, expected_0.day_index);
+ EXPECT_EQ(info_0.hour_id, expected_0.hour_id);
+ EXPECT_EQ(info_1.day_index, expected_1.day_index);
+ EXPECT_EQ(info_1.hour_id, expected_1.hour_id);
+ EXPECT_EQ(info_3.day_index, expected_3.day_index);
+ EXPECT_EQ(info_3.hour_id, expected_3.hour_id);
+
+ EXPECT_EQ(cached_0.day_index, expected_0.day_index);
+ EXPECT_EQ(cached_0.hour_id, expected_0.hour_id);
+ EXPECT_EQ(cached_1.day_index, expected_1.day_index);
+ EXPECT_EQ(cached_1.hour_id, expected_1.hour_id);
+ EXPECT_EQ(cached_3.day_index, expected_3.day_index);
+ EXPECT_EQ(cached_3.hour_id, expected_3.hour_id);
+}
+
+TEST(CivilTimeManager, GetTimeInfoOtherTimeZone) {
+ const uint32_t kDayIndex = 100;
+ std::tm time_struct;
+ std::time_t time_t = std::chrono::system_clock::to_time_t(
+ std::chrono::system_clock::time_point(util::kOneDay * kDayIndex));
+ gmtime_r(&time_t, &time_struct);
+ DefaultValue<lib::statusor::StatusOr<std::tm>>::Set(time_struct);
+
+ TimeInfo expected = TimeInfo::FromDayIndex(kDayIndex);
+
+ MockCivilTimeConverter converter;
+ CivilTimeManager manager(kInitialTime, converter);
+
+ MetricDefinition metric;
+ metric.set_time_zone_policy(MetricDefinition::OTHER_TIME_ZONE);
+ metric.set_other_time_zone("other");
+
+ // Get some TimeInfos in the "other" time zone, then get the TimeInfos again. This should invoke
+ // the civil time converter just once for each time offset.
+ EXPECT_CALL(converter, civil_time(kInitialTime, "other")).Times(1);
+ EXPECT_CALL(converter, civil_time(kInitialTime - util::kOneHour, "other")).Times(1);
+ EXPECT_CALL(converter, civil_time(kInitialTime - 3 * util::kOneHour, "other")).Times(1);
+ CB_ASSERT_OK_AND_ASSIGN(TimeInfo info_0, manager.GetTimeInfo(0, metric));
+ CB_ASSERT_OK_AND_ASSIGN(TimeInfo info_1, manager.GetTimeInfo(1, metric));
+ CB_ASSERT_OK_AND_ASSIGN(TimeInfo info_3, manager.GetTimeInfo(3, metric));
+
+ CB_ASSERT_OK_AND_ASSIGN(TimeInfo cached_0, manager.GetTimeInfo(0, metric));
+ CB_ASSERT_OK_AND_ASSIGN(TimeInfo cached_1, manager.GetTimeInfo(1, metric));
+ CB_ASSERT_OK_AND_ASSIGN(TimeInfo cached_3, manager.GetTimeInfo(3, metric));
+
+ // The TimeInfos should all be equal to `expected`, since the mock converter always returns the
+ // same civil time for an OTHER_TIME_ZONE metric.
+ EXPECT_EQ(info_0.day_index, expected.day_index);
+ EXPECT_EQ(info_0.hour_id, expected.hour_id);
+ EXPECT_EQ(info_1.day_index, expected.day_index);
+ EXPECT_EQ(info_1.hour_id, expected.hour_id);
+ EXPECT_EQ(info_3.day_index, expected.day_index);
+ EXPECT_EQ(info_3.hour_id, expected.hour_id);
+
+ EXPECT_EQ(cached_0.day_index, expected.day_index);
+ EXPECT_EQ(cached_0.hour_id, expected.hour_id);
+ EXPECT_EQ(cached_1.day_index, expected.day_index);
+ EXPECT_EQ(cached_1.hour_id, expected.hour_id);
+ EXPECT_EQ(cached_3.day_index, expected.day_index);
+ EXPECT_EQ(cached_3.hour_id, expected.hour_id);
+}
+
+} // namespace
+} // namespace cobalt::local_aggregation
diff --git a/src/local_aggregation_1_1/observation_generator.cc b/src/local_aggregation_1_1/observation_generator.cc
index c826092..8f61b0d 100644
--- a/src/local_aggregation_1_1/observation_generator.cc
+++ b/src/local_aggregation_1_1/observation_generator.cc
@@ -104,12 +104,10 @@
return Status::OkStatus();
}
-// TODO(fxbug.dev/92131): Cache `TimeInfo`s corresponding to `system_time` in the time zones
-// used by each metric, and consider precomputing and caching `TimeInfo`s for at least one hour
-// in the past.
Status ObservationGenerator::GenerateObservationsOnce(
std::chrono::system_clock::time_point system_time) {
- util::TimeInfo utc_time_info = util::TimeInfo::FromTimePointUtc(system_time);
+ CivilTimeManager civil_time_mgr(system_time, civil_time_converter_);
+ util::TimeInfo utc_time_info = civil_time_mgr.GetInitialUtc();
LOG(INFO) << "Generating aggregated observations for periods ending before system time: "
<< std::chrono::system_clock::to_time_t(system_time) << " (day index "
<< utc_time_info.day_index << ", hour ID " << utc_time_info.hour_id << " in UTC)";
@@ -139,8 +137,9 @@
ReportAggregate& report_aggregate =
aggregate.aggregate()->mutable_by_report_id()->at(report.id());
- Status status = GenerateObservationsForReportAggregate(
- aggregate, report_aggregate, *procedure, system_time, *metric, metric_ref, report);
+ Status status = GenerateObservationsForReportAggregate(aggregate, report_aggregate,
+ *procedure, &civil_time_mgr,
+ *metric, metric_ref, report);
if (!status.ok()) {
generation_errors.push_back(status);
@@ -172,7 +171,7 @@
Status ObservationGenerator::GenerateObservationsForReportAggregate(
const LocalAggregateStorage::MetricAggregateRef& aggregate, ReportAggregate& report_aggregate,
- AggregationProcedure& procedure, std::chrono::system_clock::time_point end_time,
+ AggregationProcedure& procedure, CivilTimeManager* civil_time_mgr,
const MetricDefinition& metric, const logger::MetricRef& metric_ref,
const ReportDefinition& report) {
SystemProfile current_filtered_system_profile;
@@ -180,11 +179,10 @@
¤t_filtered_system_profile);
std::map<uint64_t, SystemProfile> system_profile_cache;
- CB_ASSIGN_OR_RETURN(
- std::vector<util::TimeInfo> to_generate,
- backfill_manager_.CalculateBackfill(procedure.GetLastTimeInfo(report_aggregate), end_time,
- civil_time_converter_, metric, procedure.IsDaily(),
- procedure.IsExpedited()));
+ CB_ASSIGN_OR_RETURN(std::vector<util::TimeInfo> to_generate,
+ backfill_manager_.CalculateBackfill(
+ procedure.GetLastTimeInfo(report_aggregate), civil_time_mgr, metric,
+ procedure.IsDaily(), procedure.IsExpedited()));
for (util::TimeInfo time_info : to_generate) {
CB_ASSIGN_OR_RETURN(std::vector<ObservationAndSystemProfile> observations,
diff --git a/src/local_aggregation_1_1/observation_generator.h b/src/local_aggregation_1_1/observation_generator.h
index 67ac7d4..3d715c6 100644
--- a/src/local_aggregation_1_1/observation_generator.h
+++ b/src/local_aggregation_1_1/observation_generator.h
@@ -11,6 +11,7 @@
#include "src/lib/util/clock.h"
#include "src/local_aggregation_1_1/aggregation_procedures/aggregation_procedure.h"
#include "src/local_aggregation_1_1/backfill_manager.h"
+#include "src/local_aggregation_1_1/civil_time_manager.h"
#include "src/local_aggregation_1_1/local_aggregate_storage/local_aggregate_storage.h"
#include "src/logger/observation_writer.h"
#include "src/logger/privacy_encoder.h"
@@ -86,7 +87,7 @@
// ReportAggregate, returning a Status::OK if all observations were generated.
Status GenerateObservationsForReportAggregate(
const LocalAggregateStorage::MetricAggregateRef& aggregate, ReportAggregate& report_aggregate,
- AggregationProcedure& procedure, std::chrono::system_clock::time_point end_time,
+ AggregationProcedure& procedure, CivilTimeManager* civil_time_mgr,
const MetricDefinition& metric, const logger::MetricRef& metric_ref,
const ReportDefinition& report);
diff --git a/src/public/cobalt_service.cc b/src/public/cobalt_service.cc
index 1a208e6..b265169 100644
--- a/src/public/cobalt_service.cc
+++ b/src/public/cobalt_service.cc
@@ -265,7 +265,7 @@
// the time passed to `GenerateAggregatedObservations` must belong to the following day. Use the
// first hour of the day after `final_day_index_utc` to generate observations.
status = local_aggregation_.GenerateAggregatedObservations(util::FromUnixSeconds(
- util::HourIdToUnixSeconds(util::DayIndexToHourId(final_day_index_utc + 1))));
+ util::HourIdToUnixSeconds(util::DayIndexToHourIdNoDst(final_day_index_utc + 1))));
if (!status.ok()) {
LOG(ERROR) << "Got error while trying to GenerateAggregatedObservations: " << status;
