[clearcut uploader] Try more than 5 times and log response headers.
Also in this CL:
- Uploader takes a SteadyClockInterface and a SleeperInterface so that
these may be faked in tests.
- Added more test cases to uploader_test.
- Start actually building and running the clearcut_uploader_test
- gitfmt.py, and clang-tidy.py: Adds the ability to override some
skipped directories so that subdirectories of skipped directories
can be non-skipped. We apply this to third_party/clearcut.
Change-Id: Ie0c80829cb82dd1b77b440e9bd5ffcbfd9ea6ed6
diff --git a/BUILD.gn b/BUILD.gn
index 829cae1..a3481a4 100644
--- a/BUILD.gn
+++ b/BUILD.gn
@@ -3,8 +3,8 @@
# found in the LICENSE file.
import("//build/test.gni")
-import("$cobalt_root/metrics_registry.gni")
import("//third_party/protobuf/proto_library.gni")
+import("$cobalt_root/metrics_registry.gni")
declare_args() {
extra_package_labels = []
@@ -33,6 +33,7 @@
deps = [
"keys:tests",
"src:tests",
+ "$cobalt_root/third_party/clearcut:tests",
]
}
diff --git a/third_party/clearcut/BUILD.gn b/third_party/clearcut/BUILD.gn
index 3630033..90189f7 100644
--- a/third_party/clearcut/BUILD.gn
+++ b/third_party/clearcut/BUILD.gn
@@ -4,6 +4,8 @@
import("//third_party/protobuf/proto_library.gni")
+#TODO(rudominer) Move this module out of //third_party. fxb/38558.
+
proto_library("clearcut_proto") {
sources = [
"clearcut.proto",
@@ -23,7 +25,30 @@
public_deps = [
":clearcut_proto",
"$cobalt_root/src:logging",
+ "$cobalt_root/src/lib/util:clock",
+ "$cobalt_root/src/lib/util:sleeper",
+ "$cobalt_root/src/lib/util:status",
"$cobalt_root/third_party/statusor:statusor",
"//third_party/abseil-cpp",
]
}
+
+source_set("uploader_test") {
+ testonly = true
+ sources = [
+ "uploader_test.cc",
+ ]
+ configs += [ "$cobalt_root:cobalt_config" ]
+ deps = [
+ ":clearcut",
+ "//third_party/gflags",
+ "//third_party/googletest:gtest",
+ ]
+}
+
+group("tests") {
+ testonly = true
+ deps = [
+ ":uploader_test",
+ ]
+}
diff --git a/third_party/clearcut/http_client.h b/third_party/clearcut/http_client.h
index fa6b3e3..b67f0e6 100644
--- a/third_party/clearcut/http_client.h
+++ b/third_party/clearcut/http_client.h
@@ -2,8 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
-#ifndef THIRD_PARTY_CLEARCUT_HTTP_CLIENT_H_
-#define THIRD_PARTY_CLEARCUT_HTTP_CLIENT_H_
+#ifndef COBALT_THIRD_PARTY_CLEARCUT_HTTP_CLIENT_H_
+#define COBALT_THIRD_PARTY_CLEARCUT_HTTP_CLIENT_H_
#include <future>
#include <map>
@@ -24,10 +24,15 @@
std::string response;
Status status;
int64_t http_code;
+ std::map<std::string, std::string> headers;
HTTPResponse() {}
- HTTPResponse(std::string response, Status status, int64_t http_code)
- : response(std::move(response)), status(status), http_code(http_code) {}
+ HTTPResponse(std::string response, Status status, int64_t http_code,
+ std::map<std::string, std::string> headers = std::map<std::string, std::string>())
+ : response(std::move(response)),
+ status(status),
+ http_code(http_code),
+ headers(std::move(headers)) {}
HTTPResponse(HTTPResponse&&) = default;
HTTPResponse& operator=(HTTPResponse&&) = default;
@@ -65,4 +70,4 @@
} // namespace clearcut
-#endif // THIRD_PARTY_CLEARCUT_HTTP_CLIENT_H_
+#endif // COBALT_THIRD_PARTY_CLEARCUT_HTTP_CLIENT_H_
diff --git a/third_party/clearcut/uploader.cc b/third_party/clearcut/uploader.cc
index 69bfadf..f1e8a31 100644
--- a/third_party/clearcut/uploader.cc
+++ b/third_party/clearcut/uploader.cc
@@ -14,26 +14,35 @@
namespace clearcut {
+using cobalt::util::SleeperInterface;
using cobalt::util::Status;
using cobalt::util::StatusCode;
+using cobalt::util::SteadyClockInterface;
-ClearcutUploader::ClearcutUploader(const std::string& url, std::unique_ptr<HTTPClient> client,
- int64_t upload_timeout, int64_t initial_backoff_millis)
- : url_(url),
+ClearcutUploader::ClearcutUploader(std::string url, std::unique_ptr<HTTPClient> client,
+ int64_t upload_timeout_millis, int64_t initial_backoff_millis,
+ std::unique_ptr<SteadyClockInterface> steady_clock,
+ std::unique_ptr<cobalt::util::SleeperInterface> sleeper)
+ : url_(std::move(url)),
client_(std::move(client)),
- upload_timeout_(upload_timeout),
- initial_backoff_millis_(initial_backoff_millis),
- pause_uploads_until_(std::chrono::steady_clock::now()) // Set this to now() so that we
- // can immediately upload.
-{}
+ upload_timeout_(std::chrono::milliseconds(upload_timeout_millis)),
+ initial_backoff_(std::chrono::milliseconds(initial_backoff_millis)),
+ steady_clock_(std::move(steady_clock)),
+ sleeper_(std::move(sleeper)),
+ pause_uploads_until_(steady_clock_->now()) // Set this to now() so that we
+ // can immediately upload.
+{
+ CHECK(steady_clock_);
+ CHECK(sleeper_);
+}
Status ClearcutUploader::UploadEvents(LogRequest* log_request, int32_t max_retries) {
int32_t i = 0;
auto deadline = std::chrono::steady_clock::time_point::max();
- if (upload_timeout_ > 0) {
- deadline = std::chrono::steady_clock::now() + std::chrono::milliseconds(upload_timeout_);
+ if (upload_timeout_ > std::chrono::milliseconds(0)) {
+ deadline = steady_clock_->now() + upload_timeout_;
}
- auto backoff = std::chrono::milliseconds(initial_backoff_millis_);
+ auto backoff = initial_backoff_;
while (true) {
Status response = TryUploadEvents(log_request, deadline);
if (response.ok() || ++i == max_retries) {
@@ -49,15 +58,20 @@
default:
break;
}
- if (std::chrono::steady_clock::now() > deadline) {
+ if (steady_clock_->now() > deadline) {
return Status(StatusCode::DEADLINE_EXCEEDED, "Deadline exceeded.");
}
// Exponential backoff.
- auto time_until_pause_end = pause_uploads_until_ - std::chrono::steady_clock::now();
+ auto time_until_pause_end = pause_uploads_until_ - steady_clock_->now();
if (time_until_pause_end > backoff) {
- std::this_thread::sleep_for(time_until_pause_end);
+ VLOG(5) << "ClearcutUploader: Sleeping for time requested by server: "
+ << std::chrono::duration<double>(time_until_pause_end).count() << "s";
+ sleeper_->sleep_for(
+ std::chrono::duration_cast<std::chrono::milliseconds>(time_until_pause_end));
} else {
- std::this_thread::sleep_for(backoff);
+ VLOG(5) << "ClearcutUploader: Sleeping for backoff time: "
+ << std::chrono::duration<double>(backoff).count() << "s";
+ sleeper_->sleep_for(backoff);
}
backoff *= 2;
}
@@ -65,58 +79,83 @@
Status ClearcutUploader::TryUploadEvents(LogRequest* log_request,
std::chrono::steady_clock::time_point deadline) {
- if (std::chrono::steady_clock::now() < pause_uploads_until_) {
+ if (steady_clock_->now() < pause_uploads_until_) {
return Status(StatusCode::RESOURCE_EXHAUSTED,
"Uploads are currently paused at the request of the "
"clearcut server");
}
- HTTPRequest request(url_);
log_request->mutable_client_info()->set_client_type(kFuchsiaClientType);
- if (!log_request->SerializeToString(&request.body)) {
- return Status(StatusCode::INVALID_ARGUMENT,
- "ClearcutUploader: Unable to serialize log_request to binary proto.");
- }
- VLOG(5) << "ClearcutUploader: Sending POST request to " << url_ << ".";
- auto response_future = client_->Post(std::move(request), deadline);
- auto response_or = response_future.get();
- if (!response_or.ok()) {
- const Status& status = response_or.status();
- VLOG(5) << "ClearcutUploader: Failed to send POST request: (" << status.error_code() << ") "
- << status.error_message();
- return status;
+ HTTPResponse response;
+ // Because we will be moving the request body into the Post() method it will not be available to
+ // us later. Here we keep an escaped copy of the request body just in case we need to use it
+ // in an error log message later.
+ std::string escaped_request_body;
+ {
+ HTTPRequest request(url_);
+ if (!log_request->SerializeToString(&request.body)) {
+ return Status(StatusCode::INVALID_ARGUMENT,
+ "ClearcutUploader: Unable to serialize log_request to binary proto.");
+ }
+ escaped_request_body = absl::CEscape(request.body);
+ VLOG(5) << "ClearcutUploader: Sending POST request to " << url_ << ".";
+ auto response_future = client_->Post(std::move(request), deadline);
+
+ auto response_or = response_future.get();
+ if (!response_or.ok()) {
+ const Status& status = response_or.status();
+ VLOG(5) << "ClearcutUploader: Failed to send POST request: (" << status.error_code() << ") "
+ << status.error_message();
+ return status;
+ }
+
+ response = response_or.ConsumeValueOrDie();
}
- auto response = response_or.ConsumeValueOrDie();
+ constexpr auto kHttpOk = 200;
+ constexpr auto kHttpBadRequest = 400;
+ constexpr auto kHttpUnauhtorized = 401;
+ constexpr auto kHttpForbidden = 403;
+ constexpr auto kHttpFNotFound = 404;
+ constexpr auto kHttpServiceUnavailable = 503;
VLOG(5) << "ClearcutUploader: Received POST response: " << response.http_code << ".";
- if (response.http_code != 200) {
- std::string escaped_body = absl::CEscape(request.body);
- VLOG(1) << "ClearcutUploader: Failed POST request to " << url_
- << " with request body=" << escaped_body << ".";
- }
+ if (response.http_code != kHttpOk) {
+ std::ostringstream s;
+ std::string escaped_response_body = absl::CEscape(response.response);
+ s << "ClearcutUploader: Response was not OK: " << response.http_code << ".";
+ s << " url=" << url_;
+ s << " response contained " << response.headers.size() << " <headers>";
+ for (const auto& pair : response.headers) {
+ s << "<key>" << pair.first << "</key>"
+ << ":"
+ << "<value>" << pair.second << "</value>,";
+ }
+ s << "</headers>";
+ s << " request <body>" << escaped_request_body << "</body>.";
+ s << " response <body>" << escaped_response_body << "</body>.";
+ VLOG(1) << s.str();
- std::ostringstream s;
- s << response.http_code << ": ";
- switch (response.http_code) {
- case 200: // success
- break;
- case 400: // bad request
- s << "Bad Request";
- return Status(StatusCode::INVALID_ARGUMENT, s.str());
- case 401: // Unauthorized
- case 403: // forbidden
- s << "Permission Denied";
- return Status(StatusCode::PERMISSION_DENIED, s.str());
- case 404: // not found
- s << "Not Found";
- return Status(StatusCode::NOT_FOUND, s.str());
- case 503: // service unavailable
- s << "Service Unavailable";
- return Status(StatusCode::RESOURCE_EXHAUSTED, s.str());
- default:
- s << "Unknown Error Code";
- return Status(StatusCode::UNKNOWN, s.str());
+ std::ostringstream stauts_string_stream;
+ stauts_string_stream << response.http_code << ": ";
+ switch (response.http_code) {
+ case kHttpBadRequest: // bad request
+ stauts_string_stream << "Bad Request";
+ return Status(StatusCode::INVALID_ARGUMENT, stauts_string_stream.str());
+ case kHttpUnauhtorized: // Unauthorized
+ case kHttpForbidden: // forbidden
+ stauts_string_stream << "Permission Denied";
+ return Status(StatusCode::PERMISSION_DENIED, stauts_string_stream.str());
+ case kHttpFNotFound: // not found
+ stauts_string_stream << "Not Found";
+ return Status(StatusCode::NOT_FOUND, stauts_string_stream.str());
+ case kHttpServiceUnavailable: // service unavailable
+ stauts_string_stream << "Service Unavailable";
+ return Status(StatusCode::RESOURCE_EXHAUSTED, stauts_string_stream.str());
+ default:
+ stauts_string_stream << "Unknown Error Code";
+ return Status(StatusCode::UNKNOWN, stauts_string_stream.str());
+ }
}
LogResponse log_response;
@@ -125,8 +164,8 @@
}
if (log_response.next_request_wait_millis() >= 0) {
- pause_uploads_until_ = std::chrono::steady_clock::now() +
- std::chrono::milliseconds(log_response.next_request_wait_millis());
+ pause_uploads_until_ =
+ steady_clock_->now() + std::chrono::milliseconds(log_response.next_request_wait_millis());
}
return Status::OK;
diff --git a/third_party/clearcut/uploader.h b/third_party/clearcut/uploader.h
index 028633b..8c69c4c 100644
--- a/third_party/clearcut/uploader.h
+++ b/third_party/clearcut/uploader.h
@@ -2,13 +2,15 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
-#ifndef THIRD_PARTY_CLEARCUT_LOGGER_H_
-#define THIRD_PARTY_CLEARCUT_LOGGER_H_
+#ifndef COBALT_THIRD_PARTY_CLEARCUT_UPLOADER_H_
+#define COBALT_THIRD_PARTY_CLEARCUT_UPLOADER_H_
#include <chrono>
#include <memory>
#include <vector>
+#include "src/lib/util/clock.h"
+#include "src/lib/util/sleeper.h"
#include "src/lib/util/status.h"
#include "third_party/abseil-cpp/absl/strings/escaping.h"
#include "third_party/clearcut/clearcut.pb.h"
@@ -18,15 +20,35 @@
namespace clearcut {
static const int32_t kFuchsiaClientType = 17;
-static const int32_t kMaxRetries = 5;
+static const int32_t kMaxRetries = 8; // Wait between tries: 0.25s 0.5.s 1s 2s 4s 8s 16s
+static const int64_t kInitialBackoffMillis = 250;
// A ClearcutUploader sends events to clearcut using the given HTTPClient.
//
// Note: This class is not threadsafe.
class ClearcutUploader {
public:
- ClearcutUploader(const std::string &url, std::unique_ptr<HTTPClient> client,
- int64_t upload_timeout = 0, int64_t initial_backoff_millis = 250);
+ // url: The URL of the Clearcut server to which POST requests should be sent.
+ //
+ // client: An implementation of HTTPClient to use.
+ //
+ // upload_timeout_millis: If a positive value is specified then this number of milliseconds
+ // will be used as the HTTP request timeout. Otherwise no timeout will be used.
+ //
+ // initial_backoff_millis: The initial value to use for exponential backoff, in milliseconds.
+ // This is used when the ClearcutUploader retries on failures. Optionally override the default.
+ // Mostly useful for tests.
+ //
+ // steady_clock: Optionally replace the system steady clock. Mostly useful for tests.
+ //
+ // sleeper: Optionally replace the system sleep. Mostly useful for tests.
+ ClearcutUploader(std::string url, std::unique_ptr<HTTPClient> client,
+ int64_t upload_timeout_millis = 0,
+ int64_t initial_backoff_millis = kInitialBackoffMillis,
+ std::unique_ptr<cobalt::util::SteadyClockInterface> steady_clock =
+ std::make_unique<cobalt::util::SteadyClock>(),
+ std::unique_ptr<cobalt::util::SleeperInterface> sleeper =
+ std::make_unique<cobalt::util::Sleeper>());
// Uploads the |log_request| with retries.
Status UploadEvents(LogRequest *log_request, int32_t max_retries = kMaxRetries);
@@ -37,8 +59,11 @@
const std::string url_;
const std::unique_ptr<HTTPClient> client_;
- const int64_t upload_timeout_;
- const int64_t initial_backoff_millis_;
+ const std::chrono::milliseconds upload_timeout_;
+ const std::chrono::milliseconds initial_backoff_;
+
+ const std::unique_ptr<cobalt::util::SteadyClockInterface> steady_clock_;
+ const std::unique_ptr<cobalt::util::SleeperInterface> sleeper_;
protected:
friend class UploaderTest;
@@ -50,4 +75,4 @@
} // namespace clearcut
-#endif // THIRD_PARTY_CLEARCUT_LOGGER_H_
+#endif // COBALT_THIRD_PARTY_CLEARCUT_UPLOADER_H_
diff --git a/third_party/clearcut/uploader_test.cc b/third_party/clearcut/uploader_test.cc
index 6f78038..ad3f086 100644
--- a/third_party/clearcut/uploader_test.cc
+++ b/third_party/clearcut/uploader_test.cc
@@ -7,42 +7,57 @@
#include <chrono>
#include <thread>
-#include "src/gtest.h"
+#include "src/lib/util/clock.h"
+#include "src/lib/util/sleeper.h"
#include "src/logging.h"
#include "third_party/clearcut/clearcut.pb.h"
#include "third_party/gflags/include/gflags/gflags.h"
+#include "third_party/googletest/googletest/include/gtest/gtest.h"
namespace clearcut {
+using cobalt::util::FakeSleeper;
+using cobalt::util::IncrementingSteadyClock;
using cobalt::util::StatusCode;
+using std::chrono::steady_clock;
namespace {
-static const int64_t kInitialBackoffMillis = 10;
+constexpr char kFakeClearcutURL[] = "http://test.com";
+constexpr int64_t kUploadTimeoutMillis = 31 * 1000;
+constexpr int64_t kInitialBackoffMillisForTest = 250;
+constexpr std::chrono::milliseconds kFakeClockIncrement(1);
-static std::set<uint32_t> seen_event_codes;
-static int next_request_wait_millis;
-static bool fail_next_request;
-
-class TestHTTPClient : public HTTPClient {
+class FakeHttpClient : public HTTPClient {
public:
- std::future<StatusOr<HTTPResponse>> Post(HTTPRequest request,
- std::chrono::steady_clock::time_point _ignored) {
- if (fail_next_request) {
- fail_next_request = false;
+ std::future<StatusOr<HTTPResponse>> Post(
+ HTTPRequest request, std::chrono::steady_clock::time_point deadline) override {
+ EXPECT_EQ(request.url, kFakeClearcutURL);
+ last_deadline_seen = deadline;
+ EXPECT_EQ(0, request.headers.size());
+ if (!error_statuses_to_return.empty()) {
+ auto next_status = error_statuses_to_return.front();
+ error_statuses_to_return.pop_front();
std::promise<StatusOr<HTTPResponse>> response_promise;
- response_promise.set_value(Status(StatusCode::DEADLINE_EXCEEDED, "Artificial post failure"));
+ response_promise.set_value(Status(next_status, "Artificial post failure"));
return response_promise.get_future();
}
LogRequest req;
req.ParseFromString(request.body);
- for (auto event : req.log_event()) {
+ for (auto &event : req.log_event()) {
seen_event_codes.insert(event.event_code());
}
HTTPResponse response;
- response.http_code = 200;
+ int http_response_code = 200;
+ if (!http_response_codes_to_return.empty()) {
+ http_response_code = http_response_codes_to_return.front();
+ http_response_codes_to_return.pop_front();
+ }
+ response.http_code = http_response_code;
+ response.headers["Size"] = "LARGE";
+ response.headers["Color"] = "PURPLE";
LogResponse resp;
if (next_request_wait_millis != -1) {
resp.set_next_request_wait_millis(next_request_wait_millis);
@@ -54,27 +69,41 @@
return response_promise.get_future();
}
+
+ std::set<uint32_t> seen_event_codes = {};
+ int next_request_wait_millis = -1;
+ std::chrono::steady_clock::time_point last_deadline_seen;
+ std::deque<StatusCode> error_statuses_to_return;
+ std::deque<int> http_response_codes_to_return;
};
} // namespace
class UploaderTest : public ::testing::Test {
- void SetUp() {
- seen_event_codes = {};
- next_request_wait_millis = -1;
- fail_next_request = false;
- auto unique_client = std::make_unique<TestHTTPClient>();
+ protected:
+ void DoSetUp(int64_t upload_timeout_millis) {
+ auto unique_client = std::make_unique<FakeHttpClient>();
client = unique_client.get();
- uploader = std::make_unique<ClearcutUploader>("http://test.com", std::move(unique_client), 0,
- kInitialBackoffMillis);
+ auto unique_clock = std::make_unique<IncrementingSteadyClock>(kFakeClockIncrement);
+ fake_clock = unique_clock.get();
+ auto unique_sleeper = std::make_unique<FakeSleeper>();
+ fake_sleeper = unique_sleeper.get();
+ fake_sleeper->set_incrementing_steady_clock(fake_clock);
+ uploader = std::make_unique<ClearcutUploader>(
+ kFakeClearcutURL, std::move(unique_client), upload_timeout_millis,
+ kInitialBackoffMillisForTest, std::move(unique_clock), std::move(unique_sleeper));
}
+ void SetUp() override { DoSetUp(kUploadTimeoutMillis); }
+
public:
Status UploadClearcutDemoEvent(uint32_t event_code, int32_t max_retries = 1) {
LogRequest request;
+ constexpr int32_t kClearcutDemoSource = 12345;
request.set_log_source(kClearcutDemoSource);
request.add_log_event()->set_event_code(event_code);
- return uploader->UploadEvents(&request, max_retries);
+ auto status = uploader->UploadEvents(&request, max_retries);
+ return status;
}
std::chrono::steady_clock::time_point get_pause_uploads_until() {
@@ -82,11 +111,13 @@
}
bool SawEventCode(uint32_t event_code) {
- return seen_event_codes.find(event_code) != seen_event_codes.end();
+ return client->seen_event_codes.find(event_code) != client->seen_event_codes.end();
}
std::unique_ptr<ClearcutUploader> uploader;
- TestHTTPClient *client;
+ FakeHttpClient *client;
+ IncrementingSteadyClock *fake_clock;
+ FakeSleeper *fake_sleeper;
};
TEST_F(UploaderTest, BasicClearcutDemoUpload) {
@@ -101,44 +132,149 @@
ASSERT_TRUE(SawEventCode(4));
}
+// Tests that the correct value for the timeout is passed to the HttpClient.
+TEST_F(UploaderTest, CorrectDeadlineUsed) {
+ auto expected_deadline =
+ fake_clock->peek_later() + std::chrono::milliseconds(kUploadTimeoutMillis);
+ ASSERT_TRUE(UploadClearcutDemoEvent(1).ok());
+ EXPECT_EQ(client->last_deadline_seen, expected_deadline);
+
+ // Advance time by 10 minutes.
+ fake_clock->increment_by(std::chrono::minutes(10));
+ expected_deadline = fake_clock->peek_later() + std::chrono::milliseconds(kUploadTimeoutMillis);
+ ASSERT_TRUE(UploadClearcutDemoEvent(2).ok());
+ EXPECT_EQ(client->last_deadline_seen, expected_deadline);
+}
+
+// Tests the functionality of obeying a request from the server to rate-limit.
TEST_F(UploaderTest, RateLimitingWorks) {
- next_request_wait_millis = 10;
+ // Have the fake HTTP client return a response to the Uploader telling it that the server
+ // is asking it to wait 10ms.
+ client->next_request_wait_millis = 10;
+ fake_clock->set_time(std::chrono::steady_clock::time_point(std::chrono::milliseconds(0)));
ASSERT_TRUE(UploadClearcutDemoEvent(100).ok());
ASSERT_TRUE(SawEventCode(100));
- int code = 150;
- // Repeatedly try to upload until we pass the uploader's
- // "pause_uploads_until_" field.
- while (std::chrono::steady_clock::now() < get_pause_uploads_until()) {
- ASSERT_FALSE(UploadClearcutDemoEvent(code).ok());
+ // Haven't waited long enough. Expect the Uploader to return an error.
+ fake_clock->set_time(std::chrono::steady_clock::time_point(std::chrono::milliseconds(8)));
+ ASSERT_EQ(UploadClearcutDemoEvent(150).error_code(), StatusCode::RESOURCE_EXHAUSTED);
+ ASSERT_FALSE(SawEventCode(150));
- // We haven't waited long enough yet.
- ASSERT_FALSE(SawEventCode(code));
+ // Haven't waited long enough. Expect the Uploader to return an error.
+ fake_clock->set_time(std::chrono::steady_clock::time_point(std::chrono::milliseconds(9)));
+ ASSERT_EQ(UploadClearcutDemoEvent(150).error_code(), StatusCode::RESOURCE_EXHAUSTED);
+ ASSERT_FALSE(SawEventCode(150));
- code++;
-
- std::this_thread::sleep_for(std::chrono::milliseconds(1));
- }
-
- // Assert that we've made at least 1 rate limit verification pass.
- ASSERT_GT(code, 150);
-
- // Now that the pause has expired, we should be able to upload.
- ASSERT_TRUE(UploadClearcutDemoEvent(code).ok());
-
- // Now it should work.
- ASSERT_TRUE(SawEventCode(code));
+ // Now we have waited long enough.
+ fake_clock->set_time(std::chrono::steady_clock::time_point(std::chrono::milliseconds(12)));
+ ASSERT_TRUE(UploadClearcutDemoEvent(150).ok());
+ ASSERT_TRUE(SawEventCode(150));
}
+// Tests the functionality of retrying multiple times with exponential backoff.
TEST_F(UploaderTest, ShouldRetryOnFailedUpload) {
- fail_next_request = true;
- ASSERT_TRUE(UploadClearcutDemoEvent(1, 2).ok());
- ASSERT_TRUE(SawEventCode(1));
+ // Arrange for the upload to fail three times.
+ client->error_statuses_to_return = {StatusCode::DEADLINE_EXCEEDED, StatusCode::RESOURCE_EXHAUSTED,
+ StatusCode::DEADLINE_EXCEEDED};
+ // Try to upload three times. Expect this to fail and return the third status code.
+ fake_sleeper->Reset();
+ EXPECT_EQ(UploadClearcutDemoEvent(101, 3).error_code(), StatusCode::DEADLINE_EXCEEDED);
+ EXPECT_FALSE(SawEventCode(101));
+ // There should have been two sleeps.
+ EXPECT_EQ(fake_sleeper->last_sleep_duration().count(), kInitialBackoffMillisForTest * 2);
- fail_next_request = true;
- ASSERT_FALSE(UploadClearcutDemoEvent(2).ok());
- ASSERT_TRUE(UploadClearcutDemoEvent(3).ok());
- ASSERT_TRUE(SawEventCode(3));
+ // Arrange for the upload to fail three times.
+ client->error_statuses_to_return = {StatusCode::DEADLINE_EXCEEDED, StatusCode::RESOURCE_EXHAUSTED,
+ StatusCode::DEADLINE_EXCEEDED};
+ // Try to upload four times. It should succeed the fourth time.
+ fake_sleeper->Reset();
+ ASSERT_TRUE(UploadClearcutDemoEvent(102, 4).ok());
+ ASSERT_TRUE(SawEventCode(102));
+ // There should have been three sleeps.
+ EXPECT_EQ(fake_sleeper->last_sleep_duration().count(), kInitialBackoffMillisForTest * 4);
+
+ // Arrange for the upload to fail four times.
+ client->error_statuses_to_return = {StatusCode::DEADLINE_EXCEEDED, StatusCode::RESOURCE_EXHAUSTED,
+ StatusCode::DEADLINE_EXCEEDED,
+ StatusCode::RESOURCE_EXHAUSTED};
+ // Try to upload four times. Expect this to fail and return the fourth status code.
+ fake_sleeper->Reset();
+ EXPECT_EQ(UploadClearcutDemoEvent(103, 4).error_code(), StatusCode::RESOURCE_EXHAUSTED);
+ ASSERT_FALSE(SawEventCode(103));
+ // There should have be three sleeps.
+ EXPECT_EQ(fake_sleeper->last_sleep_duration().count(), kInitialBackoffMillisForTest * 4);
+
+ // Arrange for the upload to fail four times.
+ client->error_statuses_to_return = {StatusCode::DEADLINE_EXCEEDED, StatusCode::RESOURCE_EXHAUSTED,
+ StatusCode::DEADLINE_EXCEEDED,
+ StatusCode::RESOURCE_EXHAUSTED};
+ // Try to upload five times. Expect this to succeed the fifth time.
+ fake_sleeper->Reset();
+ EXPECT_TRUE(UploadClearcutDemoEvent(104, 5).ok());
+ ASSERT_TRUE(SawEventCode(104));
+ // There should have be 4 sleeps.
+ EXPECT_EQ(fake_sleeper->last_sleep_duration().count(), kInitialBackoffMillisForTest * 8);
+
+ // Arrange for the upload to fail three times but return a non-retryable error the first time.
+ client->error_statuses_to_return = {StatusCode::INVALID_ARGUMENT, StatusCode::NOT_FOUND,
+ StatusCode::PERMISSION_DENIED};
+ // Set up to try four times but we will in fact only try once because the first time
+ // we return a non-retryable error.
+ fake_sleeper->Reset();
+ ASSERT_EQ(UploadClearcutDemoEvent(105, 4).error_code(), StatusCode::INVALID_ARGUMENT);
+ ASSERT_FALSE(SawEventCode(105));
+ EXPECT_EQ(fake_sleeper->last_sleep_duration().count(), 0);
+}
+
+// Tests that we will stop retrying when the timeout is exceeded.
+TEST_F(UploaderTest, ExceedsTimeout) {
+ // Do the setup again with a short timeout of 2s.
+ DoSetUp(2000);
+ // Arrange for the upload to fail seven times.
+ client->error_statuses_to_return = {
+ StatusCode::RESOURCE_EXHAUSTED, StatusCode::RESOURCE_EXHAUSTED,
+ StatusCode::RESOURCE_EXHAUSTED, StatusCode::RESOURCE_EXHAUSTED,
+ StatusCode::RESOURCE_EXHAUSTED, StatusCode::RESOURCE_EXHAUSTED,
+ StatusCode::RESOURCE_EXHAUSTED};
+ fake_sleeper->Reset();
+ // Arrange to try to upload ten times. This will fail after only five attempts because
+ // the two-second deadline will have been exceeded. The returned error will be
+ // DEADLINE_EXCEEDED.
+ EXPECT_EQ(UploadClearcutDemoEvent(101, 10).error_code(), StatusCode::DEADLINE_EXCEEDED);
+ EXPECT_FALSE(SawEventCode(101));
+ // There should have be only 4 sleeps because after five upload attempts the deadline
+ // will have been exceeded.
+ EXPECT_EQ(fake_sleeper->last_sleep_duration().count(), kInitialBackoffMillisForTest * 8);
+}
+
+// Tests the logic for handling non-200 HTTP response codes.
+TEST_F(UploaderTest, HttpResponseCodes) {
+ std::vector<std::pair<int, StatusCode>> codes = {
+ {400, StatusCode::INVALID_ARGUMENT}, {401, StatusCode::PERMISSION_DENIED},
+ {403, StatusCode::PERMISSION_DENIED}, {404, StatusCode::NOT_FOUND},
+ {503, StatusCode::RESOURCE_EXHAUSTED}, {500, StatusCode::UNKNOWN}};
+ for (const auto &code : codes) {
+ client->http_response_codes_to_return = {code.first};
+ EXPECT_EQ(UploadClearcutDemoEvent(1, 1).error_code(), code.second);
+ }
+ // Arrange for the upload to fail three times.
+ client->http_response_codes_to_return = {500, 500, 503};
+ // Try to upload three times. Expect this to fail and return the third status code.
+ fake_sleeper->Reset();
+ EXPECT_EQ(UploadClearcutDemoEvent(101, 3).error_code(), StatusCode::RESOURCE_EXHAUSTED);
+ // The server returned an error but still saw the request.
+ EXPECT_TRUE(SawEventCode(101));
+ // There should have been two sleeps.
+ EXPECT_EQ(fake_sleeper->last_sleep_duration().count(), kInitialBackoffMillisForTest * 2);
+
+ // Arrange for the upload to fail three times.
+ client->http_response_codes_to_return = {503, 503, 503};
+ // Try to upload four times. It should succeed the fourth time.
+ fake_sleeper->Reset();
+ ASSERT_TRUE(UploadClearcutDemoEvent(102, 4).ok());
+ ASSERT_TRUE(SawEventCode(102));
+ // There should have been three sleeps.
+ EXPECT_EQ(fake_sleeper->last_sleep_duration().count(), kInitialBackoffMillisForTest * 4);
}
} // namespace clearcut
diff --git a/tools/clang_tidy.py b/tools/clang_tidy.py
index fa2acde..0044779 100755
--- a/tools/clang_tidy.py
+++ b/tools/clang_tidy.py
@@ -32,6 +32,14 @@
'source_generator', 'source_generator_test_files'),
]
+# A list of directory prefixes that extend directory prefixes from SKIP_LINT_DIRS
+# but that should not be skipped.
+# TODO(rudominer) Can eliminate third_party/clearcut below when
+# fxb/38558 is resolved.
+OVERRIDE_SKIP_LINT_DIRS = [
+ os.path.join(SRC_ROOT_DIR, 'third_party', 'clearcut')
+]
+
TEST_FILE_REGEX = re.compile('.*_(unit)?tests?.cc$')
TEST_FILE_CLANG_TIDY_CHECKS = [
'-readability-magic-numbers',
@@ -39,6 +47,21 @@
]
+# Given a directory's parent path and name, returns a boolean indicating whether
+# or not the directory should be skipped for liniting.
+def should_skip_dir(parent_path, name):
+ if name.startswith('.'):
+ return True
+ full_path = os.path.join(parent_path, name)
+ for p in OVERRIDE_SKIP_LINT_DIRS:
+ if p.startswith(full_path):
+ return False
+ for p in SKIP_LINT_DIRS:
+ if full_path.startswith(p):
+ return True
+ return False
+
+
def main(only_directories=[]):
status = 0
@@ -62,10 +85,7 @@
clang_tidy_files.append(full_path)
# Before recursing into directories remove the ones we want to skip.
- dirs_to_skip = [
- dir for dir in dirs
- if dir.startswith('.') or os.path.join(root, dir) in SKIP_LINT_DIRS
- ]
+ dirs_to_skip = [dir for dir in dirs if should_skip_dir(root, dir)]
for d in dirs_to_skip:
dirs.remove(d)
@@ -74,26 +94,29 @@
if f.endswith('.h') and not os.path.islink(f)
]
print('Running check-header-guards.py on %d files' % len(header_files))
- try:
- subprocess.check_call([CHECK_HEADER_GUARDS] + header_files)
- except:
- status += 1
+ if len(header_files) > 0:
+ try:
+ subprocess.check_call([CHECK_HEADER_GUARDS] + header_files)
+ except:
+ status += 1
clang_tidy_command = [CLANG_TIDY, '-quiet', '-p', OUT_DIR]
print('Running clang-tidy on %d source files' % len(clang_tidy_files))
- try:
- subprocess.check_call(clang_tidy_command + clang_tidy_files)
- except:
- status += 1
+ if len(clang_tidy_files) > 0:
+ try:
+ subprocess.check_call(clang_tidy_command + clang_tidy_files)
+ except:
+ status += 1
print('Running clang-tidy on %d test files' % len(clang_tidy_test_files))
- try:
- subprocess.check_call(
- clang_tidy_command +
- ['-checks=%s' % ','.join(TEST_FILE_CLANG_TIDY_CHECKS)] +
- clang_tidy_test_files)
- except:
- status += 1
+ if len(clang_tidy_test_files) > 0:
+ try:
+ subprocess.check_call(
+ clang_tidy_command +
+ ['-checks=%s' % ','.join(TEST_FILE_CLANG_TIDY_CHECKS)] +
+ clang_tidy_test_files)
+ except:
+ status += 1
return status
diff --git a/tools/gitfmt.py b/tools/gitfmt.py
index 1f84f25..98f6609 100755
--- a/tools/gitfmt.py
+++ b/tools/gitfmt.py
@@ -186,8 +186,17 @@
"src/bin/config_parser/src/source_generator/source_generator_test_files",
]
+# A list of file prefixes that extend file prefixes from IGNORED_FILES
+# but that should not be ignored.
+# TODO(rudominer) Can eliminate third_party/clearcut below when
+# fxb/38558 is resolved.
+OVERRIDE_IGNORED_FILES = ["third_party/clearcut"]
+
def _ignored_file(f):
+ for name in OVERRIDE_IGNORED_FILES:
+ if f.startswith(name):
+ return False
for name in IGNORED_FILES:
if f.startswith(name):
return True
@@ -230,7 +239,7 @@
session = FormattingSession(repo_path)
session.format_changed(changes)
finally:
- os.chdir(repo_path)
+ os.chdir(cwd)
if __name__ == "__main__":
