rate-limit: create ShouldRateLimit() and tests [crashpad/crashpad : main]
Jesse McKenna (Gerrit)
Jesse McKenna added 1 comment![]( "Open in Gerrit")
Related details
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 12 comments![]( "Open in Gerrit")
Thanks, Jesse!
int32_t interval_seconds);`int32_t`’s probably not right for this either. You can use `time_t` for time intervals in this code.
ShouldRateLimitResult ShouldRateLimit(time_t now,`#include <time.h>` to be able to use `time_t`.
//! \brief If the crash should be skipped, the reason why. `std::nullopt`Blank line before this, so that the line above doesn’t get lost.
int32_t attempts_in_interval;This structure isn’t written to disk or used for interchange beyond this process, so it doesn’t need precise sizing (`int32_t`). You can use an `int` or `unsigned int`. If a negative number would be meaningless, I prefer `unsigned int`, but wouldn’t object to `int`.
#include <stdint.h>Won’t be needed if you take the other suggestions in this file.
#include "base/check_op.h"I don’t see this being used here.
} else {
// The most recent upload attempt purportedly occurred in the future. If
// it “happened” at least `kBackwardsClockTolerance` in the future, assume
// that the last upload attempt time is bogus, and attempt to upload the
// report. If the most recent upload time is in the future but within
// `kBackwardsClockTolerance`, accept it and don’t attempt to upload the
// report.
if (last_upload_attempt_time - now < kBackwardsClockTolerance) {You can collapse this into `else if`.
constexpr int32_t kOneHour = 60 * 60;As elsewhere, `time_t` works for intervals of time—you can get rid of `<stdint.h>` if you make this change.
EXPECT_EQ(kUploadThrottled, should_rate_limit.skip_reason);You’ve flipped the `EXPECT_EQ`s! Chromium’s a bit all over the place on this, but I recommend writing these in natural English order. In Crashpad, we consistently write them in the natural English order:
```
EXPECT_EQ(should_rate_limit.skip_reason, kUploadThrottled);
```
Throughout this file.
// Skip upload if the last attempt was under one interval ago.
TEST(CrashReportUploadRateLimitTest, ShouldRateLimit_AttemptWithinInterval) {
const time_t now = time(nullptr);
auto should_rate_limit = ShouldRateLimit(now, now, kOneHour);
EXPECT_EQ(kUploadThrottled, should_rate_limit.skip_reason);
should_rate_limit = ShouldRateLimit(now, now - kOneHour + 1, kOneHour);
EXPECT_EQ(kUploadThrottled, should_rate_limit.skip_reason);
}For these sorts of tests, I like to test each boundary and each side of each boundary. That helps avoid off-by-one errors. So, more cases for things like `now - 1`, `now + 1`, `now - kOneHour - 1`, `now - kOneHour`, etc.
Related details
- Jesse McKenna
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jesse McKenna (Gerrit)
Jesse McKenna voted and added 12 comments![]( "Open in Gerrit")
Votes added by Jesse McKenna
| Commit-Queue | +1 |
12 comments
Thanks so much for the helpful review!
`int32_t`’s probably not right for this either. You can use `time_t` for time intervals in this code.
Done
`#include <time.h>` to be able to use `time_t`.
Done
//! \brief If the crash should be skipped, the reason why. `std::nullopt`Blank line before this, so that the line above doesn’t get lost.
Done
This structure isn’t written to disk or used for interchange beyond this process, so it doesn’t need precise sizing (`int32_t`). You can use an `int` or `unsigned int`. If a negative number would be meaningless, I prefer `unsigned int`, but wouldn’t object to `int`.
Done
Won’t be needed if you take the other suggestions in this file.
Done
I don’t see this being used here.
Done
} else {
// The most recent upload attempt purportedly occurred in the future. If
// it “happened” at least `kBackwardsClockTolerance` in the future, assume
// that the last upload attempt time is bogus, and attempt to upload the
// report. If the most recent upload time is in the future but within
// `kBackwardsClockTolerance`, accept it and don’t attempt to upload the
// report.
if (last_upload_attempt_time - now < kBackwardsClockTolerance) {You can collapse this into `else if`.
Done
As elsewhere, `time_t` works for intervals of time—you can get rid of `<stdint.h>` if you make this change.
Done
EXPECT_EQ(kUploadThrottled, should_rate_limit.skip_reason);You’ve flipped the `EXPECT_EQ`s! Chromium’s a bit all over the place on this, but I recommend writing these in natural English order. In Crashpad, we consistently write them in the natural English order:
```
EXPECT_EQ(should_rate_limit.skip_reason, kUploadThrottled);
```Throughout this file.
Done
// Skip upload if the last attempt was under one interval ago.
TEST(CrashReportUploadRateLimitTest, ShouldRateLimit_AttemptWithinInterval) {
const time_t now = time(nullptr);
auto should_rate_limit = ShouldRateLimit(now, now, kOneHour);
EXPECT_EQ(kUploadThrottled, should_rate_limit.skip_reason);
should_rate_limit = ShouldRateLimit(now, now - kOneHour + 1, kOneHour);
EXPECT_EQ(kUploadThrottled, should_rate_limit.skip_reason);
}For these sorts of tests, I like to test each boundary and each side of each boundary. That helps avoid off-by-one errors. So, more cases for things like `now - 1`, `now + 1`, `now - kOneHour - 1`, `now - kOneHour`, etc.
Done - I combined the tests into one, added a few more test cases, and ordered them from newest to oldest to hopefully make it more readable.
Related details
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai voted and added 3 comments![]( "Open in Gerrit")
Votes added by Mark Mentovai
| Code-Review | +1 |
3 comments
Thanks, Jesse!
// Skip upload if the last attempt was under one interval ago.
TEST(CrashReportUploadRateLimitTest, ShouldRateLimit_AttemptWithinInterval) {
const time_t now = time(nullptr);
auto should_rate_limit = ShouldRateLimit(now, now, kOneHour);
EXPECT_EQ(kUploadThrottled, should_rate_limit.skip_reason);
should_rate_limit = ShouldRateLimit(now, now - kOneHour + 1, kOneHour);
EXPECT_EQ(kUploadThrottled, should_rate_limit.skip_reason);
}Jesse McKennaFor these sorts of tests, I like to test each boundary and each side of each boundary. That helps avoid off-by-one errors. So, more cases for things like `now - 1`, `now + 1`, `now - kOneHour - 1`, `now - kOneHour`, etc.
Done - I combined the tests into one, added a few more test cases, and ordered them from newest to oldest to hopefully make it more readable.
- Jesse McKenna
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jesse McKenna (Gerrit)
Jesse McKenna voted and added 2 comments![]( "Open in Gerrit")
Votes added by Jesse McKenna
| Commit-Queue | +2 |
2 comments
//!Jesse McKennaBlank line, not //!.
Oops, done - thanks
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Crashpad LUCI CQ (Gerrit)
Crashpad LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
7 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: handler/crash_report_upload_rate_limit.h
Insertions: 1, Deletions: 1.
@@ -34,7 +34,7 @@
struct ShouldRateLimitResult {
//! \brief The number of upload attempts in the past interval.
unsigned int attempts_in_interval;
- //!
+
//! \brief If the crash should be skipped, the reason why. `std::nullopt`
//! otherwise.
std::optional<Metrics::CrashSkippedReason> skip_reason;
```
Change information
rate-limit: create ShouldRateLimit() and tests
This change moves the rate-limiting logic from ShouldRateLimitUpload()
into a separate helper function with its own tests. The logic itself
is unchanged.
This is a precursor to changes that will experimentally change the
rate limit. The new tests will help ensure that future changes don't
unexpectedly break existing logic that should be kept (e.g., the
CrashSkippedReason, which is emitted to a histogram).
- M handler/BUILD.gn
- A handler/crash_report_upload_rate_limit.cc
- A handler/crash_report_upload_rate_limit.h
- A handler/crash_report_upload_rate_limit_test.cc
- M handler/crash_report_upload_thread.cc
Code-Review: +1 by Mark Mentovai
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |