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.h b/payload_state.h
index d0211c5..9ad4377 100644
--- a/payload_state.h
+++ b/payload_state.h
@@ -111,6 +111,11 @@
}
private:
+ enum class AttemptType {
+ kUpdate,
+ kRollback,
+ };
+
friend class PayloadStateTest;
FRIEND_TEST(PayloadStateTest, RebootAfterUpdateFailedMetric);
FRIEND_TEST(PayloadStateTest, RebootAfterUpdateSucceed);
@@ -119,8 +124,8 @@
FRIEND_TEST(PayloadStateTest, UpdateSuccessWithWipedPrefs);
// Helper called when an attempt has begun, is called by
- // UpdateResumed() and UpdateRestarted().
- void AttemptStarted();
+ // UpdateResumed(), UpdateRestarted() and Rollback().
+ void AttemptStarted(AttemptType attempt_type);
// Increments the payload attempt number used for metrics.
void IncrementPayloadAttemptNumber();
@@ -506,6 +511,9 @@
// The connection type when the attempt started.
metrics::ConnectionType attempt_connection_type_;
+ // Whether we're currently rolling back.
+ AttemptType attempt_type_;
+
DISALLOW_COPY_AND_ASSIGN(PayloadState);
};