[NoCheck] Pass CivilTimeConverterInterface by reference
- Removes 2 CHECK calls
- Stores CivilTimeConverterInterface in a util::PinnedUniquePtr
- Removes CobaltService::civil_time_converter() that was only used in tests.
Change-Id: Id46533c09e54efcabec5530e687b20b880bd622f
Reviewed-on: https://fuchsia-review.googlesource.com/c/cobalt/+/662582
Fuchsia-Auto-Submit: Zach Bush <zmbush@google.com>
Reviewed-by: Cameron Dale <camrdale@google.com>
Commit-Queue: Auto-Submit <auto-submit@fuchsia-infra.iam.gserviceaccount.com>
diff --git a/src/local_aggregation_1_1/local_aggregation.cc b/src/local_aggregation_1_1/local_aggregation.cc
index 8b4af42..9a1df7e 100644
--- a/src/local_aggregation_1_1/local_aggregation.cc
+++ b/src/local_aggregation_1_1/local_aggregation.cc
@@ -28,7 +28,7 @@
const CobaltConfig &cfg, const logger::ProjectContextFactory *global_project_context_factory,
system_data::SystemDataInterface &system_data, MetadataBuilder &metadata_builder,
util::FileSystem &fs, const logger::ObservationWriter &observation_writer,
- util::CivilTimeConverterInterface *civil_time_converter,
+ util::CivilTimeConverterInterface &civil_time_converter,
logger::InternalMetrics *internal_metrics)
: global_project_context_factory_(global_project_context_factory),
aggregate_storage_(LocalAggregateStorage::New(
diff --git a/src/local_aggregation_1_1/local_aggregation.h b/src/local_aggregation_1_1/local_aggregation.h
index 189ead2..2bbedf8 100644
--- a/src/local_aggregation_1_1/local_aggregation.h
+++ b/src/local_aggregation_1_1/local_aggregation.h
@@ -41,7 +41,7 @@
const logger::ProjectContextFactory *global_project_context_factory,
system_data::SystemDataInterface &system_data, MetadataBuilder &metadata_builder,
util::FileSystem &fs, const logger::ObservationWriter &observation_writer,
- util::CivilTimeConverterInterface *civil_time_converter,
+ util::CivilTimeConverterInterface &civil_time_converter,
logger::InternalMetrics *internal_metrics = nullptr);
// Start should be called when the system is ready to start the background process for generating
diff --git a/src/local_aggregation_1_1/local_aggregation_test.cc b/src/local_aggregation_1_1/local_aggregation_test.cc
index 5f3c99b..cc650c0 100644
--- a/src/local_aggregation_1_1/local_aggregation_test.cc
+++ b/src/local_aggregation_1_1/local_aggregation_test.cc
@@ -97,7 +97,7 @@
return std::make_unique<LocalAggregation>(cfg, global_project_context_factory_.get(),
system_data_, *metadata_builder_, fs(),
- *observation_writer_, civil_time_converter_.get());
+ *observation_writer_, *civil_time_converter_);
}
void OverrideRegistry(const std::string& registryBase64) {
diff --git a/src/local_aggregation_1_1/observation_generator.cc b/src/local_aggregation_1_1/observation_generator.cc
index 180fd17..1b427c3 100644
--- a/src/local_aggregation_1_1/observation_generator.cc
+++ b/src/local_aggregation_1_1/observation_generator.cc
@@ -34,7 +34,7 @@
system_data::SystemDataInterface& system_data,
const logger::ObservationWriter& observation_writer,
std::unique_ptr<logger::PrivacyEncoder> privacy_encoder,
- util::CivilTimeConverterInterface* civil_time_converter,
+ util::CivilTimeConverterInterface& civil_time_converter,
bool generate_observations_with_current_system_profile, uint32_t backfill_manager_days)
: aggregate_storage_(aggregate_storage),
global_project_context_factory_(global_project_context_factory),
@@ -183,7 +183,7 @@
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()));
+ civil_time_converter_, metric, procedure.IsDaily()));
// In general, don't generate observations for the hour ID (if hourly) or the day index (if daily)
// corresponding to the current time. However, if `report` is a daily expedited report, then do
@@ -192,7 +192,7 @@
// TODO(fxbug.dev/92131): Move this conditional into BackfillManager.
if (procedure.IsDaily() && procedure.IsExpedited()) {
CB_ASSIGN_OR_RETURN(util::TimeInfo end_time_info,
- util::TimeInfo::FromTimePoint(end_time, *civil_time_converter_, metric));
+ util::TimeInfo::FromTimePoint(end_time, civil_time_converter_, metric));
to_generate.push_back(end_time_info);
}
diff --git a/src/local_aggregation_1_1/observation_generator.h b/src/local_aggregation_1_1/observation_generator.h
index 0bf05a9..67ac7d4 100644
--- a/src/local_aggregation_1_1/observation_generator.h
+++ b/src/local_aggregation_1_1/observation_generator.h
@@ -50,7 +50,7 @@
system_data::SystemDataInterface& system_data,
const logger::ObservationWriter& observation_writer,
std::unique_ptr<logger::PrivacyEncoder> privacy_encoder,
- util::CivilTimeConverterInterface* civil_time_converter,
+ util::CivilTimeConverterInterface& civil_time_converter,
bool generate_observations_with_current_system_profile,
uint32_t backfill_manager_days = kDefaultBackfillManagerDays);
@@ -114,7 +114,7 @@
std::chrono::seconds generate_obs_interval_;
BackfillManager backfill_manager_;
std::unique_ptr<logger::PrivacyEncoder> privacy_encoder_;
- util::CivilTimeConverterInterface* civil_time_converter_;
+ util::CivilTimeConverterInterface& civil_time_converter_;
bool generate_observations_with_current_system_profile_;
std::thread worker_thread_;
diff --git a/src/local_aggregation_1_1/observation_generator_test.cc b/src/local_aggregation_1_1/observation_generator_test.cc
index 2abbae3..5bf928f 100644
--- a/src/local_aggregation_1_1/observation_generator_test.cc
+++ b/src/local_aggregation_1_1/observation_generator_test.cc
@@ -143,7 +143,7 @@
void ConstructObservationGenerator(
const logger::ObservationWriter& observation_writer,
std::unique_ptr<FakePrivacyEncoder> privacy_encoder,
- util::CivilTimeConverterInterface* civil_time_converter,
+ util::CivilTimeConverterInterface& civil_time_converter,
bool generate_observations_with_current_system_profile = false) {
observation_generator_ = std::make_unique<ObservationGenerator>(
*aggregate_storage_, project_context_factory_.get(), system_data_, observation_writer,
@@ -265,7 +265,7 @@
logger::ObservationWriter observation_writer(test_writer, nullptr);
ConstructObservationGenerator(observation_writer, std::make_unique<FakePrivacyEncoder>(false),
- converter.get());
+ *converter);
for (uint32_t i = 0; i <= kMaxHourIdOffset; i += 2 * kHourIdsPerHour) {
// Generate observations for the hour ending at `end_time`.
@@ -345,7 +345,7 @@
logger::ObservationWriter observation_writer(test_writer, nullptr);
ConstructObservationGenerator(observation_writer, std::make_unique<FakePrivacyEncoder>(false),
- converter.get());
+ *converter);
SystemProfile system_profile;
system_profile.set_os(SystemProfile::FUCHSIA);
@@ -457,7 +457,7 @@
logger::ObservationWriter observation_writer(test_writer, nullptr);
ConstructObservationGenerator(observation_writer, std::make_unique<FakePrivacyEncoder>(false),
- converter.get());
+ *converter);
SystemProfile system_profile;
system_profile.set_os(SystemProfile::FUCHSIA);
@@ -586,7 +586,7 @@
logger::ObservationWriter observation_writer(test_writer, nullptr);
ConstructObservationGenerator(observation_writer, std::make_unique<FakePrivacyEncoder>(false),
- converter.get());
+ *converter);
for (uint32_t i = 0; i <= kMaxDayIndexOffset; i += 1) {
// Generate observations for the day ending at `end_time`.
@@ -667,7 +667,7 @@
logger::ObservationWriter observation_writer(test_writer, nullptr);
ConstructObservationGenerator(observation_writer, std::make_unique<FakePrivacyEncoder>(false),
- converter.get());
+ *converter);
for (uint32_t i = 0; i <= kNumDays; ++i) {
GenerateObservationsOnce(starting_time_ + std::chrono::hours(util::kNumHoursPerDay * kNumDays));
@@ -732,7 +732,7 @@
logger::ObservationWriter observation_writer(test_writer, nullptr);
ConstructObservationGenerator(observation_writer, std::make_unique<FakePrivacyEncoder>(false),
- converter.get());
+ *converter);
// This generation should fail because the aggregation procedure
// couldn't be created.
@@ -744,7 +744,7 @@
// Reset to default registry.
SetRegistry();
ConstructObservationGenerator(observation_writer, std::make_unique<FakePrivacyEncoder>(false),
- converter.get());
+ *converter);
// Data should still be collected as expected.
for (uint32_t i = start_day_index; i < start_day_index + kNumDays; i += 1) {
@@ -821,7 +821,7 @@
logger::ObservationWriter observation_writer(test_writer, nullptr);
ConstructObservationGenerator(observation_writer, std::make_unique<FakePrivacyEncoder>(false),
- converter.get());
+ *converter);
for (uint32_t i = start_day_index; i < start_day_index + kNumDays; i += 1) {
GenerateObservationsOnce(util::FromUnixSeconds(util::DayIndexToUnixSeconds(i + 1)));
@@ -890,9 +890,8 @@
});
logger::ObservationWriter observation_writer(test_writer, nullptr);
- ConstructObservationGenerator(observation_writer,
- std::make_unique<FakePrivacyEncoder>(true, kNumPrivateObs),
- converter.get());
+ ConstructObservationGenerator(
+ observation_writer, std::make_unique<FakePrivacyEncoder>(true, kNumPrivateObs), *converter);
GenerateObservationsOnce(starting_time_ + util::kOneHour);
EXPECT_EQ(observations.size(), kNumPrivateObs);
@@ -947,9 +946,8 @@
});
logger::ObservationWriter observation_writer(test_writer, nullptr);
- ConstructObservationGenerator(observation_writer,
- std::make_unique<FakePrivacyEncoder>(true, kNumPrivateObs),
- converter.get());
+ ConstructObservationGenerator(
+ observation_writer, std::make_unique<FakePrivacyEncoder>(true, kNumPrivateObs), *converter);
GenerateObservationsOnce(starting_time_ + util::kOneHour);
EXPECT_EQ(observations.size(), kNumPrivateObs);
@@ -1013,7 +1011,7 @@
logger::ObservationWriter observation_writer(test_writer, nullptr);
ConstructObservationGenerator(observation_writer, std::make_unique<FakePrivacyEncoder>(false),
- converter.get());
+ *converter);
GenerateObservationsOnce(starting_time_ + util::kOneDay);
@@ -1081,7 +1079,7 @@
logger::ObservationWriter observation_writer(test_writer, nullptr);
ConstructObservationGenerator(observation_writer, std::make_unique<FakePrivacyEncoder>(false),
- converter.get(),
+ *converter,
/*generate_observations_with_current_system_profile=*/true);
// Generate observations for the day with day index `start_day_index`.
GenerateObservationsOnce(starting_time_ + std::chrono::hours(util::kNumHoursPerDay));
@@ -1159,7 +1157,7 @@
logger::ObservationWriter observation_writer(test_writer, nullptr);
ConstructObservationGenerator(observation_writer, std::make_unique<FakePrivacyEncoder>(false),
- converter.get(),
+ *converter,
/*generate_observations_with_current_system_profile=*/true);
// Generate observations for the day with day index `start_day_index`.
GenerateObservationsOnce(starting_time_ + std::chrono::hours(util::kNumHoursPerDay));
@@ -1264,7 +1262,7 @@
logger::ObservationWriter observation_writer(obs_store, nullptr);
ConstructObservationGenerator(observation_writer, std::make_unique<FakePrivacyEncoder>(false),
- converter.get());
+ *converter);
for (uint32_t i = starting_time_info.hour_id; i < starting_time_info.hour_id + kMaxHourOffset;
i += 4) {
diff --git a/src/logger/event_loggers.cc b/src/logger/event_loggers.cc
index abf71e0..9bc66e0 100644
--- a/src/logger/event_loggers.cc
+++ b/src/logger/event_loggers.cc
@@ -36,7 +36,7 @@
EventAggregator& event_aggregator, local_aggregation::LocalAggregation& local_aggregation,
const ObservationWriter& observation_writer,
const system_data::SystemDataInterface& system_data,
- util::CivilTimeConverterInterface* civil_time_converter) {
+ util::CivilTimeConverterInterface& civil_time_converter) {
switch (metric_type) {
case MetricDefinition::EVENT_OCCURRED: {
return std::make_unique<internal::EventOccurredEventLogger>(
@@ -345,7 +345,7 @@
const std::chrono::system_clock::time_point& event_timestamp) {
// Compute the day_index and hour ID in the appropriate time zone for the metric's TimeZonePolicy.
CB_ASSIGN_OR_RETURN(util::TimeInfo event_time,
- util::TimeInfo::FromTimePoint(event_timestamp, *civil_time_converter_,
+ util::TimeInfo::FromTimePoint(event_timestamp, civil_time_converter_,
*event_record->metric()));
event_record->event()->set_day_index(event_time.day_index);
event_record->event()->set_hour_id(event_time.hour_id);
diff --git a/src/logger/event_loggers.h b/src/logger/event_loggers.h
index add8deb..2e042bf 100644
--- a/src/logger/event_loggers.h
+++ b/src/logger/event_loggers.h
@@ -37,7 +37,7 @@
local_aggregation::LocalAggregation& local_aggregation,
const ObservationWriter& observation_writer,
const system_data::SystemDataInterface& system_data,
- util::CivilTimeConverterInterface* civil_time_converter)
+ util::CivilTimeConverterInterface& civil_time_converter)
: encoder_(encoder),
event_aggregator_(event_aggregator),
local_aggregation_(local_aggregation),
@@ -58,7 +58,7 @@
local_aggregation::LocalAggregation& local_aggregation,
const ObservationWriter& observation_writer,
const system_data::SystemDataInterface& system_data,
- util::CivilTimeConverterInterface* civil_time_converter);
+ util::CivilTimeConverterInterface& civil_time_converter);
// Finds the Metric with the given ID. Expects that this has type
// |expected_metric_type|. If not logs an error and returns.
@@ -156,7 +156,7 @@
local_aggregation::LocalAggregation& local_aggregation_;
const ObservationWriter& observation_writer_;
const system_data::SystemDataInterface& system_data_;
- util::CivilTimeConverterInterface* civil_time_converter_;
+ util::CivilTimeConverterInterface& civil_time_converter_;
};
// Implementation of EventLogger for metrics of type EVENT_OCCURRED.
diff --git a/src/logger/event_loggers_test.cc b/src/logger/event_loggers_test.cc
index df0e2c7..2f3f359 100644
--- a/src/logger/event_loggers_test.cc
+++ b/src/logger/event_loggers_test.cc
@@ -121,12 +121,12 @@
cfg, fs(), *encoder_, *observation_writer_, *metadata_builder_);
local_aggregation_ = std::make_unique<LocalAggregation>(
cfg, project_context_factory_.get(), system_data_, *metadata_builder_, fs(),
- *observation_writer_, civil_time_converter_.get());
+ *observation_writer_, *civil_time_converter_);
project_context_ = GetTestProject(registry_base64);
event_aggregator_mgr_->GetEventAggregator().UpdateAggregationConfigs(*project_context_);
logger_ = std::make_unique<EventLoggerClass>(
*encoder_, event_aggregator_mgr_->GetEventAggregator(), *local_aggregation_,
- *observation_writer_, system_data_, civil_time_converter_.get());
+ *observation_writer_, system_data_, *civil_time_converter_);
}
void TearDown() override {
@@ -1511,14 +1511,14 @@
event_aggregator_mgr_->GetEventAggregator().UpdateAggregationConfigs(*project_context_);
local_aggregation_ = std::make_unique<LocalAggregation>(
cfg, project_context_factory_.get(), system_data_, *metadata_builder_, fs(),
- *observation_writer_, civil_time_converter_.get());
+ *observation_writer_, *civil_time_converter_);
}
std::unique_ptr<EventLogger> GetEventLoggerForMetricType(
MetricDefinition::MetricType metric_type) {
auto logger = EventLogger::Create(
metric_type, *encoder_, event_aggregator_mgr_->GetEventAggregator(), *local_aggregation_,
- *observation_writer_, system_data_, civil_time_converter_.get());
+ *observation_writer_, system_data_, *civil_time_converter_);
return logger;
}
@@ -1755,7 +1755,7 @@
event_aggregator_mgr_->GetEventAggregator().UpdateAggregationConfigs(*project_context_);
local_aggregation_ = std::make_unique<LocalAggregation>(
cfg, project_context_factory_.get(), system_data_, *metadata_builder_, fs(),
- *observation_writer_, civil_time_converter_.get());
+ *observation_writer_, *civil_time_converter_);
}
local_aggregation::ImmediateLocalAggregateStorage GetLocalAggregateStorage() {
@@ -1778,7 +1778,7 @@
MetricDefinition::MetricType metric_type) {
auto logger = EventLogger::Create(
metric_type, *encoder_, event_aggregator_mgr_->GetEventAggregator(), *local_aggregation_,
- *observation_writer_, system_data_, civil_time_converter_.get());
+ *observation_writer_, system_data_, *civil_time_converter_);
return logger;
}
diff --git a/src/logger/logger.cc b/src/logger/logger.cc
index 1b0b87c..f18c162 100644
--- a/src/logger/logger.cc
+++ b/src/logger/logger.cc
@@ -44,7 +44,7 @@
EventAggregator& event_aggregator,
local_aggregation::LocalAggregation& local_aggregation,
ObservationWriter& observation_writer, system_data::SystemDataInterface& system_data,
- util::CivilTimeConverterInterface* civil_time_converter,
+ util::CivilTimeConverterInterface& civil_time_converter,
std::vector<uint32_t> experiment_ids, bool enable_replacement_metrics,
InternalMetrics* internal_metrics)
: Logger(std::move(project_context), encoder, event_aggregator, local_aggregation,
@@ -57,7 +57,7 @@
local_aggregation::LocalAggregation& local_aggregation,
ObservationWriter& observation_writer, system_data::SystemDataInterface& system_data,
util::ValidatedClockInterface* validated_clock,
- util::CivilTimeConverterInterface* civil_time_converter,
+ util::CivilTimeConverterInterface& civil_time_converter,
std::weak_ptr<UndatedEventManager> undated_event_manager,
std::vector<uint32_t> experiment_ids, bool enable_replacement_metrics,
InternalMetrics* internal_metrics)
@@ -74,7 +74,6 @@
enable_replacement_metrics_(enable_replacement_metrics),
internal_metrics_(internal_metrics) {
CHECK(project_context_);
- CHECK(civil_time_converter_);
if (!validated_clock_) {
local_validated_clock_ =
std::make_unique<util::AlwaysAccurateClock>(std::make_unique<util::SystemClock>());
diff --git a/src/logger/logger.h b/src/logger/logger.h
index 07991cd..d7209a4 100644
--- a/src/logger/logger.h
+++ b/src/logger/logger.h
@@ -79,7 +79,7 @@
local_aggregation::EventAggregator& event_aggregator,
local_aggregation::LocalAggregation& local_aggregation,
ObservationWriter& observation_writer, system_data::SystemDataInterface& system_data,
- util::CivilTimeConverterInterface* civil_time_converter,
+ util::CivilTimeConverterInterface& civil_time_converter,
std::vector<uint32_t> experiment_ids, bool enable_replacement_metrics = false,
InternalMetrics* internal_metrics = nullptr);
@@ -129,7 +129,7 @@
local_aggregation::LocalAggregation& local_aggregation,
ObservationWriter& observation_writer, system_data::SystemDataInterface& system_data,
util::ValidatedClockInterface* validated_clock,
- util::CivilTimeConverterInterface* civil_time_converter,
+ util::CivilTimeConverterInterface& civil_time_converter,
std::weak_ptr<UndatedEventManager> undated_event_manager,
std::vector<uint32_t> experiment_ids, bool enable_replacement_metrics = false,
InternalMetrics* internal_metrics = nullptr);
@@ -208,7 +208,7 @@
const system_data::SystemDataInterface& system_data_;
util::ValidatedClockInterface* validated_clock_;
std::unique_ptr<util::ValidatedClockInterface> local_validated_clock_;
- util::CivilTimeConverterInterface* civil_time_converter_;
+ util::CivilTimeConverterInterface& civil_time_converter_;
std::weak_ptr<UndatedEventManager> undated_event_manager_;
std::vector<uint32_t> experiment_ids_;
diff --git a/src/logger/logger_test.cc b/src/logger/logger_test.cc
index db2294f..efcc6e6 100644
--- a/src/logger/logger_test.cc
+++ b/src/logger/logger_test.cc
@@ -118,19 +118,19 @@
cfg, fs(), *encoder_, *observation_writer_, *metadata_builder_);
local_aggregation_ = std::make_unique<LocalAggregation>(
cfg, project_context_factory_.get(), system_data_, *metadata_builder_, fs(),
- *observation_writer_, civil_time_converter_.get());
+ *observation_writer_, *civil_time_converter_);
internal_logger_ = std::make_unique<testing::FakeLogger>();
internal_metrics_ = InternalMetrics::NewWithLogger(internal_logger_.get(), nullptr);
undated_event_manager_ = std::make_shared<UndatedEventManager>(
*encoder_, event_aggregator_mgr_->GetEventAggregator(), *local_aggregation_,
- *observation_writer_, system_data_, civil_time_converter_.get());
+ *observation_writer_, system_data_, *civil_time_converter_);
std::vector<uint32_t> fake_experiment_ids = {123456789, 987654321};
logger_ = std::make_unique<Logger>(
GetTestProject(registry_base64), *encoder_, event_aggregator_mgr_->GetEventAggregator(),
*local_aggregation_, *observation_writer_, system_data_, validated_clock_.get(),
- civil_time_converter_.get(), undated_event_manager_, fake_experiment_ids, true,
+ *civil_time_converter_, undated_event_manager_, fake_experiment_ids, true,
internal_metrics_.get());
}
@@ -1167,7 +1167,7 @@
cfg, fs(), *encoder_, *observation_writer_, *metadata_builder_);
local_aggregation_ = std::make_unique<LocalAggregation>(
cfg, project_context_factory_.get(), system_data_, *metadata_builder_, fs(),
- *observation_writer_, civil_time_converter_.get());
+ *observation_writer_, *civil_time_converter_);
internal_logger_ = std::make_unique<testing::FakeLogger>();
@@ -1175,7 +1175,7 @@
logger_ = std::make_unique<Logger>(
GetTestProject(testing::all_report_types::kCobaltRegistryBase64), *encoder_,
event_aggregator_mgr_->GetEventAggregator(), *local_aggregation_, *observation_writer_,
- system_data_, civil_time_converter_.get(), empty_experiment_ids, internal_logger_.get());
+ system_data_, *civil_time_converter_, empty_experiment_ids, internal_logger_.get());
}
void TearDown() override {
diff --git a/src/logger/undated_event_manager.cc b/src/logger/undated_event_manager.cc
index 1edeb46..8e9500b 100644
--- a/src/logger/undated_event_manager.cc
+++ b/src/logger/undated_event_manager.cc
@@ -23,7 +23,7 @@
local_aggregation::LocalAggregation& local_aggregation,
ObservationWriter& observation_writer,
system_data::SystemDataInterface& system_data,
- util::CivilTimeConverterInterface* civil_time_converter,
+ util::CivilTimeConverterInterface& civil_time_converter,
int32_t max_saved_events)
: encoder_(encoder),
event_aggregator_(event_aggregator),
@@ -32,7 +32,6 @@
system_data_(system_data),
civil_time_converter_(civil_time_converter),
max_saved_events_(max_saved_events) {
- CHECK(civil_time_converter_);
steady_clock_ = std::make_unique<util::SteadyClock>();
}
diff --git a/src/logger/undated_event_manager.h b/src/logger/undated_event_manager.h
index f365c3d..20b4b41 100644
--- a/src/logger/undated_event_manager.h
+++ b/src/logger/undated_event_manager.h
@@ -55,7 +55,7 @@
local_aggregation::LocalAggregation& local_aggregation,
ObservationWriter& observation_writer,
system_data::SystemDataInterface& system_data,
- util::CivilTimeConverterInterface* civil_time_converter,
+ util::CivilTimeConverterInterface& civil_time_converter,
int32_t max_saved_events = kDefaultMaxSavedEvents);
// Saves the fact that an event has occurred.
@@ -89,7 +89,7 @@
local_aggregation::LocalAggregation& local_aggregation_;
const ObservationWriter& observation_writer_;
const system_data::SystemDataInterface& system_data_;
- util::CivilTimeConverterInterface* civil_time_converter_;
+ util::CivilTimeConverterInterface& civil_time_converter_;
size_t max_saved_events_;
diff --git a/src/logger/undated_event_manager_test.cc b/src/logger/undated_event_manager_test.cc
index fbd6994..5c0550e 100644
--- a/src/logger/undated_event_manager_test.cc
+++ b/src/logger/undated_event_manager_test.cc
@@ -97,14 +97,14 @@
cfg, fs(), *encoder_, *observation_writer_, *metadata_builder_);
local_aggregation_ = std::make_unique<LocalAggregation>(
cfg, project_context_factory_.get(), system_data_, *metadata_builder_, fs(),
- *observation_writer_, civil_time_converter_.get());
+ *observation_writer_, *civil_time_converter_);
internal_logger_ = std::make_unique<testing::FakeLogger>();
internal_metrics_ = logger::InternalMetrics::NewWithLogger(internal_logger_.get(), nullptr);
undated_event_manager_ = std::make_unique<UndatedEventManager>(
*encoder_, event_aggregator_mgr_->GetEventAggregator(), *local_aggregation_,
- *observation_writer_, system_data_, civil_time_converter_.get(), kMaxSavedEvents);
+ *observation_writer_, system_data_, *civil_time_converter_, kMaxSavedEvents);
undated_event_manager_->SetSteadyClock(mock_steady_clock_);
}
diff --git a/src/public/BUILD.gn b/src/public/BUILD.gn
index 5ca2337..1bb2501 100644
--- a/src/public/BUILD.gn
+++ b/src/public/BUILD.gn
@@ -80,7 +80,10 @@
"$cobalt_root/src/system_data:configuration_data",
]
- public_deps = [ "$cobalt_root/src/uploader:shipping_manager" ]
+ public_deps = [
+ "$cobalt_root/src/lib/util:not_null",
+ "$cobalt_root/src/uploader:shipping_manager",
+ ]
}
source_set("cobalt_config_test") {
diff --git a/src/public/cobalt_service.cc b/src/public/cobalt_service.cc
index f3b5cfb..5bfa998 100644
--- a/src/public/cobalt_service.cc
+++ b/src/public/cobalt_service.cc
@@ -9,6 +9,7 @@
#include "src/lib/util/clock.h"
#include "src/lib/util/datetime_util.h"
#include "src/lib/util/encrypted_message_util.h"
+#include "src/lib/util/not_null.h"
#include "src/local_aggregation_1_1/aggregation_procedures/aggregation_procedure.h"
#include "src/logger/internal_metrics_config.cb.h"
#include "src/logger/project_context.h"
@@ -110,14 +111,14 @@
metadata_builder_(system_data_, cfg.validated_clock, cfg.system_data_cache_path, *fs_),
logger_encoder_(cfg.client_secret, metadata_builder_),
observation_writer_(*observation_store_, shipping_manager_.get(), encrypt_to_analyzer_.get()),
- civil_time_converter_(cfg.civil_time_converter ? std::move(cfg.civil_time_converter)
- : std::make_unique<util::UtcTimeConverter>()),
+ civil_time_converter_(util::WrapNotNullUniquePtrOrDefault<util::UtcTimeConverter>(
+ std::move(cfg.civil_time_converter))),
event_aggregator_manager_(cfg, *fs_, logger_encoder_, observation_writer_, metadata_builder_),
local_aggregation_(cfg, global_project_context_factory_.get(), system_data_,
- metadata_builder_, *fs_, observation_writer_, civil_time_converter()),
+ metadata_builder_, *fs_, observation_writer_, *civil_time_converter_),
undated_event_manager_(new logger::UndatedEventManager(
logger_encoder_, event_aggregator_manager_.GetEventAggregator(), local_aggregation_,
- observation_writer_, system_data_, civil_time_converter())),
+ observation_writer_, system_data_, *civil_time_converter_)),
validated_clock_(cfg.validated_clock),
internal_logger_(
(global_project_context_factory_)
@@ -180,12 +181,12 @@
return std::make_unique<logger::Logger>(
std::move(project_context), logger_encoder_, event_aggregator_manager_.GetEventAggregator(),
local_aggregation_, observation_writer_, system_data_, validated_clock_,
- civil_time_converter(), undated_event_manager_, std::move(experiment_ids),
+ *civil_time_converter_, undated_event_manager_, std::move(experiment_ids),
enable_replacement_metrics_, metrics);
}
return std::make_unique<logger::Logger>(
std::move(project_context), logger_encoder_, event_aggregator_manager_.GetEventAggregator(),
- local_aggregation_, observation_writer_, system_data_, civil_time_converter(),
+ local_aggregation_, observation_writer_, system_data_, *civil_time_converter_,
std::move(experiment_ids), enable_replacement_metrics_, metrics);
}
diff --git a/src/public/cobalt_service.h b/src/public/cobalt_service.h
index 30e2063..77b446a 100644
--- a/src/public/cobalt_service.h
+++ b/src/public/cobalt_service.h
@@ -13,6 +13,7 @@
#include "src/lib/util/clock.h"
#include "src/lib/util/consistent_proto_store.h"
#include "src/lib/util/encrypted_message_util.h"
+#include "src/lib/util/not_null.h"
#include "src/local_aggregation/event_aggregator_mgr.h"
#include "src/local_aggregation_1_1/local_aggregation.h"
#include "src/logger/internal_metrics.h"
@@ -138,20 +139,18 @@
bool has_internal_logger() const { return internal_logger_ != nullptr; }
private:
- friend class internal::RealLoggerFactory;
friend class CobaltControllerImpl;
- friend class CobaltServiceTest;
+ friend class CobaltServicePeer;
+ // These three methods are used by TestApp (internal::RealLoggerFactory) and shouldn't be used
+ // elsewhere.
+ friend class internal::RealLoggerFactory;
observation_store::ObservationStore &observation_store() { return *observation_store_; }
-
local_aggregation::EventAggregatorManager *event_aggregator_manager() {
return &event_aggregator_manager_;
}
-
uploader::ShippingManager *shipping_manager() { return shipping_manager_.get(); }
- util::CivilTimeConverterInterface *civil_time_converter() { return civil_time_converter_.get(); }
-
const bool enable_replacement_metrics_;
std::unique_ptr<util::FileSystem> fs_;
@@ -165,7 +164,7 @@
MetadataBuilder metadata_builder_;
logger::Encoder logger_encoder_;
logger::ObservationWriter observation_writer_;
- std::unique_ptr<util::CivilTimeConverterInterface> civil_time_converter_;
+ util::PinnedUniquePtr<util::CivilTimeConverterInterface> civil_time_converter_;
local_aggregation::EventAggregatorManager event_aggregator_manager_;
local_aggregation::LocalAggregation local_aggregation_;
std::shared_ptr<logger::UndatedEventManager> undated_event_manager_;
diff --git a/src/public/cobalt_service_test.cc b/src/public/cobalt_service_test.cc
index 4d51005..59ae9d2 100644
--- a/src/public/cobalt_service_test.cc
+++ b/src/public/cobalt_service_test.cc
@@ -7,11 +7,12 @@
#include <gtest/gtest.h>
#include "src/lib/util/posix_file_system.h"
+#include "src/public/lib/clock_interfaces.h"
+#include "src/uploader/shipping_manager.h"
#include "third_party/abseil-cpp/absl/strings/escaping.h"
namespace cobalt {
-using observation_store::ObservationStore;
using uploader::ClearcutV1ShippingManager;
using uploader::ShippingManager;
@@ -40,45 +41,48 @@
} // namespace
-class CobaltServiceTest : public ::testing::Test {
- protected:
- // Gives access to the private getter method shipping_manager() for the
- // given |cobalt_service|.
- static ShippingManager* GetShippingManager(CobaltService* cobalt_service) {
- return cobalt_service->shipping_manager();
+class CobaltServicePeer {
+ public:
+ static bool HasShippingManager(const CobaltService& service, ShippingManager::Impl impl) {
+ return service.shipping_manager_->impl() == impl;
}
- // Gives access to the private getter method observation_store() for the
- // given |cobalt_service|.
- static ObservationStore& GetObservationStore(CobaltService* cobalt_service) {
- return cobalt_service->observation_store();
+ static bool HasRealInternalMetrics(const CobaltService& service) {
+ if (service.shipping_manager_->impl() == ShippingManager::Impl::CLEARCUTV1) {
+ const auto* clearcut_shipping =
+ static_cast<const ClearcutV1ShippingManager*>(service.shipping_manager_.get());
+ if (!clearcut_shipping->internal_metrics()->IsRealImpl()) {
+ return false;
+ }
+ if (!clearcut_shipping->clearcut_uploader()->internal_metrics()->IsRealImpl()) {
+ return false;
+ }
+ }
+
+ return service.observation_store_->internal_metrics()->IsRealImpl();
}
- // Gives access to the private getter method civil_time_converter() for the
- // given |cobalt_service|.
- static util::CivilTimeConverterInterface* GetCivilTimeConverter(CobaltService* cobalt_service) {
- return cobalt_service->civil_time_converter();
+ static bool HasCivilTimeConverterInterface(const CobaltService& service) {
+ return service.civil_time_converter_.get() != nullptr;
}
};
-TEST_F(CobaltServiceTest, CrashesWhenConstructingWithNoGlobalRegistry) {
+TEST(CobaltService, CrashesWhenConstructingWithNoGlobalRegistry) {
auto cfg = MinConfigForTesting();
cfg.global_registry = nullptr;
ASSERT_DEATH(CobaltService service(std::move(cfg)), "Cannot initialize store");
}
-TEST_F(CobaltServiceTest, DoesNotCreateInternalLoggerWithEmptyGlobalRegistry) {
+TEST(CobaltService, DoesNotCreateInternalLoggerWithEmptyGlobalRegistry) {
auto cfg = MinConfigForTesting();
cfg.global_registry = std::make_unique<CobaltRegistry>();
CobaltService service(std::move(cfg));
EXPECT_FALSE(service.has_internal_logger());
- ShippingManager* shipping_manager = GetShippingManager(&service);
- EXPECT_TRUE(shipping_manager->impl() == ShippingManager::Impl::OTHER);
- ObservationStore& observation_store = GetObservationStore(&service);
- EXPECT_FALSE(observation_store.internal_metrics()->IsRealImpl());
+ EXPECT_TRUE(CobaltServicePeer::HasShippingManager(service, ShippingManager::Impl::OTHER));
+ EXPECT_FALSE(CobaltServicePeer::HasRealInternalMetrics(service));
}
-TEST_F(CobaltServiceTest, CreatesInternalLoggerWithValidRegistry) {
+TEST(CobaltService, CreatesInternalLoggerWithValidRegistry) {
auto cfg = MinConfigForTesting();
cfg.global_registry = std::make_unique<CobaltRegistry>();
std::string registry_bytes;
@@ -86,15 +90,13 @@
ASSERT_TRUE(cfg.global_registry->ParseFromString(registry_bytes));
CobaltService service(std::move(cfg));
EXPECT_TRUE(service.has_internal_logger());
- ShippingManager* shipping_manager = GetShippingManager(&service);
- EXPECT_TRUE(shipping_manager->impl() == ShippingManager::Impl::OTHER);
- ObservationStore& observation_store = GetObservationStore(&service);
- EXPECT_TRUE(observation_store.internal_metrics()->IsRealImpl());
+ EXPECT_TRUE(CobaltServicePeer::HasShippingManager(service, ShippingManager::Impl::OTHER));
+ EXPECT_TRUE(CobaltServicePeer::HasRealInternalMetrics(service));
}
// Tests that when a real ClearcutV1ShippingManager is used that it
// gets a real internal logger to use.
-TEST_F(CobaltServiceTest, CreatesClearcutV1ShippingManager) {
+TEST(CobaltService, CreatesClearcutV1ShippingManager) {
auto cfg = MinConfigForTesting();
cfg.global_registry = std::make_unique<CobaltRegistry>();
cfg.target_pipeline = MinClearcutTargetPipeline();
@@ -103,21 +105,16 @@
ASSERT_TRUE(cfg.global_registry->ParseFromString(registry_bytes));
CobaltService service(std::move(cfg));
EXPECT_TRUE(service.has_internal_logger());
- ShippingManager* shipping_manager = GetShippingManager(&service);
- ASSERT_TRUE(shipping_manager->impl() == ShippingManager::Impl::CLEARCUTV1);
- const auto* clearcut_shipping = static_cast<const ClearcutV1ShippingManager*>(shipping_manager);
- EXPECT_TRUE(clearcut_shipping->internal_metrics()->IsRealImpl());
- EXPECT_TRUE(clearcut_shipping->clearcut_uploader()->internal_metrics()->IsRealImpl());
- ObservationStore& observation_store = GetObservationStore(&service);
- EXPECT_TRUE(observation_store.internal_metrics()->IsRealImpl());
+ ASSERT_TRUE(CobaltServicePeer::HasShippingManager(service, ShippingManager::Impl::CLEARCUTV1));
+ EXPECT_TRUE(CobaltServicePeer::HasRealInternalMetrics(service));
}
-TEST_F(CobaltServiceTest, CreatesDefaultCivilTimeConverter) {
+TEST(CobaltServiceTest, CreatesDefaultCivilTimeConverter) {
auto cfg = MinConfigForTesting();
- ASSERT_FALSE(cfg.civil_time_converter);
+ EXPECT_EQ(cfg.civil_time_converter, nullptr);
cfg.global_registry = std::make_unique<CobaltRegistry>();
CobaltService service(std::move(cfg));
- EXPECT_TRUE(GetCivilTimeConverter(&service));
+ EXPECT_TRUE(CobaltServicePeer::HasCivilTimeConverterInterface(service));
}
} // namespace cobalt
