Upper bound individual retries, move configurations in web config. (#6975)

Upper bound individual retries, move configurations in web config.

Author: cristinamanum

src/NuGetGallery/App_Start/DefaultDependenciesModule.cs

@@ -744,9 +744,14 @@ private static void ConfigureResilientSearch(ILoggerFactory loggerFactory, IGall
                     .AddHttpMessageHandler<TracingHttpHandler>()
                     .AddHttpMessageHandler<CorrelatingHttpClientHandler>()
                     .AddPolicyHandler(SearchClientPolicies.SearchClientFallBackCircuitBreakerPolicy(logger, searchClient.name, telemetryService))
-                    .AddPolicyHandler(SearchClientPolicies.SearchClientWaitAndRetryForeverPolicy(logger, searchClient.name, telemetryService))
+                    .AddPolicyHandler(SearchClientPolicies.SearchClientWaitAndRetryPolicy(
+                            configuration.Current.SearchCircuitBreakerWaitAndRetryCount,
+                            configuration.Current.SearchCircuitBreakerWaitAndRetryIntervalInMilliseconds,
+                            logger,
+                            searchClient.name,
+                            telemetryService))
                     .AddPolicyHandler(SearchClientPolicies.SearchClientCircuitBreakerPolicy(
-                            SearchClientConfiguration.SearchRetryCount,
+                            configuration.Current.SearchCircuitBreakerBreakAfterCount,
                             TimeSpan.FromSeconds(configuration.Current.SearchCircuitBreakerDelayInSeconds),
                             logger,
                             searchClient.name,

src/NuGetGallery/Configuration/AppConfiguration.cs

@@ -378,6 +378,20 @@ public string ExternalBrandingMessage
         /// </summary>
         public Uri SearchServiceUriSecondary { get; set; }
 
+        [DefaultValue(600)]
         public int SearchCircuitBreakerDelayInSeconds { get; set; }
+
+        // The default value was chosen to have searchRetryCount*retryInterval to be close to 1 second in order to keep the user still engaged.
+        // https://www.nngroup.com/articles/website-response-times/
+        [DefaultValue(500)]
+        public int SearchCircuitBreakerWaitAndRetryIntervalInMilliseconds { get; set; }
+
+        [DefaultValue(3)]
+        public int SearchCircuitBreakerWaitAndRetryCount { get; set; }
+
+        // Default value was chosen using the AI data.
+        // It is the average of the search request count per second during the last 90 days.
+        [DefaultValue(200)]
+        public int SearchCircuitBreakerBreakAfterCount { get; set; }
     }
 }

src/NuGetGallery/Configuration/IAppConfiguration.cs

@@ -405,5 +405,20 @@ public interface IAppConfiguration : IMessageServiceConfiguration
         /// The time in seconds for the circuit breaker delay. (The time the circuit breaker will stay in open state)
         /// </summary>
         int SearchCircuitBreakerDelayInSeconds { get; set; }
+
+        /// <summary>
+        /// The wait time in milliseconds for the WaitAndRetry policy.
+        /// </summary>
+        int SearchCircuitBreakerWaitAndRetryIntervalInMilliseconds { get; set; }
+
+        /// <summary>
+        /// A request will fail after this number of retries. In total a request will fail after this number of retries + 1.
+        /// </summary>
+        int SearchCircuitBreakerWaitAndRetryCount { get; set; }
+
+        /// <summary>
+        /// CircuitBreaker will open after this number of consecutive failed requests.
+        /// </summary>
+        int SearchCircuitBreakerBreakAfterCount { get; set; }
     }
 }

src/NuGetGallery/Infrastructure/Lucene/SearchClientConfiguration.cs

@@ -7,8 +7,5 @@ public class SearchClientConfiguration
     {
         public static string SearchPrimaryInstance = "SearchPrimary";
         public static string SearchSecondaryInstance = "SearchSecondary";
-        public static int SearchRetryCount = 3;
-        // Try to have searchRetryCount*retryInterval to be close to 1 second in order to keep the user still engaged. https://www.nngroup.com/articles/website-response-times/
-        public static int WaitAndRetryDefaultIntervalInMilliseconds = 500;
     }
 }

src/NuGetGallery/Infrastructure/Lucene/SearchClientPolicies.cs

@@ -27,7 +27,7 @@ public class SearchClientPolicies
         /// When the timeout expires trial requsts are allowed. The state will be changed to a Close state if they succeed.
         /// If they do not succeed the circuit will be moved back in the Open state for another <paramref name="breakDuration"/>. 
         /// </summary>
-        /// <param name="breakAfterCount">The number of retries until circuit will be open.</param>
+        /// <param name="breakAfterCount">The number of allowed failures until circuit will be open.</param>
         /// <param name="breakDuration">The <see cref="TimeSpan"/> for the circuit to stay open.</param>
         /// <param name="logger">An <see cref="ILogger"/> instance.</param>
         /// <returns>The policy.</returns>
@@ -58,11 +58,15 @@ public static IAsyncPolicy<HttpResponseMessage> SearchClientCircuitBreakerPolicy
 
         /// <summary>
         /// A WaitAndRetryForever policy to be used with the SearchClientCircuitBreakerPolicy. 
-        /// The policy will retry on any transient error and will not retry on <see cref="BrokenCircuitException"/> to avoid non-necessary retries due to circuit breaker exceptions.
+        /// The policy will retry for <paramref name="retryCount"/> on any transient error and will not retry on <see cref="BrokenCircuitException"/> to avoid non-necessary retries due to circuit breaker exceptions.
         /// </summary>
-        /// <param name="logger">An <see cref="ILogger"/> instance.</param>
+        /// <param name="retryCount">The allowed number of retries.</param>
+        /// <param name="waitInMilliseconds">The time to wait between retries.</param>
+        /// <param name="logger">The logger</param>
+        /// <param name="searchName">The search name.</param>
+        /// <param name="telemetryService">Telemetry service</param>
         /// <returns>The policy.</returns>
-        public static IAsyncPolicy<HttpResponseMessage> SearchClientWaitAndRetryForeverPolicy(ILogger logger, string searchName, ITelemetryService telemetryService)
+        public static IAsyncPolicy<HttpResponseMessage> SearchClientWaitAndRetryPolicy(int retryCount, int waitInMilliseconds, ILogger logger, string searchName, ITelemetryService telemetryService)
         {
             return HttpPolicyExtensions.
                          HandleTransientHttpError()
@@ -73,17 +77,18 @@ public static IAsyncPolicy<HttpResponseMessage> SearchClientWaitAndRetryForeverP
                                 // There should not be any retry in this case
                                 return false;
                             }).
-                        WaitAndRetryForeverAsync(
-                            sleepDurationProvider: (retryAttempt, context) => TimeSpan.FromMilliseconds(SearchClientConfiguration.WaitAndRetryDefaultIntervalInMilliseconds),
+                        WaitAndRetryAsync(
+                            retryCount: retryCount,
+                            sleepDurationProvider: (retryAttempt, context) => TimeSpan.FromMilliseconds(waitInMilliseconds),
                             onRetry: (delegateResult, waitDuration, context) =>
-                                    {
-                                        telemetryService.TrackMetricForSearchOnRetry(searchName,
-                                            delegateResult.Exception,
-                                            context.CorrelationId.ToString(),
-                                            GetValueFromContext(ContextKey_RequestUri, context),
-                                            GetValueFromContext(ContextKey_CircuitBreakerStatus, context));
-                                        logger.LogInformation("Policy retry - it will retry after {RetryMilliseconds} milliseconds. {Exception} {SearchName}", waitDuration.TotalMilliseconds, delegateResult.Exception, searchName);
-                                    });
+                            {
+                                telemetryService.TrackMetricForSearchOnRetry(searchName,
+                                    delegateResult.Exception,
+                                    context.CorrelationId.ToString(),
+                                    GetValueFromContext(ContextKey_RequestUri, context),
+                                    GetValueFromContext(ContextKey_CircuitBreakerStatus, context));
+                                logger.LogInformation("Policy retry - it will retry after {RetryMilliseconds} milliseconds. {Exception} {SearchName}", waitDuration.TotalMilliseconds, delegateResult.Exception, searchName);
+                            });
         }
 
         /// <summary>

src/NuGetGallery/Web.config

@@ -138,6 +138,9 @@
     <add key="Gallery.SearchServiceUriPrimary" value=""/>
     <add key="Gallery.SearchServiceUriSecondary" value=""/>
     <add key="Gallery.SearchCircuitBreakerDelayInSeconds" value="600"/>
+    <add key="Gallery.SearchCircuitBreakerWaitAndRetryIntervalInMilliseconds" value="500"/>
+    <add key="Gallery.SearchCircuitBreakerWaitAndRetryCount" value="3"/>
+    <add key="Gallery.SearchCircuitBreakerBreakAfterCount" value="200"/>
     <add key="Gallery.Brand" value="NuGet Gallery"/>
     <add key="Gallery.GalleryOwner" value="NuGet Gallery &lt;[email protected]&gt;"/>
     <add key="Gallery.GalleryNoReplyAddress" value="NuGet Gallery &lt;[email protected]&gt;"/>

tests/NuGetGallery.Facts/Infrastructure/Lucene/SearchPolicyFacts.cs

@@ -17,7 +17,12 @@ namespace NuGetGallery.Infrastructure.Search
 {
     public class SearchPolicyFacts
     {
-        private static int _retryCount = 2;
+        private static int _waitBetweenRetriesInMilliseconds = 500;
+        private static int _circuitBreakerFailAfter = 2;
+        private static int _retryCount = 10;
+        // set the circuit breker to be larger than the retryCount
+        private static int _circuitBreakerFailAfter_2 = 10;
+        private static int _retryCount_2 = 2;
         private static int _circuitBreakerLongDelaySeconds = 600;
         private static int _circuitBreakerShortDelaySeconds = 1;
         private static ServiceCollection _services = null;
@@ -26,11 +31,14 @@ public class SearchPolicyFacts
         private static LoggerFor_TestSearchHttpClient _loggerFor_InvalidTestSearchHttpClientWithShortCircuitBreakDelay;
         private static LoggerFor_TestSearchHttpClient _loggerFor_ValidTestSearchHttpClient;
         private static string _nameFor_InvalidTestSearchHttpClientWithLongCircuitBreakDelay = "InvalidTestSearchHttpClientWithLongCircuitBreakDelay";
-        private static string _nameFor_InvalidTestSearchHttpClientWithShortCircuitBreakDelay = "InvalidTestSearchHttpClientWithSWhortCircuitBreakDelay";
+        private static string _nameFor_InvalidTestSearchHttpClientWithShortCircuitBreakDelay = "InvalidTestSearchHttpClientWithShortCircuitBreakDelay";
         private static string _nameFor_ValidTestSearchHttpClient = "ValidTestSearchHttpClient";
         private static readonly string _longInvalidAddress = "https://api-v2v3search-long.nuget.org";
         private static readonly string _shortInvalidAddress = "https://api-v2v3search-short.nuget.org";
         private static readonly string _validAddress = "https://api-v2v3search-0.nuget.org";
+        private static readonly string _shortInvalidAddress_2 = "https://api-v2v3search-short-2.nuget.org";
+        private static string _nameFor_InvalidTestSearchHttpClientRetryCountExpires = "InvalidTestSearchHttpClientRetryCountExpires";
+        private static LoggerFor_TestSearchHttpClient _loggerFor_InvalidTestSearchHttpClientRetryCountExpires;
 
         public SearchPolicyFacts()
         {
@@ -163,6 +171,28 @@ public async Task TestCircuitBreakerForValidRequests()
             Assert.Equal(HttpStatusCode.OK, r.StatusCode);
         }
 
+        [Fact]
+        public async Task TestWaitAndRetryForInvalidRequests()
+        {
+            var invalidClient = _services.BuildServiceProvider().GetServices<TestSearchHttpClient>().Where(s => s.BaseAddress == new Uri(_shortInvalidAddress_2)).ElementAt(0);
+            var uri = new Uri($"{invalidClient.BaseAddress}query?q=packageid:Newtonsoft.Json version:12.0.1");
+
+            var r = await invalidClient.GetAsync(uri);
+
+            var retryInfo = _loggerFor_InvalidTestSearchHttpClientRetryCountExpires.Informations.Where(s => s.StartsWith("Policy retry - it will retry after")).Count();
+            var onCircuitBreakerfallBackInfo = _loggerFor_InvalidTestSearchHttpClientRetryCountExpires.Informations.Where(s => s.StartsWith("On circuit breaker fallback.")).Count();
+            var circuitBreakerWarning = _loggerFor_InvalidTestSearchHttpClientRetryCountExpires.Warnings.Where(s => s.StartsWith("SearchCircuitBreaker logging: Breaking the circuit for")).Count();
+            var onCircuitBreakerReset = _loggerFor_InvalidTestSearchHttpClientRetryCountExpires.Informations.Where(s => s.StartsWith("SearchCircuitBreaker logging: Call ok! Closed the circuit again!")).Count();
+            var onCircuitBreakerHalfOpen = _loggerFor_InvalidTestSearchHttpClientRetryCountExpires.Informations.Where(s => s.StartsWith("SearchCircuitBreaker logging: Half-open: Next call is a trial!")).Count();
+
+            Assert.Equal(_retryCount_2, retryInfo);
+            Assert.Equal(1, onCircuitBreakerfallBackInfo);
+            Assert.Equal(0, circuitBreakerWarning);
+            Assert.Equal(0, onCircuitBreakerReset);
+            Assert.Equal(0, onCircuitBreakerHalfOpen);
+            Assert.Equal(HttpStatusCode.ServiceUnavailable, r.StatusCode);
+        }
+
         private static ILogger<ResilientSearchHttpClient> GetLogger()
         {
             var mockConfiguration = new Mock<ILogger<ResilientSearchHttpClient>>();
@@ -178,34 +208,44 @@ private static ServiceCollection ConfigureServices()
             _loggerFor_InvalidTestSearchHttpClientWithLongCircuitBreakDelay = new LoggerFor_TestSearchHttpClient();
             _loggerFor_InvalidTestSearchHttpClientWithShortCircuitBreakDelay = new LoggerFor_TestSearchHttpClient();
             _loggerFor_ValidTestSearchHttpClient = new LoggerFor_TestSearchHttpClient();
+            _loggerFor_InvalidTestSearchHttpClientRetryCountExpires = new LoggerFor_TestSearchHttpClient();
 
             services.AddHttpClient<TestSearchHttpClient>(_nameFor_InvalidTestSearchHttpClientWithLongCircuitBreakDelay, c => c.BaseAddress = new Uri(_longInvalidAddress))
                     .AddPolicyHandler(SearchClientPolicies.SearchClientFallBackCircuitBreakerPolicy(_loggerFor_InvalidTestSearchHttpClientWithLongCircuitBreakDelay, "InvalidTestSearchHttpClientWithLongCircuitBreakDelay", telemetryServiceResolver))
-                    .AddPolicyHandler(SearchClientPolicies.SearchClientWaitAndRetryForeverPolicy(_loggerFor_InvalidTestSearchHttpClientWithLongCircuitBreakDelay, "InvalidTestSearchHttpClientWithLongCircuitBreakDelay", telemetryServiceResolver))
+                    .AddPolicyHandler(SearchClientPolicies.SearchClientWaitAndRetryPolicy(_retryCount, _waitBetweenRetriesInMilliseconds, _loggerFor_InvalidTestSearchHttpClientWithLongCircuitBreakDelay, "InvalidTestSearchHttpClientWithLongCircuitBreakDelay", telemetryServiceResolver))
                     .AddPolicyHandler(SearchClientPolicies.SearchClientCircuitBreakerPolicy(
-                            _retryCount,
+                            _circuitBreakerFailAfter,
                             TimeSpan.FromSeconds(_circuitBreakerLongDelaySeconds),
                             _loggerFor_InvalidTestSearchHttpClientWithLongCircuitBreakDelay,
                             "InvalidTestSearchHttpClientWithLongCircuitBreakDelay", telemetryServiceResolver));
 
             services.AddHttpClient<TestSearchHttpClient>(_nameFor_InvalidTestSearchHttpClientWithShortCircuitBreakDelay, c => c.BaseAddress = new Uri(_shortInvalidAddress))
                     .AddPolicyHandler(SearchClientPolicies.SearchClientFallBackCircuitBreakerPolicy(_loggerFor_InvalidTestSearchHttpClientWithShortCircuitBreakDelay, "InvalidTestSearchHttpClientWithShortCircuitBreakDelay", telemetryServiceResolver))
-                    .AddPolicyHandler(SearchClientPolicies.SearchClientWaitAndRetryForeverPolicy(_loggerFor_InvalidTestSearchHttpClientWithShortCircuitBreakDelay, "InvalidTestSearchHttpClientWithShortCircuitBreakDelay", telemetryServiceResolver))
+                    .AddPolicyHandler(SearchClientPolicies.SearchClientWaitAndRetryPolicy(_retryCount, _waitBetweenRetriesInMilliseconds, _loggerFor_InvalidTestSearchHttpClientWithShortCircuitBreakDelay, "InvalidTestSearchHttpClientWithShortCircuitBreakDelay", telemetryServiceResolver))
                     .AddPolicyHandler(SearchClientPolicies.SearchClientCircuitBreakerPolicy(
-                            _retryCount,
+                            _circuitBreakerFailAfter,
                             TimeSpan.FromSeconds(_circuitBreakerShortDelaySeconds),
                             _loggerFor_InvalidTestSearchHttpClientWithShortCircuitBreakDelay,
                             "InvalidTestSearchHttpClientWithShortCircuitBreakDelay", telemetryServiceResolver));
 
             services.AddHttpClient<TestSearchHttpClient>(_nameFor_ValidTestSearchHttpClient, c => c.BaseAddress = new Uri(_validAddress))
-                   .AddPolicyHandler(SearchClientPolicies.SearchClientFallBackCircuitBreakerPolicy(_loggerFor_ValidTestSearchHttpClient, "ValidTestSearchHttpClient", telemetryServiceResolver))
-                   .AddPolicyHandler(SearchClientPolicies.SearchClientWaitAndRetryForeverPolicy(_loggerFor_ValidTestSearchHttpClient, "InvalidTestSearchHttpClientWithShortCircuitBreakDelay", telemetryServiceResolver))
+                   .AddPolicyHandler(SearchClientPolicies.SearchClientFallBackCircuitBreakerPolicy(_loggerFor_ValidTestSearchHttpClient, "InvalidTestSearchHttpClientWithShortCircuitBreakDelay", telemetryServiceResolver))
+                   .AddPolicyHandler(SearchClientPolicies.SearchClientWaitAndRetryPolicy(_retryCount, _waitBetweenRetriesInMilliseconds, _loggerFor_ValidTestSearchHttpClient, "InvalidTestSearchHttpClientWithShortCircuitBreakDelay", telemetryServiceResolver))
                    .AddPolicyHandler(SearchClientPolicies.SearchClientCircuitBreakerPolicy(
-                           _retryCount,
+                           _circuitBreakerFailAfter,
                            TimeSpan.FromSeconds(_circuitBreakerShortDelaySeconds),
                            _loggerFor_ValidTestSearchHttpClient,
                            "InvalidTestSearchHttpClientWithShortCircuitBreakDelay", telemetryServiceResolver));
 
+            services.AddHttpClient<TestSearchHttpClient>(_nameFor_InvalidTestSearchHttpClientRetryCountExpires, c => c.BaseAddress = new Uri(_shortInvalidAddress_2))
+                   .AddPolicyHandler(SearchClientPolicies.SearchClientFallBackCircuitBreakerPolicy(_loggerFor_InvalidTestSearchHttpClientRetryCountExpires, "InvalidTestSearchHttpClientRetryCountExpires", telemetryServiceResolver))
+                   .AddPolicyHandler(SearchClientPolicies.SearchClientWaitAndRetryPolicy(_retryCount_2, _waitBetweenRetriesInMilliseconds, _loggerFor_InvalidTestSearchHttpClientRetryCountExpires, "InvalidTestSearchHttpClientRetryCountExpires", telemetryServiceResolver))
+                   .AddPolicyHandler(SearchClientPolicies.SearchClientCircuitBreakerPolicy(
+                           _circuitBreakerFailAfter_2,
+                           TimeSpan.FromSeconds(_circuitBreakerShortDelaySeconds),
+                           _loggerFor_InvalidTestSearchHttpClientRetryCountExpires,
+                           "InvalidTestSearchHttpClientRetryCountExpires", telemetryServiceResolver));
+
             return services;
         }