ref(transport): Measure rate-limit backoff on a monotonic clock (JAVA-574) - #6030
ref(transport): Measure rate-limit backoff on a monotonic clock (JAVA-574)#6030runningcode wants to merge 6 commits into
Conversation
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
1 similar comment
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
📲 Install BuildsAndroid
|
00b4937 to
7eae6db
Compare
eb4c4ad to
0a21879
Compare
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
7eae6db to
e20e3fa
Compare
0a21879 to
074890f
Compare
e20e3fa to
2b35532
Compare
074890f to
781c864
Compare
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
2b35532 to
76fef57
Compare
781c864 to
8bb8665
Compare
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
76fef57 to
81e7109
Compare
8bb8665 to
fc85c12
Compare
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
81e7109 to
47bf886
Compare
fc85c12 to
be5eaeb
Compare
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
47bf886 to
c45669e
Compare
be5eaeb to
2168b35
Compare
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
0154972 to
3fb8770
Compare
da479f2 to
7de7107
Compare
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
240c770 to
4562a70
Compare
7de7107 to
62c2029
Compare
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
…-574) Retry-after limits were java.util.Date values derived from System.currentTimeMillis(). A wall clock is the wrong instrument for a backoff window: it steps when the device syncs time, so an NTP correction could lift a 60 second rate limit early or extend it by however far the clock jumped. The limits now live on the monotonic clock, which counts forward at a steady rate and keeps counting while the device sleeps, which is what a server-dictated wait means. Storing Deadline rather than a timestamp also removes the duplicated parameter on applyRetryAfterOnlyIfLonger, which took both an absolute deadline and the delay needed to reach it, and lets three JdkObsolete and JavaUtilDate suppressions go with the Dates. RateLimiter also took the whole SentryOptions while reading exactly three methods from it. It now depends on RateLimiterConfig, declared next to its consumer, so what a rate limiter touches is three lines to read rather than three hundred. SentryOptions implements it with no new methods, so every existing caller compiles unchanged. Both existing constructors stay, so the .api diff is additions only. The ICurrentDateProvider one is deprecated and adapts the injected provider rather than ignoring it, since a custom ITransportFactory may be passing one. One boundary moves by a nanosecond: a limit used to be active while `now <= deadline` and is now active while `now < deadline`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The RateLimiterConfig constructor is now private behind RateLimiter.create, and both SentryOptions constructors are deprecated. Handing over the whole options object hides which three of its methods a rate limiter reads, which is the very thing the config interface exists to name, so both transport factories call the factory method instead. RateLimiterTest supplies its own RateLimiterConfig rather than a SentryOptions carrying a @testonly executor setter and a reflective client-report-recorder injection. A DeferredExecutorService replaces the real executor, so the lifted-limit notification runs when the test says so instead of a real second later, leaving the deadline's virtual clock as the only clock in the test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
62c2029 to
258eedb
Compare
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
runningcode
added
the
sanity-check
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 258eedb. Configure here.
A server-supplied `Retry-After: -1`, or a negative delay in `X-Sentry-Rate-Limits`, reached `Deadline.after` unvalidated and threw `IllegalArgumentException` out of response handling. Clamp a negative amount to an already-passed deadline, restoring the behavior the wall-clock implementation had, and keeping the tick arithmetic away from wrapping on a large negative value. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
There was a problem hiding this comment.
So good! Just one potential blocker; otherwise looks fantastic 💯
A wall clock is bad for RateLimiting since it can jump backwards.
:poetry:
We also use a static constructor because Josh Bloch is my man. (See Effective Java item 1 "Consider static factory methods instead of constructors.")
🐐 🏅
| } | ||
|
|
||
| /** | ||
| * @deprecated use the create static constructor instead |
There was a problem hiding this comment.
nit: Could add a link to make this foolproof if you want (same below)
| /** | ||
| * @deprecated use the create static constructor instead | ||
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
(Same below)
| return true; | ||
| } | ||
| final @Nullable Deadline allCategories = sentryRetryAfterLimit.get(DataCategory.All); | ||
| if (allCategories != null && !allCategories.hasPassed()) { |
There was a problem hiding this comment.
(future aside: still thinking hasNotPassed() would be nice at some point 😄... )
| import org.jetbrains.annotations.NotNull; | ||
|
|
||
| /** | ||
| * The configuration {@link RateLimiter} reads. Declared next to its consumer rather than alongside |
There was a problem hiding this comment.
l: Consider moving a generalized version of the "Declared next..." comment into SentryOptions where we declare its implementation of these interfaces, since that'll be a pattern we want to continue (we could then remove it here).
| * <p>Implementations are expected to delegate to live configuration rather than snapshot it, so | ||
| * that a logger or executor replaced after {@code Sentry.init} is still picked up. | ||
| */ | ||
| @ApiStatus.Internal |
| public interface RateLimiterConfig { | ||
|
|
||
| @NotNull | ||
| ILogger getLogger(); |
There was a problem hiding this comment.
future aside: maybe we'll want to hoist some of these want-them-everywhere deps into their own interface at some point and let RateLimiterConfig, etc. implement that.
| /** Sentry SDK options */ | ||
| @Open | ||
| public class SentryOptions { | ||
| public class SentryOptions implements RateLimiterConfig { |
There was a problem hiding this comment.
The many prices paid for service locators... sigh. (But I like this move given that we have one!)
RateLimiter is public because ITransport.getRateLimiter returns it and a custom transport has to build one. Deprecating the options constructor pointed those callers at RateLimiter.create, whose two parameter types are both @ApiStatus.Internal, so following the deprecation meant reaching into internal API. Un-deprecate the options constructor and mark the factory internal instead, which is what it already was in practice. Both transport factories call the constructor, dropping the getMonotonicTicker/options double-pass and, in the apache module, an internal call from another module. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
…ted (JAVA-574) Both transport factories go back to RateLimiter.create, which names the collaborators a rate limiter reads instead of handing over the whole options object. The constructor stays as the one path a caller outside the SDK can take, and now says so: it is only non-deprecated because create takes the internal MonotonicTicker, and should be deprecated once that type is public API. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
0xadam-brown
left a comment
0xadam-brown
left a comment
There was a problem hiding this comment.
Thanks for the update 💯 🕐 🚀
2 participants
📜 Description
I wrote all these lines by hand in case you care to read them. :)
Wall clocks are bad
So this PR started as a way to fix the rate limiter using a wall clock. That's clearly bad™️ because a wall clock can drift which means if the clock jumps backward or forward suddenly we could send a ton of requests when the ratelimiter expires or extend the rate limiter for far longer.
I think the code is a LOT easier to read and speaks for itself.
Static constructors are good
With that change, we need to change the object's constructor because we don't pass in a
ICurrentDateProvideranymore. So the old constructors are kept (and deprecated) only to maintain backward compatibility with any possible existing customers. We should remove the constructors in v9. We also use a static constructor because Josh Bloch is my man. (See Effective Java item 1 "Consider static factory methods instead of constructors.")SentryOptions is a god object
Since we are changing the constructor, I decided it would be a good idea to NOT pass in the
SentryOptionsgod object and instead hide it behind an interface that only gives us what we need from that object. There's no functionality change but it makes it easier to test and confines the scope ourSentryOptionsusage to make future refactors easier.Another route instead of the
RateLimiterConfigAnother option would be to only pass in the objects from
SentryOptionsthat we need to the RateLimiter's constructor. I really wanted to do this. The issue is that all the fields are publicly mutable so we can't pass them in to the constructor because they can change. 😢 Mutable state is very fun /sSo TL;DR on what's changed:
💡 Motivation and Context
A wall clock is bad for RateLimiting since it can jump backwards.
💚 How did you test it?
Tests pass.
📝 Checklist
sendDefaultPIIis enabled.