Segregate UMA metrics for production scenarios from test scenarios.
Currently we separate the UMA metrics only by one category: whether the
device is in dev mode or not. In addition, we need to exclude the noise
from these two categories:
1. Most of our testing on MP-signed images which are performed
with autest.
2. All our hwlab tests run in non-dev mode but they use dev-signed images
with dev-firmware keys.
So this CL defines additional bit fields to represent these states and
if any of these three flags are set, the UMA metric is sent to a
DevModeErrorCodes bucket. Thus the NormalErrorCodes bucket will have only
the production errors and thus we can monitor more effectively.
BUG=chromium-os:37613
TEST=Updated unit tests, ran on ZGB for all scenarios.
Change-Id: Id9cce33f09d1cc50cb15e67c731f7548940cbc24
Reviewed-on: https://gerrit.chromium.org/gerrit/41103
Reviewed-by: Chris Sosa <sosa@chromium.org>
Commit-Queue: Jay Srinivasan <jaysri@chromium.org>
Tested-by: Jay Srinivasan <jaysri@chromium.org>
diff --git a/SConstruct b/SConstruct
index 47df390..cbdf9de 100644
--- a/SConstruct
+++ b/SConstruct
@@ -314,6 +314,7 @@
http_fetcher_unittest.cc
metadata_unittest.cc
mock_http_fetcher.cc
+ mock_system_state.cc
omaha_hash_calculator_unittest.cc
omaha_request_action_unittest.cc
omaha_request_params_unittest.cc
diff --git a/action_processor.h b/action_processor.h
index 7c425de..58616b5 100644
--- a/action_processor.h
+++ b/action_processor.h
@@ -92,13 +92,26 @@
// TODO(jaysri): Move out all the bit masks into separate constants
// outside the enum as part of fixing bug 34369.
// Bit flags. Remember to update the mask below for new bits.
- kActionCodeResumedFlag = 1 << 30, // Set if resuming an interruped update.
- kActionCodeBootModeFlag = 1 << 31, // Set if boot mode not normal.
- // Mask to be used to extract the actual code without the higher-order
- // bit flags (for reporting to UMA which requires small contiguous
- // enum values)
- kActualCodeMask = ~(kActionCodeResumedFlag | kActionCodeBootModeFlag)
+ // Set if boot mode not normal.
+ kActionCodeDevModeFlag = 1 << 31,
+
+ // Set if resuming an interruped update.
+ kActionCodeResumedFlag = 1 << 30,
+
+ // Set if using a dev/test image as opposed to an MP-signed image.
+ kActionCodeTestImageFlag = 1 << 29,
+
+ // Set if using devserver or Omaha sandbox (using crosh autest).
+ kActionCodeTestOmahaUrlFlag = 1 << 28,
+
+ // Mask that indicates bit positions that are used to indicate special flags
+ // that are embedded in the error code to provide additional context about
+ // the system in which the error was encountered.
+ kSpecialFlags = (kActionCodeDevModeFlag |
+ kActionCodeResumedFlag |
+ kActionCodeTestImageFlag |
+ kActionCodeTestOmahaUrlFlag)
};
class AbstractAction;
diff --git a/delta_performer.cc b/delta_performer.cc
index 3dbfbc0..3f549cd 100644
--- a/delta_performer.cc
+++ b/delta_performer.cc
@@ -23,6 +23,7 @@
#include "update_engine/extent_writer.h"
#include "update_engine/graph_types.h"
#include "update_engine/payload_signer.h"
+#include "update_engine/payload_state_interface.h"
#include "update_engine/prefs_interface.h"
#include "update_engine/subprocess.h"
#include "update_engine/terminator.h"
@@ -1118,9 +1119,7 @@
}
void DeltaPerformer::SendUmaStat(ActionExitCode code) {
- if (system_state_) {
- utils::SendErrorCodeToUma(system_state_->metrics_lib(), code);
- }
+ utils::SendErrorCodeToUma(system_state_, code);
}
} // namespace chromeos_update_engine
diff --git a/main.cc b/main.cc
index c956278..cd51995 100644
--- a/main.cc
+++ b/main.cc
@@ -21,11 +21,11 @@
#include "update_engine/dbus_constants.h"
#include "update_engine/dbus_interface.h"
#include "update_engine/dbus_service.h"
+#include "update_engine/real_system_state.h"
#include "update_engine/subprocess.h"
#include "update_engine/terminator.h"
#include "update_engine/update_attempter.h"
#include "update_engine/update_check_scheduler.h"
-#include "update_engine/system_state.h"
#include "update_engine/utils.h"
extern "C" {
@@ -171,6 +171,9 @@
// protocol for testing of MP-signed images (chromium-os:25400).
LOG_IF(ERROR, !real_system_state.Initialize(false))
<< "Failed to initialize system state.";
+ chromeos_update_engine::UpdateAttempter *update_attempter =
+ real_system_state.update_attempter();
+ CHECK(update_attempter);
// Sets static members for the certificate checker.
chromeos_update_engine::CertificateChecker::set_system_state(
@@ -179,40 +182,35 @@
chromeos_update_engine::CertificateChecker::set_openssl_wrapper(
&openssl_wrapper);
- // Create the update attempter:
- chromeos_update_engine::ConcreteDbusGlib dbus;
- chromeos_update_engine::UpdateAttempter update_attempter(&real_system_state,
- &dbus);
-
// Create the dbus service object:
dbus_g_object_type_install_info(UPDATE_ENGINE_TYPE_SERVICE,
&dbus_glib_update_engine_service_object_info);
UpdateEngineService* service =
UPDATE_ENGINE_SERVICE(g_object_new(UPDATE_ENGINE_TYPE_SERVICE, NULL));
- service->update_attempter_ = &update_attempter;
- update_attempter.set_dbus_service(service);
+ service->update_attempter_ = update_attempter;
+ update_attempter->set_dbus_service(service);
chromeos_update_engine::SetupDbusService(service);
// Schedule periodic update checks.
- chromeos_update_engine::UpdateCheckScheduler scheduler(&update_attempter,
+ chromeos_update_engine::UpdateCheckScheduler scheduler(update_attempter,
&real_system_state);
scheduler.Run();
// Update boot flags after 45 seconds.
g_timeout_add_seconds(45,
&chromeos_update_engine::UpdateBootFlags,
- &update_attempter);
+ update_attempter);
// Broadcast the update engine status on startup to ensure consistent system
// state on crashes.
- g_idle_add(&chromeos_update_engine::BroadcastStatus, &update_attempter);
+ g_idle_add(&chromeos_update_engine::BroadcastStatus, update_attempter);
// Run the main loop until exit time:
g_main_loop_run(loop);
// Cleanup:
g_main_loop_unref(loop);
- update_attempter.set_dbus_service(NULL);
+ update_attempter->set_dbus_service(NULL);
g_object_unref(G_OBJECT(service));
LOG(INFO) << "Chrome OS Update Engine terminating";
diff --git a/mock_system_state.cc b/mock_system_state.cc
new file mode 100644
index 0000000..13017b9
--- /dev/null
+++ b/mock_system_state.cc
@@ -0,0 +1,28 @@
+// Copyright (c) 2012 The Chromium OS 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 "update_engine/mock_system_state.h"
+#include "update_engine/update_attempter_mock.h"
+
+namespace chromeos_update_engine {
+
+// Mock the SystemStateInterface so that we could lie that
+// OOBE is completed even when there's no such marker file, etc.
+MockSystemState::MockSystemState() : prefs_(&mock_prefs_) {
+ mock_payload_state_.Initialize(&mock_prefs_);
+ mock_gpio_handler_ = new testing::NiceMock<MockGpioHandler>();
+ mock_update_attempter_ = new testing::NiceMock<UpdateAttempterMock>(
+ this, &dbus_);
+}
+
+MockSystemState::~MockSystemState() {
+ delete mock_gpio_handler_;
+}
+
+UpdateAttempter* MockSystemState::update_attempter() {
+ return mock_update_attempter_;
+}
+
+} // namespeace chromeos_update_engine
+
diff --git a/mock_system_state.h b/mock_system_state.h
index 2b17ce4..956088b 100644
--- a/mock_system_state.h
+++ b/mock_system_state.h
@@ -10,6 +10,7 @@
#include <metrics/metrics_library_mock.h>
#include <policy/mock_device_policy.h>
+#include "update_engine/mock_dbus_interface.h"
#include "update_engine/mock_gpio_handler.h"
#include "update_engine/mock_payload_state.h"
#include "update_engine/prefs_mock.h"
@@ -17,17 +18,15 @@
namespace chromeos_update_engine {
+class UpdateAttempterMock;
+
// Mock the SystemStateInterface so that we could lie that
// OOBE is completed even when there's no such marker file, etc.
class MockSystemState : public SystemState {
public:
- inline MockSystemState() : prefs_(&mock_prefs_) {
- mock_payload_state_.Initialize(&mock_prefs_);
- mock_gpio_handler_ = new testing::NiceMock<MockGpioHandler>();
- }
- inline virtual ~MockSystemState() {
- delete mock_gpio_handler_;
- }
+ MockSystemState();
+
+ virtual ~MockSystemState();
MOCK_METHOD0(IsOOBEComplete, bool());
MOCK_METHOD1(set_device_policy, void(const policy::DevicePolicy*));
@@ -53,6 +52,8 @@
return mock_gpio_handler_;
}
+ virtual UpdateAttempter* update_attempter();
+
// MockSystemState-specific public method.
inline void set_connection_manager(ConnectionManager* connection_manager) {
connection_manager_ = connection_manager;
@@ -76,10 +77,13 @@
private:
// These are Mock objects or objects we own.
- MetricsLibraryMock mock_metrics_lib_;
+ testing::NiceMock<MetricsLibraryMock> mock_metrics_lib_;
testing::NiceMock<PrefsMock> mock_prefs_;
testing::NiceMock<MockPayloadState> mock_payload_state_;
testing::NiceMock<MockGpioHandler>* mock_gpio_handler_;
+ testing::NiceMock<UpdateAttempterMock>* mock_update_attempter_;
+
+ MockDbusGlib dbus_;
// These are pointers to objects which caller can override.
PrefsInterface* prefs_;
diff --git a/omaha_hash_calculator.cc b/omaha_hash_calculator.cc
index 33d8a9e..245706d 100644
--- a/omaha_hash_calculator.cc
+++ b/omaha_hash_calculator.cc
@@ -176,7 +176,7 @@
&ctx_) == 1);
// Convert raw_hash_ to base64 encoding and store it in hash_.
- return Base64Encode(&raw_hash_[0], raw_hash_.size(), &hash_);;
+ return Base64Encode(&raw_hash_[0], raw_hash_.size(), &hash_);
}
bool OmahaHashCalculator::RawHashOfBytes(const char* data,
diff --git a/omaha_request_action.cc b/omaha_request_action.cc
index 679d326..edfea53 100644
--- a/omaha_request_action.cc
+++ b/omaha_request_action.cc
@@ -20,6 +20,7 @@
#include "update_engine/action_pipe.h"
#include "update_engine/omaha_request_params.h"
+#include "update_engine/payload_state_interface.h"
#include "update_engine/prefs_interface.h"
#include "update_engine/utils.h"
diff --git a/omaha_request_params.cc b/omaha_request_params.cc
index e240c86..d857297 100644
--- a/omaha_request_params.cc
+++ b/omaha_request_params.cc
@@ -30,7 +30,7 @@
"{87efface-864d-49a5-9bb3-4b050a7c227a}");
const char* const OmahaRequestParams::kOsPlatform("Chrome OS");
const char* const OmahaRequestParams::kOsVersion("Indy");
-const char* const OmahaRequestParams::kUpdateUrl(
+const char* const kProductionOmahaUrl(
"https://tools.google.com/service/update2");
const char OmahaRequestParams::kUpdateTrackKey[] = "CHROMEOS_RELEASE_TRACK";
@@ -95,7 +95,7 @@
update_url = in_update_url.empty() ?
GetLsbValue("CHROMEOS_AUSERVER",
- OmahaRequestParams::kUpdateUrl,
+ kProductionOmahaUrl,
NULL,
stateful_override) :
in_update_url;
diff --git a/omaha_request_params.h b/omaha_request_params.h
index 7435468..ea8bab0 100644
--- a/omaha_request_params.h
+++ b/omaha_request_params.h
@@ -16,6 +16,9 @@
namespace chromeos_update_engine {
+// The default "official" Omaha update URL.
+extern const char* const kProductionOmahaUrl;
+
// This struct encapsulates the data Omaha gets for the request, along with
// essential state needed for the processing of the request/response.
// The strings in this struct should not be XML escaped.
diff --git a/omaha_request_params_unittest.cc b/omaha_request_params_unittest.cc
index ecaa682..8908311 100644
--- a/omaha_request_params_unittest.cc
+++ b/omaha_request_params_unittest.cc
@@ -216,7 +216,7 @@
EXPECT_EQ("en-US", out.app_lang);
EXPECT_TRUE(out.delta_okay);
EXPECT_EQ("footrack", out.app_track);
- EXPECT_EQ(OmahaRequestParams::kUpdateUrl, out.update_url);
+ EXPECT_EQ(kProductionOmahaUrl, out.update_url);
}
TEST_F(OmahaRequestDeviceParamsTest, NoDeltasTest) {
diff --git a/omaha_response_handler_action.cc b/omaha_response_handler_action.cc
index ab083aa..24b8dcb 100644
--- a/omaha_response_handler_action.cc
+++ b/omaha_response_handler_action.cc
@@ -10,6 +10,7 @@
#include "base/string_util.h"
#include "update_engine/delta_performer.h"
+#include "update_engine/payload_state_interface.h"
#include "update_engine/prefs_interface.h"
#include "update_engine/utils.h"
diff --git a/payload_state.cc b/payload_state.cc
index 26a3a3f..9c4a7e4 100644
--- a/payload_state.cc
+++ b/payload_state.cc
@@ -94,7 +94,8 @@
void PayloadState::UpdateFailed(ActionExitCode error) {
ActionExitCode base_error = utils::GetBaseErrorCode(error);
- LOG(INFO) << "Updating payload state for error code: " << base_error;
+ LOG(INFO) << "Updating payload state for error code: " << base_error
+ << " (" << utils::CodeToString(base_error) << ")";
if (GetNumUrls() == 0) {
// This means we got this error even before we got a valid Omaha response.
@@ -177,9 +178,11 @@
case kActionCodeSetBootableFlagError: // unused
case kActionCodeUmaReportedMax: // not an error code
case kActionCodeOmahaRequestHTTPResponseBase: // aggregated already
+ case kActionCodeDevModeFlag: // not an error code
case kActionCodeResumedFlag: // not an error code
- case kActionCodeBootModeFlag: // not an error code
- case kActualCodeMask: // not an error code
+ case kActionCodeTestImageFlag: // not an error code
+ case kActionCodeTestOmahaUrlFlag: // not an error code
+ case kSpecialFlags: // not an error code
// These shouldn't happen. Enumerating these explicitly here so that we
// can let the compiler warn about new error codes that are added to
// action_processor.h but not added here.
diff --git a/real_system_state.h b/real_system_state.h
new file mode 100644
index 0000000..871a942
--- /dev/null
+++ b/real_system_state.h
@@ -0,0 +1,99 @@
+// Copyright (c) 2013 The Chromium OS 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 CHROMEOS_PLATFORM_UPDATE_ENGINE_REAL_SYSTEM_STATE_H_
+#define CHROMEOS_PLATFORM_UPDATE_ENGINE_REAL_SYSTEM_STATE_H_
+
+#include <update_engine/system_state.h>
+
+#include <update_engine/connection_manager.h>
+#include <update_engine/gpio_handler.h>
+#include <update_engine/payload_state.h>
+#include <update_engine/prefs.h>
+#include <update_engine/update_attempter.h>
+
+namespace chromeos_update_engine {
+
+// A real implementation of the SystemStateInterface which is
+// used by the actual product code.
+class RealSystemState : public SystemState {
+public:
+ // Constructors and destructors.
+ RealSystemState();
+ virtual ~RealSystemState() {}
+
+ virtual bool IsOOBEComplete();
+
+ virtual inline void set_device_policy(
+ const policy::DevicePolicy* device_policy) {
+ device_policy_ = device_policy;
+ }
+
+ virtual inline const policy::DevicePolicy* device_policy() const {
+ return device_policy_;
+ }
+
+ virtual inline ConnectionManager* connection_manager() {
+ return &connection_manager_;
+ }
+
+ virtual inline MetricsLibraryInterface* metrics_lib() {
+ return &metrics_lib_;
+ }
+
+ virtual inline PrefsInterface* prefs() {
+ return &prefs_;
+ }
+
+ virtual inline PayloadStateInterface* payload_state() {
+ return &payload_state_;
+ }
+
+ virtual inline GpioHandler* gpio_handler() const {
+ return gpio_handler_.get();
+ }
+
+ virtual inline UpdateAttempter* update_attempter() {
+ return update_attempter_.get();
+ }
+
+ // Initializes this concrete object. Other methods should be invoked only
+ // if the object has been initialized successfully.
+ bool Initialize(bool enable_gpio);
+
+private:
+ // The latest device policy object from the policy provider.
+ const policy::DevicePolicy* device_policy_;
+
+ // The connection manager object that makes download
+ // decisions depending on the current type of connection.
+ ConnectionManager connection_manager_;
+
+ // The Metrics Library interface for reporting UMA stats.
+ MetricsLibrary metrics_lib_;
+
+ // Interface for persisted store.
+ Prefs prefs_;
+
+ // All state pertaining to payload state such as
+ // response, URL, backoff states.
+ PayloadState payload_state_;
+
+ // Pointer to a GPIO handler and other needed modules (note that the order of
+ // declaration significant for destruction, as the latter depends on the
+ // former).
+ scoped_ptr<UdevInterface> udev_iface_;
+ scoped_ptr<FileDescriptor> file_descriptor_;
+ scoped_ptr<GpioHandler> gpio_handler_;
+
+ // The dbus object used to initialize the update attempter.
+ ConcreteDbusGlib dbus_;
+
+ // Pointer to the update attempter object.
+ scoped_ptr<UpdateAttempter> update_attempter_;
+};
+
+} // namespace chromeos_update_engine
+
+#endif // CHROMEOS_PLATFORM_UPDATE_ENGINE_REAL_SYSTEM_STATE_H_
diff --git a/system_state.cc b/system_state.cc
index fc3ba57..42737bb 100644
--- a/system_state.cc
+++ b/system_state.cc
@@ -4,7 +4,7 @@
#include <base/file_util.h>
-#include "update_engine/system_state.h"
+#include "update_engine/real_system_state.h"
namespace chromeos_update_engine {
@@ -42,6 +42,9 @@
gpio_handler_.reset(new NoopGpioHandler(false));
}
+ // Create the update attempter.
+ update_attempter_.reset(new UpdateAttempter(this, &dbus_));
+
// All is well. Initialization successful.
return true;
}
diff --git a/system_state.h b/system_state.h
index 772205c..20e11a7 100644
--- a/system_state.h
+++ b/system_state.h
@@ -9,13 +9,17 @@
#include <policy/device_policy.h>
#include <policy/libpolicy.h>
-#include <update_engine/connection_manager.h>
-#include <update_engine/gpio_handler.h>
-#include <update_engine/payload_state.h>
-#include <update_engine/prefs.h>
-
namespace chromeos_update_engine {
+// SystemState is the root class within the update engine. So we should avoid
+// any circular references in header file inclusion. Hence forward-declaring
+// the required classes.
+class ConnectionManager;
+class PrefsInterface;
+class PayloadStateInterface;
+class GpioHandler;
+class UpdateAttempter;
+
// An interface to global system context, including platform resources,
// the current state of the system, high-level objects whose lifetime is same
// as main, system interfaces, etc.
@@ -50,78 +54,11 @@
// Returns a pointer to the GPIO handler.
virtual GpioHandler* gpio_handler() const = 0;
-};
-// A real implementation of the SystemStateInterface which is
-// used by the actual product code.
-class RealSystemState : public SystemState {
-public:
- // Constructors and destructors.
- RealSystemState();
- virtual ~RealSystemState() {}
-
- virtual bool IsOOBEComplete();
-
- virtual inline void set_device_policy(
- const policy::DevicePolicy* device_policy) {
- device_policy_ = device_policy;
- }
-
- virtual inline const policy::DevicePolicy* device_policy() const {
- return device_policy_;
- }
-
- virtual inline ConnectionManager* connection_manager() {
- return &connection_manager_;
- }
-
- virtual inline MetricsLibraryInterface* metrics_lib() {
- return &metrics_lib_;
- }
-
- virtual inline PrefsInterface* prefs() {
- return &prefs_;
- }
-
- virtual inline PayloadStateInterface* payload_state() {
- return &payload_state_;
- }
-
- // Returns a pointer to the GPIO handler.
- virtual inline GpioHandler* gpio_handler() const {
- return gpio_handler_.get();
- }
-
- // Initializs this concrete object. Other methods should be invoked only
- // if the object has been initialized successfully.
- bool Initialize(bool enable_gpio);
-
-private:
- // The latest device policy object from the policy provider.
- const policy::DevicePolicy* device_policy_;
-
- // The connection manager object that makes download
- // decisions depending on the current type of connection.
- ConnectionManager connection_manager_;
-
- // The Metrics Library interface for reporting UMA stats.
- MetricsLibrary metrics_lib_;
-
- // Interface for persisted store.
- Prefs prefs_;
-
- // All state pertaining to payload state such as
- // response, URL, backoff states.
- PayloadState payload_state_;
-
- // Pointer to a GPIO handler and other needed modules (note that the order of
- // declaration significant for destruction, as the latter depends on the
- // former).
- scoped_ptr<UdevInterface> udev_iface_;
- scoped_ptr<FileDescriptor> file_descriptor_;
- scoped_ptr<GpioHandler> gpio_handler_;
+ // Returns a pointer to the update attempter object.
+ virtual UpdateAttempter* update_attempter() = 0;
};
} // namespace chromeos_update_engine
-#endif // CHROMEOS_PLATFORM_UPDATE_ENGINE_UTILS_H_
+#endif // CHROMEOS_PLATFORM_UPDATE_ENGINE_SYSTEM_STATE_H_
diff --git a/update_attempter.cc b/update_attempter.cc
index 3aa9749..569b626 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -25,11 +25,13 @@
#include "update_engine/dbus_service.h"
#include "update_engine/download_action.h"
#include "update_engine/filesystem_copier_action.h"
+#include "update_engine/gpio_handler.h"
#include "update_engine/libcurl_http_fetcher.h"
#include "update_engine/multi_range_http_fetcher.h"
#include "update_engine/omaha_request_action.h"
#include "update_engine/omaha_request_params.h"
#include "update_engine/omaha_response_handler_action.h"
+#include "update_engine/payload_state_interface.h"
#include "update_engine/postinstall_runner_action.h"
#include "update_engine/prefs_interface.h"
#include "update_engine/subprocess.h"
@@ -602,7 +604,7 @@
// Also report the success code so that the percentiles can be
// interpreted properly for the remaining error codes in UMA.
- utils::SendErrorCodeToUma(system_state_->metrics_lib(), code);
+ utils::SendErrorCodeToUma(system_state_, code);
return;
}
@@ -811,6 +813,25 @@
new_payload_size_);
}
+uint32_t UpdateAttempter::GetErrorCodeFlags() {
+ uint32_t flags = 0;
+
+ if (!utils::IsNormalBootMode())
+ flags |= kActionCodeDevModeFlag;
+
+ if (response_handler_action_.get() &&
+ response_handler_action_->install_plan().is_resume)
+ flags |= kActionCodeResumedFlag;
+
+ if (!utils::IsOfficialBuild())
+ flags |= kActionCodeTestImageFlag;
+
+ if (omaha_request_params_.update_url != kProductionOmahaUrl)
+ flags |= kActionCodeTestOmahaUrlFlag;
+
+ return flags;
+}
+
void UpdateAttempter::SetStatusAndNotify(UpdateStatus status,
UpdateNotice notice) {
status_ = status;
@@ -847,6 +868,7 @@
switch (code) {
case kActionCodeOmahaUpdateIgnoredPerPolicy:
case kActionCodeOmahaUpdateDeferredPerPolicy:
+ case kActionCodeOmahaUpdateDeferredForBackoff:
event_result = OmahaEvent::kResultUpdateDeferred;
break;
default:
@@ -857,15 +879,8 @@
code = GetErrorCodeForAction(action, code);
fake_update_success_ = code == kActionCodePostinstallBootedFromFirmwareB;
- // Apply the bit modifiers to the error code.
- if (!utils::IsNormalBootMode()) {
- code = static_cast<ActionExitCode>(code | kActionCodeBootModeFlag);
- }
- if (response_handler_action_.get() &&
- response_handler_action_->install_plan().is_resume) {
- code = static_cast<ActionExitCode>(code | kActionCodeResumedFlag);
- }
-
+ // Compute the final error code with all the bit flags to be sent to Omaha.
+ code = static_cast<ActionExitCode>(code | GetErrorCodeFlags());
error_event_.reset(new OmahaEvent(OmahaEvent::kTypeUpdateComplete,
event_result,
code));
@@ -878,9 +893,11 @@
LOG(ERROR) << "Update failed.";
system_state_->payload_state()->UpdateFailed(error_event_->error_code);
+ // Send it to Uma.
LOG(INFO) << "Reporting the error event";
- utils::SendErrorCodeToUma(system_state_->metrics_lib(),
- error_event_->error_code);
+ utils::SendErrorCodeToUma(system_state_, error_event_->error_code);
+
+ // Send it to Omaha.
shared_ptr<OmahaRequestAction> error_event_action(
new OmahaRequestAction(system_state_,
&omaha_request_params_,
diff --git a/update_attempter.h b/update_attempter.h
index ec0567b..c42715f 100644
--- a/update_attempter.h
+++ b/update_attempter.h
@@ -154,6 +154,10 @@
// Broadcasts the current status over D-Bus.
void BroadcastStatus();
+ // Returns the special flags to be added to ActionExitCode values based on the
+ // parameters used in the current update attempt.
+ uint32_t GetErrorCodeFlags();
+
private:
// Update server URL for automated lab test.
static const char* const kTestUpdateUrl;
diff --git a/update_attempter_mock.h b/update_attempter_mock.h
index 8b47c35..782277a 100644
--- a/update_attempter_mock.h
+++ b/update_attempter_mock.h
@@ -13,6 +13,8 @@
namespace chromeos_update_engine {
+class MockSystemState;
+
class UpdateAttempterMock : public UpdateAttempter {
public:
explicit UpdateAttempterMock(MockSystemState* mock_system_state,
diff --git a/update_attempter_unittest.cc b/update_attempter_unittest.cc
index 61d9010..18bcb67 100644
--- a/update_attempter_unittest.cc
+++ b/update_attempter_unittest.cc
@@ -64,7 +64,7 @@
EXPECT_EQ(0, attempter_.last_checked_time_);
EXPECT_EQ("0.0.0.0", attempter_.new_version_);
EXPECT_EQ(0, attempter_.new_payload_size_);
- processor_ = new ActionProcessorMock();
+ processor_ = new NiceMock<ActionProcessorMock>();
attempter_.processor_.reset(processor_); // Transfers ownership.
prefs_ = mock_system_state_.mock_prefs();
}
@@ -102,12 +102,12 @@
static gboolean StaticNoScatteringDoneDuringManualUpdateTestStart(
gpointer data);
- MockSystemState mock_system_state_;
- MockDbusGlib dbus_;
+ NiceMock<MockSystemState> mock_system_state_;
+ NiceMock<MockDbusGlib> dbus_;
UpdateAttempterUnderTest attempter_;
- ActionProcessorMock* processor_;
+ NiceMock<ActionProcessorMock>* processor_;
NiceMock<PrefsMock>* prefs_; // shortcut to mock_system_state_->mock_prefs()
- MockConnectionManager mock_connection_manager;
+ NiceMock<MockConnectionManager> mock_connection_manager;
GMainLoop* loop_;
};
@@ -433,7 +433,8 @@
ASSERT_TRUE(attempter_.error_event_.get() != NULL);
EXPECT_EQ(OmahaEvent::kTypeUpdateComplete, attempter_.error_event_->type);
EXPECT_EQ(OmahaEvent::kResultError, attempter_.error_event_->result);
- EXPECT_EQ(kCode, attempter_.error_event_->error_code);
+ EXPECT_EQ(kCode | kActionCodeTestOmahaUrlFlag,
+ attempter_.error_event_->error_code);
}
TEST_F(UpdateAttempterTest, CreatePendingErrorEventResumedTest) {
@@ -447,7 +448,7 @@
ASSERT_TRUE(attempter_.error_event_.get() != NULL);
EXPECT_EQ(OmahaEvent::kTypeUpdateComplete, attempter_.error_event_->type);
EXPECT_EQ(OmahaEvent::kResultError, attempter_.error_event_->result);
- EXPECT_EQ(kCode | kActionCodeResumedFlag,
+ EXPECT_EQ(kCode | kActionCodeResumedFlag | kActionCodeTestOmahaUrlFlag,
attempter_.error_event_->error_code);
}
diff --git a/update_check_scheduler.cc b/update_check_scheduler.cc
index 85e734f..227816d 100644
--- a/update_check_scheduler.cc
+++ b/update_check_scheduler.cc
@@ -6,6 +6,7 @@
#include "update_engine/certificate_checker.h"
#include "update_engine/http_common.h"
+#include "update_engine/gpio_handler.h"
#include "update_engine/system_state.h"
#include "update_engine/utils.h"
diff --git a/utils.cc b/utils.cc
index e5d7290..fee3439 100644
--- a/utils.cc
+++ b/utils.cc
@@ -34,6 +34,8 @@
#include "update_engine/file_writer.h"
#include "update_engine/omaha_request_params.h"
#include "update_engine/subprocess.h"
+#include "update_engine/system_state.h"
+#include "update_engine/update_attempter.h"
using base::Time;
using base::TimeDelta;
@@ -715,8 +717,7 @@
// Ignore the higher order bits in the code by applying the mask as
// we want the enumerations to be in the small contiguous range
// with values less than kActionCodeUmaReportedMax.
- ActionExitCode base_code = static_cast<ActionExitCode>(
- code & kActualCodeMask);
+ ActionExitCode base_code = static_cast<ActionExitCode>(code & ~kSpecialFlags);
// Make additional adjustments required for UMA and error classification.
// TODO(jaysri): Move this logic to UeErrorCode.cc when we fix
@@ -724,26 +725,168 @@
if (base_code >= kActionCodeOmahaRequestHTTPResponseBase) {
// Since we want to keep the enums to a small value, aggregate all HTTP
// errors into this one bucket for UMA and error classification purposes.
+ LOG(INFO) << "Converting error code " << base_code
+ << " to kActionCodeOmahaErrorInHTTPResponse";
base_code = kActionCodeOmahaErrorInHTTPResponse;
}
return base_code;
}
+// Returns a printable version of the various flags denoted in the higher order
+// bits of the given code. Returns an empty string if none of those bits are
+// set.
+string GetFlagNames(uint32_t code) {
+ uint32_t flags = code & kSpecialFlags;
+ string flag_names;
+ string separator = "";
+ for(size_t i = 0; i < sizeof(flags) * 8; i++) {
+ uint32_t flag = flags & (1 << i);
+ if (flag) {
+ flag_names += separator + CodeToString(static_cast<ActionExitCode>(flag));
+ separator = ", ";
+ }
+ }
-
-void SendErrorCodeToUma(MetricsLibraryInterface* metrics_lib,
- ActionExitCode code) {
- string metric = utils::IsNormalBootMode() ? "Installer.NormalErrorCodes" :
- "Installer.DevModeErrorCodes";
-
- ActionExitCode reported_code = GetBaseErrorCode(code);
-
- LOG(INFO) << "Sending error code " << reported_code
- << " to UMA metric: " << metric;
- metrics_lib->SendEnumToUMA(metric, reported_code, kActionCodeUmaReportedMax);
+ return flag_names;
}
+void SendErrorCodeToUma(SystemState* system_state, ActionExitCode code) {
+ if (!system_state)
+ return;
+
+ ActionExitCode uma_error_code = GetBaseErrorCode(code);
+
+ // If the code doesn't have flags computed already, compute them now based on
+ // the state of the current update attempt.
+ uint32_t flags = code & kSpecialFlags;
+ if (!flags)
+ flags = system_state->update_attempter()->GetErrorCodeFlags();
+
+ // Determine the UMA bucket depending on the flags. But, ignore the resumed
+ // flag, as it's perfectly normal for production devices to resume their
+ // downloads and so we want to record those cases also in NormalErrorCodes
+ // bucket.
+ string metric = (flags & ~kActionCodeResumedFlag) ?
+ "Installer.DevModeErrorCodes" : "Installer.NormalErrorCodes";
+
+ LOG(INFO) << "Sending error code " << uma_error_code
+ << " (" << CodeToString(uma_error_code) << ")"
+ << " to UMA metric: " << metric
+ << ". Flags = " << (flags ? GetFlagNames(flags) : "None");
+
+ system_state->metrics_lib()->SendEnumToUMA(metric,
+ uma_error_code,
+ kActionCodeUmaReportedMax);
+}
+
+string CodeToString(ActionExitCode code) {
+ // If the given code has both parts (i.e. the error code part and the flags
+ // part) then strip off the flags part since the switch statement below
+ // has case statements only for the base error code or a single flag but
+ // doesn't support any combinations of those.
+ if ((code & kSpecialFlags) && (code & ~kSpecialFlags))
+ code = static_cast<ActionExitCode>(code & ~kSpecialFlags);
+ switch (code) {
+ case kActionCodeSuccess: return "kActionCodeSuccess";
+ case kActionCodeError: return "kActionCodeError";
+ case kActionCodeOmahaRequestError: return "kActionCodeOmahaRequestError";
+ case kActionCodeOmahaResponseHandlerError:
+ return "kActionCodeOmahaResponseHandlerError";
+ case kActionCodeFilesystemCopierError:
+ return "kActionCodeFilesystemCopierError";
+ case kActionCodePostinstallRunnerError:
+ return "kActionCodePostinstallRunnerError";
+ case kActionCodeSetBootableFlagError:
+ return "kActionCodeSetBootableFlagError";
+ case kActionCodeInstallDeviceOpenError:
+ return "kActionCodeInstallDeviceOpenError";
+ case kActionCodeKernelDeviceOpenError:
+ return "kActionCodeKernelDeviceOpenError";
+ case kActionCodeDownloadTransferError:
+ return "kActionCodeDownloadTransferError";
+ case kActionCodePayloadHashMismatchError:
+ return "kActionCodePayloadHashMismatchError";
+ case kActionCodePayloadSizeMismatchError:
+ return "kActionCodePayloadSizeMismatchError";
+ case kActionCodeDownloadPayloadVerificationError:
+ return "kActionCodeDownloadPayloadVerificationError";
+ case kActionCodeDownloadNewPartitionInfoError:
+ return "kActionCodeDownloadNewPartitionInfoError";
+ case kActionCodeDownloadWriteError:
+ return "kActionCodeDownloadWriteError";
+ case kActionCodeNewRootfsVerificationError:
+ return "kActionCodeNewRootfsVerificationError";
+ case kActionCodeNewKernelVerificationError:
+ return "kActionCodeNewKernelVerificationError";
+ case kActionCodeSignedDeltaPayloadExpectedError:
+ return "kActionCodeSignedDeltaPayloadExpectedError";
+ case kActionCodeDownloadPayloadPubKeyVerificationError:
+ return "kActionCodeDownloadPayloadPubKeyVerificationError";
+ case kActionCodePostinstallBootedFromFirmwareB:
+ return "kActionCodePostinstallBootedFromFirmwareB";
+ case kActionCodeDownloadStateInitializationError:
+ return "kActionCodeDownloadStateInitializationError";
+ case kActionCodeDownloadInvalidMetadataMagicString:
+ return "kActionCodeDownloadInvalidMetadataMagicString";
+ case kActionCodeDownloadSignatureMissingInManifest:
+ return "kActionCodeDownloadSignatureMissingInManifest";
+ case kActionCodeDownloadManifestParseError:
+ return "kActionCodeDownloadManifestParseError";
+ case kActionCodeDownloadMetadataSignatureError:
+ return "kActionCodeDownloadMetadataSignatureError";
+ case kActionCodeDownloadMetadataSignatureVerificationError:
+ return "kActionCodeDownloadMetadataSignatureVerificationError";
+ case kActionCodeDownloadMetadataSignatureMismatch:
+ return "kActionCodeDownloadMetadataSignatureMismatch";
+ case kActionCodeDownloadOperationHashVerificationError:
+ return "kActionCodeDownloadOperationHashVerificationError";
+ case kActionCodeDownloadOperationExecutionError:
+ return "kActionCodeDownloadOperationExecutionError";
+ case kActionCodeDownloadOperationHashMismatch:
+ return "kActionCodeDownloadOperationHashMismatch";
+ case kActionCodeOmahaRequestEmptyResponseError:
+ return "kActionCodeOmahaRequestEmptyResponseError";
+ case kActionCodeOmahaRequestXMLParseError:
+ return "kActionCodeOmahaRequestXMLParseError";
+ case kActionCodeDownloadInvalidMetadataSize:
+ return "kActionCodeDownloadInvalidMetadataSize";
+ case kActionCodeDownloadInvalidMetadataSignature:
+ return "kActionCodeDownloadInvalidMetadataSignature";
+ case kActionCodeOmahaResponseInvalid:
+ return "kActionCodeOmahaResponseInvalid";
+ case kActionCodeOmahaUpdateIgnoredPerPolicy:
+ return "kActionCodeOmahaUpdateIgnoredPerPolicy";
+ case kActionCodeOmahaUpdateDeferredPerPolicy:
+ return "kActionCodeOmahaUpdateDeferredPerPolicy";
+ case kActionCodeOmahaErrorInHTTPResponse:
+ return "kActionCodeOmahaErrorInHTTPResponse";
+ case kActionCodeDownloadOperationHashMissingError:
+ return "kActionCodeDownloadOperationHashMissingError";
+ case kActionCodeDownloadMetadataSignatureMissingError:
+ return "kActionCodeDownloadMetadataSignatureMissingError";
+ case kActionCodeOmahaUpdateDeferredForBackoff:
+ return "kActionCodeOmahaUpdateDeferredForBackoff";
+ case kActionCodeUmaReportedMax:
+ return "kActionCodeUmaReportedMax";
+ case kActionCodeOmahaRequestHTTPResponseBase:
+ return "kActionCodeOmahaRequestHTTPResponseBase";
+ case kActionCodeResumedFlag:
+ return "Resumed";
+ case kActionCodeDevModeFlag:
+ return "DevMode";
+ case kActionCodeTestImageFlag:
+ return "TestImage";
+ case kActionCodeTestOmahaUrlFlag:
+ return "TestOmahaUrl";
+ case kSpecialFlags:
+ return "kSpecialFlags";
+ // Don't add a default case to let the compiler warn about newly added
+ // error codes which should be added here.
+ }
+
+ return "Unknown error: " + base::UintToString(static_cast<unsigned>(code));
+}
} // namespace utils
diff --git a/utils.h b/utils.h
index c2cd21d..6c17853 100644
--- a/utils.h
+++ b/utils.h
@@ -23,6 +23,8 @@
namespace chromeos_update_engine {
+class SystemState;
+
namespace utils {
// Returns true if this is an official Chrome OS build, false otherwise.
@@ -286,11 +288,15 @@
// it'll return the same value again.
ActionExitCode GetBaseErrorCode(ActionExitCode code);
-// Sends the error code to the appropriate bucket in UMA using the metrics_lib
-// interface. This method uses GetBaseErrorCode to process the given code and
-// report only the base error code.
-void SendErrorCodeToUma(MetricsLibraryInterface* metrics_lib,
- ActionExitCode code);
+// Sends the error code to UMA using the metrics interface object in the given
+// system state. It also uses the system state to determine the right UMA
+// bucket for the error code.
+void SendErrorCodeToUma(SystemState* system_state, ActionExitCode code);
+
+// Returns a string representation of the ActionExitCodes (either the base
+// error codes or the bit flags) for logging purposes.
+std::string CodeToString(ActionExitCode code);
+
} // namespace utils