feat: Use recommended retries for token and IAM sign blob endpoints. #2879

amanda-tarafa · 2024-11-13T08:18:09Z

Closes b/332955964 and b/368412308

@jskeet , as always, one commit at a time.

jskeet

I've reviewed the first 6 commits - the comment on the 7th doesn't indicate that I've reviewed much of it yet! I think it's fine to add a single commit to fix typos etc - no need to move the change into the relevant original commit. Will try to get to the rest of the PR this time tomorrow.

jskeet · 2024-11-13T12:25:32Z

Src/Support/Google.Apis.Core/Util/ExponentialBackOff.cs

@@ -28,48 +28,45 @@ public class ExponentialBackOff : IBackOff
        /// <summary>The maximum allowed number of retries.</summary>
        private const int MaxAllowedNumRetries = 20;

-        private readonly TimeSpan deltaBackOff;
+        /// <summary>The random instance which generates a random number to add the to next back-off.</summary>
+        private static readonly Random Random = new Random();


Random isn't thread-safe - at least in some implementations. (I think it may be in .NET 8+but that's only a vague memory.)

We should probably lock on every use of it.

(I've just noticed that this isn't actually introducing a new Random, just moving it - but the comment still stands... we should probably fix that in a separate PR.)

Done, I added a commit at the end.

Starting on .NET 6 there's Random.Shared, but we can't use that at the moment.

jskeet · 2024-11-13T12:27:39Z

Src/Support/Google.Apis.Core/Util/ExponentialBackOff.cs

+        /// <summary>
+        /// Percent of the current unjittered back-off used to bound the jitter.
+        /// If the value is set to 10, and the current back-off is 1000ms, then the jitter will be generated
+        /// from [-100, 100]ms and the jittered back-off will be [900,1100]ms.


I'd change this to "-100ms to 100ms" or similar, to avoid anything thinking it may be markdown. (Ditto the later part.)

Yep, done here and in all other places in this file.

jskeet · 2024-11-13T12:29:15Z

Src/Support/Google.Apis.Core/Util/ExponentialBackOff.cs

+        /// Constructs a new exponential back-off with the given delta percent and maximum retries.
+        /// If unspecified, <paramref name="maximumNumOfRetries"/> is set to 10.
+        /// </summary>
+        public ExponentialBackOff(short deltaBackOffPercent, int maximumNumOfRetries = 10)


I'd be tempted to make this a factory method with a private constructor - something taking "two integers" could end up getting confusing if we add more overloads later.

Yes, good idea.

jskeet · 2024-11-13T12:30:36Z

Src/Support/Google.Apis.Core/Util/ExponentialBackOff.cs

-                (int)(DeltaBackOff.TotalMilliseconds * 1));
-            int backOffMilli = (int)(Math.Pow(2.0, (double)currentRetry - 1) * 1000 + randomMilli);
-            return TimeSpan.FromMilliseconds(backOffMilli);
+            double unjitterdBackOffMs = Math.Pow(2.0, currentRetry - 1) * 1000;


Nit: unjitterd => unjittered (or possibly "raw"?)

Raw seems better, done.

jskeet · 2024-11-13T12:31:29Z

Src/Support/Google.Apis.Core/Util/ExponentialBackOff.cs

+        private int GetPercentBoundedJitter(double unjitterdBackOffMs) =>
+            DeltaBackOffPercent == 0 ?
+            0 :
+            GetRandomBoundedValue((int)(DeltaBackOffPercent * unjitterdBackOffMs)/100);


Nit: space before and after the /

jskeet · 2024-11-13T12:38:04Z

Src/Support/Google.Apis.Core/Http/ExponentialBackOffInitializer.cs

@@ -50,6 +57,8 @@ public class ExponentialBackOffInitializer : IConfigurableHttpClientInitializer

        /// <summary>
        /// Constructs a new back-off initializer with the given policy and back-off handler create function.
+        /// If <paramref name="policy"/> has the <see cref="ExponentialBackOffPolicy.RecommendedOrDefault"/> flag set,
+        /// the <see cref="BackOffHandler"/> will be set for handling both excetions and HTTP Status codes.


excetions => exceptions

jskeet · 2024-11-13T12:42:20Z

Src/Support/Google.Apis.Auth/OAuth2/GoogleAuthConsts.cs

+        /// <summary>
+        /// Retry strategy recommended for the OAuth2 token endpoints.
+        /// </summary>
+        internal static readonly ExponentialBackOffInitializer OAuth2TokenEndpointRecommendedRetry = new ExponentialBackOffInitializer(


Suggest making this a property instead of an internal field. (Just by adding { get; })

jskeet · 2024-11-13T12:42:39Z

Src/Support/Google.Apis.Auth/OAuth2/GoogleAuthConsts.cs

+                HandleExceptionFunc = ex => false,
+                HandleUnsuccessfulResponseFunc = response =>
+                    response.StatusCode == HttpStatusCode.RequestTimeout || // 408
+                    (int) response.StatusCode == 429 || // 429 Enum value not availbale in .NET Standard 2.0 or .NET Framework 4.6.2


availbale => available

And add a comment saying what the enum value would be - I don't know what 429 means offhand.

jskeet · 2024-11-13T12:47:33Z

Src/Support/Google.Apis.Auth/OAuth2/GoogleAuthConsts.cs

+        /// <see cref="ExponentialBackOffPolicy.UnsuccessfulResponse503"/> flags. Otherwise, the returned value will be <paramref name="originalPolicy"/>.
+        /// This method facilitate honoring any custom policies that may have been set. Calling code is resposible for honoring the recommended policies.
+        /// </summary>
+        internal static ExponentialBackOffPolicy StripOAuth2TokenEndpointRecommendedPolicy(ExponentialBackOffPolicy originalPolicy) =>


I'm very glad this is internal, as it feels a bit odd to me. I'm not quite sure what the purpose is yet, but I suspect I'll understand later.

We just don't have enums for configuring retries, they are also Flags.

The recommended OAuth Token endpoint retry includes retrying on 503, but not on exceptions. So, if the original enum value has RecommendedOrDefault set we don't need to add a specific policy for 503. We only keep here what's not recommended, in that way, calling code know whether it needs to add a policy different than recommended.
Calling code also needs to check if it needs to add the recommended policy or not.

Happy to chat about this though. I had several different approaches for this, but ended up preferring this one.

jskeet · 2024-11-13T12:48:18Z

Src/Support/Google.Apis.Auth.Tests/OAuth2/ServiceAccountCredentialTests.cs

@@ -40,6 +39,97 @@ public class ServiceAccountCredentialTests
        private static readonly TimeSpan JwtLifetime = TimeSpan.FromMinutes(60);
        private static readonly DateTime UnixEpoch = new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc);

+        [Fact]
+        public void Deafault_RecommendedRetryPolicy()


Deafault => Default

jskeet

Okay, I think I understand all of this now. I'm sure we'd have represented backoffs in a different (non-enum-based) way if we got to redesign it from scratch, but this seems fine as a way of fitting in with that design :)

jskeet · 2024-11-13T14:28:28Z

Src/Support/Google.Apis.Auth.Tests/GoogleAuthConstsTests.cs

@@ -81,4 +81,59 @@ public void StripOAuth2TokenEndpointRecommendedPolicy_Strips(ExponentialBackOffP
        var stripped = GoogleAuthConsts.StripOAuth2TokenEndpointRecommendedPolicy(original);
        Assert.Equal(expected, stripped);
    }
+
+    [Fact]
+    public void IamSingBlobTokenEndpointRecommendedRetry()


Sing => Sign

Fix an exception message.

amanda-tarafa

All comments addressed in sorted review commits (it was not hard this time to do that).
Plus, and extra commit at the end to lock on random.

Thanks!

amanda-tarafa · 2024-11-13T19:27:34Z

Src/Support/Google.Apis.Core/Util/ExponentialBackOff.cs

@@ -28,48 +28,45 @@ public class ExponentialBackOff : IBackOff
        /// <summary>The maximum allowed number of retries.</summary>
        private const int MaxAllowedNumRetries = 20;

-        private readonly TimeSpan deltaBackOff;
+        /// <summary>The random instance which generates a random number to add the to next back-off.</summary>
+        private static readonly Random Random = new Random();


Done, I added a commit at the end.

Starting on .NET 6 there's Random.Shared, but we can't use that at the moment.

amanda-tarafa · 2024-11-13T19:36:53Z

Src/Support/Google.Apis.Core/Util/ExponentialBackOff.cs

+        /// <summary>
+        /// Percent of the current unjittered back-off used to bound the jitter.
+        /// If the value is set to 10, and the current back-off is 1000ms, then the jitter will be generated
+        /// from [-100, 100]ms and the jittered back-off will be [900,1100]ms.


Yep, done here and in all other places in this file.

amanda-tarafa · 2024-11-13T19:37:21Z

Src/Support/Google.Apis.Core/Util/ExponentialBackOff.cs

+        /// Constructs a new exponential back-off with the given delta percent and maximum retries.
+        /// If unspecified, <paramref name="maximumNumOfRetries"/> is set to 10.
+        /// </summary>
+        public ExponentialBackOff(short deltaBackOffPercent, int maximumNumOfRetries = 10)


Yes, good idea.

amanda-tarafa · 2024-11-13T19:42:50Z

Src/Support/Google.Apis.Core/Util/ExponentialBackOff.cs

-                (int)(DeltaBackOff.TotalMilliseconds * 1));
-            int backOffMilli = (int)(Math.Pow(2.0, (double)currentRetry - 1) * 1000 + randomMilli);
-            return TimeSpan.FromMilliseconds(backOffMilli);
+            double unjitterdBackOffMs = Math.Pow(2.0, currentRetry - 1) * 1000;


Raw seems better, done.

amanda-tarafa · 2024-11-13T19:44:23Z

Src/Support/Google.Apis.Core/Util/ExponentialBackOff.cs

+        private int GetPercentBoundedJitter(double unjitterdBackOffMs) =>
+            DeltaBackOffPercent == 0 ?
+            0 :
+            GetRandomBoundedValue((int)(DeltaBackOffPercent * unjitterdBackOffMs)/100);


amanda-tarafa · 2024-11-13T20:00:46Z

Src/Support/Google.Apis.Auth/OAuth2/GoogleAuthConsts.cs

+        /// <summary>
+        /// Retry strategy recommended for the OAuth2 token endpoints.
+        /// </summary>
+        internal static readonly ExponentialBackOffInitializer OAuth2TokenEndpointRecommendedRetry = new ExponentialBackOffInitializer(


amanda-tarafa · 2024-11-13T20:03:11Z

Src/Support/Google.Apis.Auth/OAuth2/GoogleAuthConsts.cs

+                HandleExceptionFunc = ex => false,
+                HandleUnsuccessfulResponseFunc = response =>
+                    response.StatusCode == HttpStatusCode.RequestTimeout || // 408
+                    (int) response.StatusCode == 429 || // 429 Enum value not availbale in .NET Standard 2.0 or .NET Framework 4.6.2


amanda-tarafa · 2024-11-13T20:31:32Z

Src/Support/Google.Apis.Auth/OAuth2/GoogleAuthConsts.cs

+        /// <see cref="ExponentialBackOffPolicy.UnsuccessfulResponse503"/> flags. Otherwise, the returned value will be <paramref name="originalPolicy"/>.
+        /// This method facilitate honoring any custom policies that may have been set. Calling code is resposible for honoring the recommended policies.
+        /// </summary>
+        internal static ExponentialBackOffPolicy StripOAuth2TokenEndpointRecommendedPolicy(ExponentialBackOffPolicy originalPolicy) =>


We just don't have enums for configuring retries, they are also Flags.

The recommended OAuth Token endpoint retry includes retrying on 503, but not on exceptions. So, if the original enum value has RecommendedOrDefault set we don't need to add a specific policy for 503. We only keep here what's not recommended, in that way, calling code know whether it needs to add a policy different than recommended.
Calling code also needs to check if it needs to add the recommended policy or not.

Happy to chat about this though. I had several different approaches for this, but ended up preferring this one.

amanda-tarafa · 2024-11-13T20:33:37Z

Src/Support/Google.Apis.Auth.Tests/OAuth2/ServiceAccountCredentialTests.cs

@@ -40,6 +39,97 @@ public class ServiceAccountCredentialTests
        private static readonly TimeSpan JwtLifetime = TimeSpan.FromMinutes(60);
        private static readonly DateTime UnixEpoch = new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc);

+        [Fact]
+        public void Deafault_RecommendedRetryPolicy()


amanda-tarafa · 2024-11-13T20:36:33Z

Src/Support/Google.Apis.Auth.Tests/GoogleAuthConstsTests.cs

@@ -81,4 +81,59 @@ public void StripOAuth2TokenEndpointRecommendedPolicy_Strips(ExponentialBackOffP
        var stripped = GoogleAuthConsts.StripOAuth2TokenEndpointRecommendedPolicy(original);
        Assert.Equal(expected, stripped);
    }
+
+    [Fact]
+    public void IamSingBlobTokenEndpointRecommendedRetry()


jskeet

All good!

amanda-tarafa · 2024-11-14T06:27:43Z

Thanks, rebasing the review commits and merging after.

Towards b/332955964 and b/368412308

Actual recommended policies and their usage will be added in upcoming commits. Towards b/332955964 and b/368412308

Towards b/332955964

…oints. Towards b/332955964

Towards b/368412308

…point. Towards b/368412308

…b endpoint. Towards b/368412308

amanda-tarafa requested a review from jskeet November 13, 2024 08:18

amanda-tarafa requested a review from a team as a code owner November 13, 2024 08:18

jskeet reviewed Nov 13, 2024

View reviewed changes

jskeet approved these changes Nov 13, 2024

View reviewed changes

refactor: Minor refactors to Google.Apis.Util.ExponentialBackoff

64bc95f

Fix an exception message.

amanda-tarafa commented Nov 13, 2024

View reviewed changes

amanda-tarafa force-pushed the retries branch from ad82ce2 to abc3758 Compare November 13, 2024 20:43

jskeet approved these changes Nov 14, 2024

View reviewed changes

amanda-tarafa added 10 commits November 13, 2024 22:28

feat: Support exponential backoff with percent bound jitter.

ee9d5c3

Towards b/332955964 and b/368412308

refactor: Move from readonly fields to readonly properties

d9e1d19

feat: Allow choosing a recommended retry policy.

8b74a9c

Actual recommended policies and their usage will be added in upcoming commits. Towards b/332955964 and b/368412308

feat: Define recommended retry policy for OAuth2 Token endpoint.

09270d0

Towards b/332955964

feat: AuthorizationCodeFlow uses recommended retry for OAuth2 endpoints.

4f44029

Towards b/332955964

feat: ServiceAccountCredential uses recommended retry for OAuth2 endp…

bf19539

…oints. Towards b/332955964

feat: Define recommended retry policy for IAM SignBlob endpoint.

ef9e892

Towards b/368412308

feat: ComputeCredential uses recommended retries for IAM SignBlob end…

043d8a3

…point. Towards b/368412308

feat: ImpersonatedCredential uses recommended retries for IAM SignBlo…

3e0b9aa

…b endpoint. Towards b/368412308

fix: Lock when using System.Random in ExponentialBackOff.

a5d793a

amanda-tarafa force-pushed the retries branch from abc3758 to a5d793a Compare November 14, 2024 06:30

amanda-tarafa merged commit 3d5804a into googleapis:main Nov 14, 2024
4 checks passed

amanda-tarafa deleted the retries branch November 14, 2024 06:32

feat: Use recommended retries for token and IAM sign blob endpoints. #2879

feat: Use recommended retries for token and IAM sign blob endpoints. #2879

Conversation

amanda-tarafa commented Nov 13, 2024

jskeet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskeet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanda-tarafa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskeet left a comment

Choose a reason for hiding this comment

amanda-tarafa commented Nov 14, 2024