update_engine: Add internal error codes that used for metrics.
Whenever we are dealing with some problem that require defining error
codes and sending UMA metrics, we need to define a new ErrorCode enum
value. These error codes then will be used to send which UMA metric
value to send. Some of the UMA metrics seems to have circumvent this
process by introducing their own error codes without adding them to the
global list of error codes.
This CL introduces three new error codes:
- kInternalLibCurlError
- kUnresolvedHostError
- kUnresolvedHostRecovered (Technically not an error code, but fits the
description and use case of it.)
That are then translated to the UMA metric values we send for
DownloadErrorCode.
In addition, this CL moves the responsibility of sending these UMA
metrics from LibCurlHttpFetcher to OmahaRequestAction which is the more
correct place to send it because that's where the operations are
completed (success or failure) and we can safely decide on the value of
UMA without risking to send overlapping or duplicated metrics.
For example, previously we send kInternalLibCurlError in conjunction
with the kUnresolvedHostError. But doing this can hide the fact that
these two error codes might be related and caused by the same underlying
issue.
Same goes for kUnresolvedHostError and kUnresolvedHosRecovered. If we
send both these metrics at the same time, then we need to subtract the
number of kUnresolvedHosRecovered from kUnresolvedHostError to figure out
the number of unresolved host errors that did not recover. By
exclusively sending one or another. We can see exactly how many are
recovered and how many did not. Although this might change the meaning
of kUnresolvedHostError metric, in the long term it will not be an issue
as all the results will converge to the new behavior.
The enum.xml (chrome) is updated in crrev.com/c/1774101
BUG=None
TEST=cros_workon_make --board=amd64-generic --test --noreconf update_engine
Change-Id: I3c7bb5f6159a0bc3a37d55666572b9cd6730f3cb
Reviewed-on: https://chromium-review.googlesource.com/c/aosp/platform/system/update_engine/+/1759544
Reviewed-by: Nicolas Norvez <norvez@chromium.org>
Tested-by: Amin Hassani <ahassani@chromium.org>
Commit-Queue: Amin Hassani <ahassani@chromium.org>
diff --git a/common/error_code.h b/common/error_code.h
index 252cc42..3dd7402 100644
--- a/common/error_code.h
+++ b/common/error_code.h
@@ -80,6 +80,9 @@
kRollbackNotPossible = 54,
kFirstActiveOmahaPingSentPersistenceError = 55,
kVerityCalculationError = 56,
+ kInternalLibCurlError = 57,
+ kUnresolvedHostError = 58,
+ kUnresolvedHostRecovered = 59,
// VERY IMPORTANT! When adding new error codes:
//
diff --git a/common/error_code_utils.cc b/common/error_code_utils.cc
index b0bbbd4..5bcbaa4 100644
--- a/common/error_code_utils.cc
+++ b/common/error_code_utils.cc
@@ -161,6 +161,12 @@
return "ErrorCode::kFirstActiveOmahaPingSentPersistenceError";
case ErrorCode::kVerityCalculationError:
return "ErrorCode::kVerityCalculationError";
+ case ErrorCode::kInternalLibCurlError:
+ return "ErrorCode::kInternalLibCurlError";
+ case ErrorCode::kUnresolvedHostError:
+ return "ErrorCode::kUnresolvedHostError";
+ case ErrorCode::kUnresolvedHostRecovered:
+ return "ErrorCode::kUnresolvedHostRecovered";
// Don't add a default case to let the compiler warn about newly added
// error codes which should be added here.
}
diff --git a/common/http_fetcher.h b/common/http_fetcher.h
index 94f31d7..f74a0f0 100644
--- a/common/http_fetcher.h
+++ b/common/http_fetcher.h
@@ -59,6 +59,12 @@
HttpFetcherDelegate* delegate() const { return delegate_; }
int http_response_code() const { return http_response_code_; }
+ // Returns additional error code that can't be expressed in terms of an HTTP
+ // response code. For example, if there was a specific internal error code in
+ // the objects used in the implementation of this class (like libcurl) that we
+ // are interested about, we can communicate it through this value.
+ ErrorCode GetAuxiliaryErrorCode() const { return auxiliary_error_code_; }
+
// Optional: Post data to the server. The HttpFetcher should make a copy
// of this data and upload it via HTTP POST during the transfer. The type of
// the data is necessary for properly setting the Content-Type HTTP header.
@@ -159,6 +165,10 @@
// set to the response code when the transfer is complete.
int http_response_code_;
+ // Set when there is an error that can't be expressed in the form of
+ // |http_response_code_|.
+ ErrorCode auxiliary_error_code_{ErrorCode::kSuccess};
+
// The delegate; may be null.
HttpFetcherDelegate* delegate_;
@@ -209,13 +219,6 @@
// situations. It's OK to destroy the |fetcher| object in this callback.
virtual void TransferComplete(HttpFetcher* fetcher, bool successful) = 0;
virtual void TransferTerminated(HttpFetcher* fetcher) {}
-
- // This allows |HttpFetcher| to send UMA metrics for its internal states
- // (unrecoverable libcurl internal error, etc.).
- virtual void ReportUpdateCheckMetrics(
- metrics::CheckResult result,
- metrics::CheckReaction reaction,
- metrics::DownloadErrorCode download_error_code) {}
};
} // namespace chromeos_update_engine
diff --git a/libcurl_http_fetcher.cc b/libcurl_http_fetcher.cc
index ad823cf..4bea4ef 100644
--- a/libcurl_http_fetcher.cc
+++ b/libcurl_http_fetcher.cc
@@ -438,10 +438,7 @@
// In case of an update check, we send UMA metrics and log the error.
if (is_update_check_ &&
(retcode == CURLM_OUT_OF_MEMORY || retcode == CURLM_INTERNAL_ERROR)) {
- delegate_->ReportUpdateCheckMetrics(
- metrics::CheckResult::kUnset,
- metrics::CheckReaction::kUnset,
- metrics::DownloadErrorCode::kInternalError);
+ auxiliary_error_code_ = ErrorCode::kInternalLibCurlError;
LOG(ERROR) << "curl_multi_perform is in an unrecoverable error condition: "
<< retcode;
} else if (retcode != CURLM_OK) {
@@ -478,19 +475,14 @@
if (curl_code == CURLE_COULDNT_RESOLVE_HOST) {
LOG(ERROR) << "libcurl can not resolve host.";
unresolved_host_state_machine_.UpdateState(true);
- if (delegate_) {
- delegate_->ReportUpdateCheckMetrics(
- metrics::CheckResult::kUnset,
- metrics::CheckReaction::kUnset,
- metrics::DownloadErrorCode::kUnresolvedHost);
- }
+ auxiliary_error_code_ = ErrorCode::kUnresolvedHostError;
}
}
// we're done!
CleanUp();
- if (unresolved_host_state_machine_.getState() ==
+ if (unresolved_host_state_machine_.GetState() ==
UnresolvedHostStateMachine::State::kRetry) {
// Based on
// https://curl.haxx.se/docs/todo.html#updated_DNS_server_while_running,
@@ -499,14 +491,9 @@
no_network_max_retries_++;
LOG(INFO) << "Will retry after reloading resolv.conf because last attempt "
"failed to resolve host.";
- } else if (unresolved_host_state_machine_.getState() ==
+ } else if (unresolved_host_state_machine_.GetState() ==
UnresolvedHostStateMachine::State::kRetriedSuccess) {
- if (delegate_) {
- delegate_->ReportUpdateCheckMetrics(
- metrics::CheckResult::kUnset,
- metrics::CheckReaction::kUnset,
- metrics::DownloadErrorCode::kUnresolvedHostRecovered);
- }
+ auxiliary_error_code_ = ErrorCode::kUnresolvedHostRecovered;
}
// TODO(petkov): This temporary code tries to deal with the case where the
diff --git a/libcurl_http_fetcher.h b/libcurl_http_fetcher.h
index 8f4258d..97a9a87 100644
--- a/libcurl_http_fetcher.h
+++ b/libcurl_http_fetcher.h
@@ -50,7 +50,7 @@
kNotRetry = 3,
};
- State getState() { return state_; }
+ State GetState() { return state_; }
// Updates the following internal state machine:
//
@@ -159,6 +159,8 @@
}
private:
+ FRIEND_TEST(LibcurlHttpFetcherTest, HostResolvedTest);
+
// libcurl's CURLOPT_CLOSESOCKETFUNCTION callback function. Called when
// closing a socket created with the CURLOPT_OPENSOCKETFUNCTION callback.
static int LibcurlCloseSocketCallback(void* clientp, curl_socket_t item);
@@ -168,7 +170,7 @@
void ProxiesResolved();
// Asks libcurl for the http response code and stores it in the object.
- void GetHttpResponseCode();
+ virtual void GetHttpResponseCode();
// Returns the last |CURLcode|.
CURLcode GetCurlCode();
diff --git a/libcurl_http_fetcher_unittest.cc b/libcurl_http_fetcher_unittest.cc
index 7f00dae..20f05b9 100644
--- a/libcurl_http_fetcher_unittest.cc
+++ b/libcurl_http_fetcher_unittest.cc
@@ -19,10 +19,12 @@
#include <string>
#include <brillo/message_loops/fake_message_loop.h>
+#include <gmock/gmock.h>
#include <gtest/gtest.h>
#include "update_engine/common/fake_hardware.h"
#include "update_engine/common/mock_proxy_resolver.h"
+#include "update_engine/mock_libcurl_http_fetcher.h"
using std::string;
@@ -42,7 +44,7 @@
brillo::FakeMessageLoop loop_{nullptr};
FakeHardware fake_hardware_;
- LibcurlHttpFetcher libcurl_fetcher_{nullptr, &fake_hardware_};
+ MockLibcurlHttpFetcher libcurl_fetcher_{nullptr, &fake_hardware_};
UnresolvedHostStateMachine state_machine_;
};
@@ -83,7 +85,7 @@
int no_network_max_retries = 1;
libcurl_fetcher_.set_no_network_max_retries(no_network_max_retries);
- libcurl_fetcher_.BeginTransfer("not-an-URL");
+ libcurl_fetcher_.BeginTransfer("not-a-URL");
while (loop_.PendingTasks()) {
loop_.RunOnce(true);
}
@@ -92,10 +94,34 @@
no_network_max_retries);
}
-TEST_F(LibcurlHttpFetcherTest, CouldntResolveHostTest) {
+TEST_F(LibcurlHttpFetcherTest, CouldNotResolveHostTest) {
int no_network_max_retries = 1;
libcurl_fetcher_.set_no_network_max_retries(no_network_max_retries);
+ libcurl_fetcher_.BeginTransfer("https://An-uNres0lvable-uRl.invalid");
+
+ // The first time it can't resolve.
+ loop_.RunOnce(true);
+ EXPECT_EQ(libcurl_fetcher_.GetAuxiliaryErrorCode(),
+ ErrorCode::kUnresolvedHostError);
+
+ while (loop_.PendingTasks()) {
+ loop_.RunOnce(true);
+ }
+ // The auxilary error code should've have been changed.
+ EXPECT_EQ(libcurl_fetcher_.GetAuxiliaryErrorCode(),
+ ErrorCode::kUnresolvedHostError);
+
+ // If libcurl fails to resolve the name, we call res_init() to reload
+ // resolv.conf and retry exactly once more. See crbug.com/982813 for details.
+ EXPECT_EQ(libcurl_fetcher_.get_no_network_max_retries(),
+ no_network_max_retries + 1);
+}
+
+TEST_F(LibcurlHttpFetcherTest, HostResolvedTest) {
+ int no_network_max_retries = 2;
+ libcurl_fetcher_.set_no_network_max_retries(no_network_max_retries);
+
// This test actually sends request to internet but according to
// https://tools.ietf.org/html/rfc2606#section-2, .invalid domain names are
// reserved and sure to be invalid. Ideally we should mock libcurl or
@@ -104,9 +130,33 @@
// TODO(xiaochu) Refactor LibcurlHttpFetcher (and its relates) so it's
// easier to mock the part that depends on internet connectivity.
libcurl_fetcher_.BeginTransfer("https://An-uNres0lvable-uRl.invalid");
+
+ // The first time it can't resolve.
+ loop_.RunOnce(true);
+ EXPECT_EQ(libcurl_fetcher_.GetAuxiliaryErrorCode(),
+ ErrorCode::kUnresolvedHostError);
+
+ // The second time, it will resolve, with error code 200 but we set the
+ // download size be smaller than the transfer size so it will retry again.
+ EXPECT_CALL(libcurl_fetcher_, GetHttpResponseCode())
+ .WillOnce(testing::Invoke(
+ [this]() { libcurl_fetcher_.http_response_code_ = 200; }))
+ .WillRepeatedly(testing::Invoke(
+ [this]() { libcurl_fetcher_.http_response_code_ = 0; }));
+ libcurl_fetcher_.transfer_size_ = 10;
+
+ // This time the host is resolved. But after that again we can't resolve
+ // anymore (See above).
+ loop_.RunOnce(true);
+ EXPECT_EQ(libcurl_fetcher_.GetAuxiliaryErrorCode(),
+ ErrorCode::kUnresolvedHostRecovered);
+
while (loop_.PendingTasks()) {
loop_.RunOnce(true);
}
+ // The auxilary error code should not have been changed.
+ EXPECT_EQ(libcurl_fetcher_.GetAuxiliaryErrorCode(),
+ ErrorCode::kUnresolvedHostRecovered);
// If libcurl fails to resolve the name, we call res_init() to reload
// resolv.conf and retry exactly once more. See crbug.com/982813 for details.
@@ -117,21 +167,21 @@
TEST_F(LibcurlHttpFetcherTest, HttpFetcherStateMachineRetryFailedTest) {
state_machine_.UpdateState(true);
state_machine_.UpdateState(true);
- EXPECT_EQ(state_machine_.getState(),
+ EXPECT_EQ(state_machine_.GetState(),
UnresolvedHostStateMachine::State::kNotRetry);
}
TEST_F(LibcurlHttpFetcherTest, HttpFetcherStateMachineRetrySucceedTest) {
state_machine_.UpdateState(true);
state_machine_.UpdateState(false);
- EXPECT_EQ(state_machine_.getState(),
+ EXPECT_EQ(state_machine_.GetState(),
UnresolvedHostStateMachine::State::kRetriedSuccess);
}
TEST_F(LibcurlHttpFetcherTest, HttpFetcherStateMachineNoRetryTest) {
state_machine_.UpdateState(false);
state_machine_.UpdateState(false);
- EXPECT_EQ(state_machine_.getState(),
+ EXPECT_EQ(state_machine_.GetState(),
UnresolvedHostStateMachine::State::kInit);
}
diff --git a/metrics_constants.h b/metrics_constants.h
index 167e577..db21d90 100644
--- a/metrics_constants.h
+++ b/metrics_constants.h
@@ -64,10 +64,10 @@
// calling res_init() can recover.
kUnresolvedHostRecovered = 97,
// This error is reported when libcurl returns CURLE_COULDNT_RESOLVE_HOST.
- kUnresolvedHost = 98,
+ kUnresolvedHostError = 98,
// This error is reported when libcurl has an internal error that
// update_engine can't recover from.
- kInternalError = 99,
+ kInternalLibCurlError = 99,
// This error code is used to convey that malformed input was given
// to the utils::GetDownloadErrorCode() function. This should never
diff --git a/metrics_utils.cc b/metrics_utils.cc
index 88c8d52..efbd067 100644
--- a/metrics_utils.cc
+++ b/metrics_utils.cc
@@ -43,6 +43,9 @@
return metrics::AttemptResult::kUpdateSucceededNotActive;
case ErrorCode::kDownloadTransferError:
+ case ErrorCode::kInternalLibCurlError:
+ case ErrorCode::kUnresolvedHostError:
+ case ErrorCode::kUnresolvedHostRecovered:
return metrics::AttemptResult::kPayloadDownloadError;
case ErrorCode::kDownloadInvalidMetadataSize:
@@ -168,6 +171,13 @@
case ErrorCode::kDownloadTransferError:
return metrics::DownloadErrorCode::kDownloadError;
+ case ErrorCode::kInternalLibCurlError:
+ return metrics::DownloadErrorCode::kInternalLibCurlError;
+ case ErrorCode::kUnresolvedHostError:
+ return metrics::DownloadErrorCode::kUnresolvedHostError;
+ case ErrorCode::kUnresolvedHostRecovered:
+ return metrics::DownloadErrorCode::kUnresolvedHostRecovered;
+
// All of these error codes are not related to downloading so break
// out so we can warn and return InputMalformed.
case ErrorCode::kSuccess:
diff --git a/mock_libcurl_http_fetcher.h b/mock_libcurl_http_fetcher.h
new file mode 100644
index 0000000..a8ef0f4
--- /dev/null
+++ b/mock_libcurl_http_fetcher.h
@@ -0,0 +1,37 @@
+//
+// Copyright (C) 2019 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+//
+
+#ifndef UPDATE_ENGINE_MOCK_LIBCURL_HTTP_FETCHER_H_
+#define UPDATE_ENGINE_MOCK_LIBCURL_HTTP_FETCHER_H_
+
+#include <gmock/gmock.h>
+
+#include "update_engine/connection_manager_interface.h"
+
+namespace chromeos_update_engine {
+
+class MockLibcurlHttpFetcher : public LibcurlHttpFetcher {
+ public:
+ MockLibcurlHttpFetcher(ProxyResolver* proxy_resolver,
+ HardwareInterface* hardware)
+ : LibcurlHttpFetcher(proxy_resolver, hardware) {}
+
+ MOCK_METHOD0(GetHttpResponseCode, void());
+};
+
+} // namespace chromeos_update_engine
+
+#endif // UPDATE_ENGINE_MOCK_LIBCURL_HTTP_FETCHER_H_
diff --git a/omaha_request_action.cc b/omaha_request_action.cc
index 6c67a3b..4d86586 100644
--- a/omaha_request_action.cc
+++ b/omaha_request_action.cc
@@ -851,6 +851,17 @@
return;
}
+ ErrorCode aux_error_code = fetcher->GetAuxiliaryErrorCode();
+ if (aux_error_code != ErrorCode::kSuccess) {
+ metrics::DownloadErrorCode download_error_code =
+ metrics_utils::GetDownloadErrorCode(aux_error_code);
+ system_state_->metrics_reporter()->ReportUpdateCheckMetrics(
+ system_state_,
+ metrics::CheckResult::kUnset,
+ metrics::CheckReaction::kUnset,
+ download_error_code);
+ }
+
if (!successful) {
LOG(ERROR) << "Omaha request network transfer failed.";
int code = GetHTTPResponseCode();
@@ -980,14 +991,6 @@
}
}
-void OmahaRequestAction::ReportUpdateCheckMetrics(
- metrics::CheckResult result,
- metrics::CheckReaction reaction,
- metrics::DownloadErrorCode download_error_code) {
- system_state_->metrics_reporter()->ReportUpdateCheckMetrics(
- system_state_, result, reaction, download_error_code);
-}
-
void OmahaRequestAction::CompleteProcessing() {
ScopedActionCompleter completer(processor_, this);
OmahaResponse& output_object = const_cast<OmahaResponse&>(GetOutputObject());
@@ -1383,7 +1386,8 @@
break;
}
- ReportUpdateCheckMetrics(result, reaction, download_error_code);
+ system_state_->metrics_reporter()->ReportUpdateCheckMetrics(
+ system_state_, result, reaction, download_error_code);
}
bool OmahaRequestAction::ShouldIgnoreUpdate(const OmahaResponse& response,
diff --git a/omaha_request_action.h b/omaha_request_action.h
index 8dffb5c..12d36d9 100644
--- a/omaha_request_action.h
+++ b/omaha_request_action.h
@@ -126,11 +126,6 @@
void TransferComplete(HttpFetcher* fetcher, bool successful) override;
- void ReportUpdateCheckMetrics(
- metrics::CheckResult result,
- metrics::CheckReaction reaction,
- metrics::DownloadErrorCode download_error_code) override;
-
// Returns true if this is an Event request, false if it's an UpdateCheck.
bool IsEvent() const { return event_.get() != nullptr; }
diff --git a/payload_state.cc b/payload_state.cc
index a6c3620..355552e 100644
--- a/payload_state.cc
+++ b/payload_state.cc
@@ -365,6 +365,9 @@
case ErrorCode::kNoUpdate:
case ErrorCode::kRollbackNotPossible:
case ErrorCode::kFirstActiveOmahaPingSentPersistenceError:
+ case ErrorCode::kInternalLibCurlError:
+ case ErrorCode::kUnresolvedHostError:
+ case ErrorCode::kUnresolvedHostRecovered:
LOG(INFO) << "Not incrementing URL index or failure count for this error";
break;
diff --git a/update_manager/chromeos_policy.cc b/update_manager/chromeos_policy.cc
index fab111a..dd6cc8d 100644
--- a/update_manager/chromeos_policy.cc
+++ b/update_manager/chromeos_policy.cc
@@ -151,6 +151,9 @@
case ErrorCode::kNoUpdate:
case ErrorCode::kRollbackNotPossible:
case ErrorCode::kFirstActiveOmahaPingSentPersistenceError:
+ case ErrorCode::kInternalLibCurlError:
+ case ErrorCode::kUnresolvedHostError:
+ case ErrorCode::kUnresolvedHostRecovered:
LOG(INFO) << "Not changing URL index or failure count due to error "
<< chromeos_update_engine::utils::ErrorCodeToString(err_code)
<< " (" << static_cast<int>(err_code) << ")";