Don't report metrics when rolling back.
While reviewing histograms for the new metrics, some of the values for
the UpdateEngine.Attempt.* metrics had bogus values. For example, the
DownloadSource and ConnectionType metrics had values in their overflow
buckets. This is weird as the code carefully tries to ensure that
values outside the respective enum classes are never used.
Here's how this can happen: If we're rolling back, the UpdateAttempter
class calls PayloadState::Rollback() instead of PayloadState::UpdateResumed()
or PayloadState::UpdateRestarted().
Crucially, PayloadState::Rollback() never calls the AttemptStarted()
method so the attempt_*_ member variables are left uninitialized.
Then later on UpdateAttempter::ProcessingDone() calls
PayloadState::UpdateSucceeded() or PayloadState::UpdateFailed() which
ends up calling PayloadState::CollectAndReportAttemptMetrics() and we
report the uninitialized attempt_*_ members.
This CL fixes this problem by making PayloadState::Rollback() call
AttemptStarted() and then keep track of whether it's a rollback or
not. In the affirmative we don't report metrics. There's also a unit
test to verify this.
Additionally, this CL also fixes the oversight that the attempt_*_
members were not initialized in the constructor, per policy.
It would probably be better if rollback was implemented in another way
(so it didn't trigger codepaths like this) but that's not how it was
done. We should probably also think about reporting metrics specific
to rollback.
BUG=chromium:355745
TEST=New unit test + Unit tests pass.
Change-Id: Id2e606f02797714520290c4cbe34a056ccdae053
Reviewed-on: https://chromium-review.googlesource.com/193950
Reviewed-by: David Zeuthen <zeuthen@chromium.org>
Commit-Queue: David Zeuthen <zeuthen@chromium.org>
Tested-by: David Zeuthen <zeuthen@chromium.org>
diff --git a/payload_state_unittest.cc b/payload_state_unittest.cc
index 20d321c..11eb4af 100644
--- a/payload_state_unittest.cc
+++ b/payload_state_unittest.cc
@@ -1080,6 +1080,13 @@
rollback_version));
payload_state.LoadRollbackVersion();
EXPECT_EQ(rollback_version, payload_state.GetRollbackVersion());
+
+ // Check that we don't report any metrics in UpdateSucceeded().
+ EXPECT_CALL(*fake_system_state.mock_metrics_lib(), SendToUMA(_, _, _, _, _))
+ .Times(0);
+ EXPECT_CALL(*fake_system_state.mock_metrics_lib(), SendEnumToUMA(_, _, _))
+ .Times(0);
+ payload_state.UpdateSucceeded();
}
TEST(PayloadStateTest, DurationsAreCorrect) {