#1783 Less logs for circuit breakers (Polly exceptions) (#1786)

* #1783 More accurate logs for circuit breakers (and other "polly" exceptions)
Remove try/catch in PollyPoliciesDelegatingHandler and add a more generic AddPolly<T> to be able to use a specific PollyQoSProvider

* fix should_be_invalid_re_route_using_downstream_http_version UT

* fix remarks on PR

* arrange code

* fix UT

* merge with release/net8 branch

* switch benchmark to Net8

* Fix warnings

* Final review

---------

Co-authored-by: Ray <rmessie@traceparts.com>
Co-authored-by: raman-m <dotnet044@gmail.com>

src/Ocelot.Provider.Polly/OcelotBuilderExtensions.cs

@@ -13,6 +13,19 @@ namespace Ocelot.Provider.Polly;
 
 public static class OcelotBuilderExtensions
 {
+    public static IOcelotBuilder AddPolly<T>(this IOcelotBuilder builder,
+        QosDelegatingHandlerDelegate delegatingHandler,
+        Dictionary<Type, Func<Exception, Error>> errorMapping)
+        where T : class, IPollyQoSProvider<HttpResponseMessage>
+    {
+        builder.Services
+            .AddSingleton(errorMapping)
+            .AddSingleton<IPollyQoSProvider<HttpResponseMessage>, T>()
+            .AddSingleton(delegatingHandler);
+
+        return builder;
+    }
+
     public static IOcelotBuilder AddPolly(this IOcelotBuilder builder)
     {
         var errorMapping = new Dictionary<Type, Func<Exception, Error>>
@@ -22,14 +35,9 @@ public static IOcelotBuilder AddPolly(this IOcelotBuilder builder)
             { typeof(BrokenCircuitException), e => new RequestTimedOutError(e) },
             { typeof(BrokenCircuitException<HttpResponseMessage>), e => new RequestTimedOutError(e) },
         };
-
-        builder.Services
-            .AddSingleton(errorMapping)
-            .AddSingleton<IPollyQoSProvider<HttpResponseMessage>, PollyQoSProvider>()
-            .AddSingleton<QosDelegatingHandlerDelegate>(GetDelegatingHandler);
-        return builder;
+        return AddPolly<PollyQoSProvider>(builder, GetDelegatingHandler, errorMapping);
     }
 
-    private static DelegatingHandler GetDelegatingHandler(DownstreamRoute route, IHttpContextAccessor contextAccessor, IOcelotLoggerFactory loggerFactory) 
+    private static DelegatingHandler GetDelegatingHandler(DownstreamRoute route, IHttpContextAccessor contextAccessor, IOcelotLoggerFactory loggerFactory)
         => new PollyPoliciesDelegatingHandler(route, contextAccessor, loggerFactory);
 }

src/Ocelot.Provider.Polly/PollyPoliciesDelegatingHandler.cs

@@ -30,27 +30,23 @@ private IPollyQoSProvider<HttpResponseMessage> GetQoSProvider()
         return _contextAccessor.HttpContext.RequestServices.GetService<IPollyQoSProvider<HttpResponseMessage>>();
     }
 
-    protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request,
-        CancellationToken cancellationToken)
+    /// <summary>
+    /// Sends an HTTP request to the inner handler to send to the server as an asynchronous operation.
+    /// </summary>
+    /// <param name="request">Downstream request.</param>
+    /// <param name="cancellationToken">Token to cancel the task.</param>
+    /// <returns>A <see cref="Task{HttpResponseMessage}"/> object of a <see cref="HttpResponseMessage"/> result.</returns>
+    /// <exception cref="BrokenCircuitException">Exception thrown when a circuit is broken.</exception>
+    /// <exception cref="HttpRequestException">Exception thrown by <see cref="HttpClient"/> and <see cref="HttpMessageHandler"/> classes.</exception>
+    protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
     {
         var qoSProvider = GetQoSProvider();
-        try
-        {
-            // at least one policy (timeout) will be returned
-            // AsyncPollyPolicy can't be null
-            // AsyncPollyPolicy constructor will throw if no policy is provided
-            var policy = qoSProvider.GetPollyPolicyWrapper(_route).AsyncPollyPolicy;
-            return await policy.ExecuteAsync(async () => await base.SendAsync(request, cancellationToken));
-        }
-        catch (BrokenCircuitException ex)
-        {
-            _logger.LogError("Reached to allowed number of exceptions. Circuit is open", ex);
-            throw;
-        }
-        catch (HttpRequestException ex)
-        {
-            _logger.LogError($"Error in {nameof(PollyPoliciesDelegatingHandler)}.{nameof(SendAsync)}", ex);
-            throw;
-        }
+
+        // At least one policy (timeout) will be returned
+        // AsyncPollyPolicy can't be null
+        // AsyncPollyPolicy constructor will throw if no policy is provided
+        var policy = qoSProvider.GetPollyPolicyWrapper(_route).AsyncPollyPolicy;
+
+        return await policy.ExecuteAsync(async () => await base.SendAsync(request, cancellationToken));
     }
 }

src/Ocelot.Provider.Polly/PollyPolicyWrapper.cs

@@ -11,12 +11,10 @@ public class PollyPolicyWrapper<TResult>
     public PollyPolicyWrapper(params IAsyncPolicy<TResult>[] policies)
     {
         var allPolicies = policies.Where(p => p != null).ToArray();
-        AsyncPollyPolicy = allPolicies.First();
 
-        if (allPolicies.Length > 1)
-        {
-            AsyncPollyPolicy = Policy.WrapAsync(allPolicies);
-        }
+        AsyncPollyPolicy = allPolicies.Length > 1 ? 
+            Policy.WrapAsync(allPolicies) : 
+            allPolicies[0];
     }
 
     public IAsyncPolicy<TResult> AsyncPollyPolicy { get; }

test/Ocelot.AcceptanceTests/PollyQoSTests.cs

@@ -7,34 +7,34 @@ namespace Ocelot.AcceptanceTests
     public class PollyQoSTests : IDisposable
     {
         private readonly Steps _steps;
-        private readonly ServiceHandler _serviceHandler;
-
+        private readonly ServiceHandler _serviceHandler;
+
         public PollyQoSTests()
         {
             _serviceHandler = new ServiceHandler();
             _steps = new Steps();
         }
-
-        private static FileConfiguration FileConfigurationFactory(int port, QoSOptions options, string httpMethod = nameof(HttpMethods.Get))
-            => new()
+
+        private static FileConfiguration FileConfigurationFactory(int port, QoSOptions options,
+            string httpMethod = nameof(HttpMethods.Get)) => new()
+        {
+            Routes = new List<FileRoute>
             {
-                Routes = new List<FileRoute>
+                new()
                 {
-                    new()
+                    DownstreamPathTemplate = "/",
+                    DownstreamScheme = Uri.UriSchemeHttp,
+                    DownstreamHostAndPorts = new()
                     {
-                        DownstreamPathTemplate = "/",
-                        DownstreamScheme = Uri.UriSchemeHttp,
-                        DownstreamHostAndPorts = new()
-                        {
-                            new("localhost", port),
-                        },
-                        UpstreamPathTemplate = "/",
-                        UpstreamHttpMethod = new() { httpMethod },
-                        QoSOptions = new FileQoSOptions(options),
+                        new("localhost", port),
                     },
+                    UpstreamPathTemplate = "/",
+                    UpstreamHttpMethod = new() {httpMethod},
+                    QoSOptions = new FileQoSOptions(options),
                 },
-            };
-
+            },
+        };
+
         [Fact]
         public void Should_not_timeout()
         {
@@ -49,13 +49,13 @@ public void Should_not_timeout()
                 .Then(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.OK))
                 .BDDfy();
         }
-
+
         [Fact]
         public void Should_timeout()
         {
             var port = PortFinder.GetRandomPort();
             var configuration = FileConfigurationFactory(port, new QoSOptions(0, 0, 10, null), HttpMethods.Post);
-
+
             this.Given(x => x.GivenThereIsAServiceRunningOn($"http://localhost:{port}", 201, string.Empty, 1000))
                 .And(x => _steps.GivenThereIsAConfiguration(configuration))
                 .And(x => _steps.GivenOcelotIsRunningWithPolly())
@@ -64,13 +64,13 @@ public void Should_timeout()
                 .Then(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.ServiceUnavailable))
                 .BDDfy();
         }
-
+
         [Fact]
         public void Should_open_circuit_breaker_after_two_exceptions()
         {
             var port = PortFinder.GetRandomPort();
             var configuration = FileConfigurationFactory(port, new QoSOptions(2, 5000, 100000, null));
-
+
             this.Given(x => x.GivenThereIsABrokenServiceRunningOn($"http://localhost:{port}"))
                 .And(x => _steps.GivenThereIsAConfiguration(configuration))
                 .And(x => _steps.GivenOcelotIsRunningWithPolly())
@@ -86,7 +86,7 @@ public void Should_open_circuit_breaker_then_close()
         {
             var port = PortFinder.GetRandomPort();
             var configuration = FileConfigurationFactory(port, new QoSOptions(1, 500, 1000, null));
-
+
             this.Given(x => x.GivenThereIsAPossiblyBrokenServiceRunningOn($"http://localhost:{port}", "Hello from Laura"))
                 .Given(x => _steps.GivenThereIsAConfiguration(configuration))
                 .Given(x => _steps.GivenOcelotIsRunningWithPolly())
@@ -105,21 +105,21 @@ public void Should_open_circuit_breaker_then_close()
                 .And(x => _steps.ThenTheResponseBodyShouldBe("Hello from Laura"))
                 .BDDfy();
         }
-
+
         [Fact]
         public void Open_circuit_should_not_effect_different_route()
         {
             var port1 = PortFinder.GetRandomPort();
             var port2 = PortFinder.GetRandomPort();
             var qos1 = new QoSOptions(1, 1000, 500, null);
-
+
             var configuration = FileConfigurationFactory(port1, qos1);
             var route2 = configuration.Routes[0].Clone() as FileRoute;
             route2.DownstreamHostAndPorts[0].Port = port2;
             route2.UpstreamPathTemplate = "/working";
             route2.QoSOptions = new();
             configuration.Routes.Add(route2);
-
+
             this.Given(x => x.GivenThereIsAPossiblyBrokenServiceRunningOn($"http://localhost:{port1}", "Hello from Laura"))
                 .And(x => x.GivenThereIsAServiceRunningOn($"http://localhost:{port2}/", 200, "Hello from Tom", 0))
                 .And(x => _steps.GivenThereIsAConfiguration(configuration))
@@ -142,7 +142,7 @@ public void Open_circuit_should_not_effect_different_route()
                 .And(x => _steps.ThenTheResponseBodyShouldBe("Hello from Laura"))
                 .BDDfy();
         }
-
+
         private static void GivenIWaitMilliseconds(int ms) => Thread.Sleep(ms);
 
         private void GivenThereIsABrokenServiceRunningOn(string url)
@@ -168,8 +168,8 @@ private void GivenThereIsAPossiblyBrokenServiceRunningOn(string url, string resp
                 context.Response.StatusCode = 200;
                 await context.Response.WriteAsync(responseBody);
             });
-        }
-
+        }
+
         private void GivenThereIsAServiceRunningOn(string url, int statusCode, string responseBody, int timeout)
         {
             _serviceHandler.GivenThereIsAServiceRunningOn(url, async context =>
@@ -180,11 +180,11 @@ private void GivenThereIsAServiceRunningOn(string url, int statusCode, string re
             });
         }
 
-        public void Dispose()
-        {
-            _serviceHandler?.Dispose();
+        public void Dispose()
+        {
+            _serviceHandler?.Dispose();
             _steps.Dispose();
-            GC.SuppressFinalize(this);
-        }
-    }
+            GC.SuppressFinalize(this);
+        }
+    }
 }

test/Ocelot.Testing/PortFinder.cs

@@ -1,4 +1,5 @@
-using System.Collections.Concurrent;
+using System;
+using System.Collections.Concurrent;
 using System.Net;
 using System.Net.Sockets;
 

test/Ocelot.UnitTests/Configuration/Validation/RouteFluentValidatorTests.cs

@@ -358,7 +358,7 @@ public void should_be_invalid_re_route_using_downstream_http_version(string vers
             this.Given(_ => GivenThe(fileRoute))
                 .When(_ => WhenIValidate())
                 .Then(_ => ThenTheResultIsInvalid())
-                .And(_ => ThenTheErrorsContains("'Downstream Http Version' is not in the correct format."))
+                .And(_ => ThenTheErrorsContains("'Downstream Http Version'")) // this error message changes depending on the OS language
                 .BDDfy();
         }
 
@@ -396,7 +396,7 @@ private void ThenTheResultIsInvalid()
 
         private void ThenTheErrorsContains(string expected)
         {
-            _result.Errors.ShouldContain(x => x.ErrorMessage == expected);
+            _result.Errors.ShouldContain(x => x.ErrorMessage.Contains(expected));
         }
 
         private class FakeAutheHandler : IAuthenticationHandler

test/Ocelot.UnitTests/Ocelot.UnitTests.csproj

@@ -34,6 +34,7 @@
     <ProjectReference Include="..\..\src\Ocelot.Provider.Eureka\Ocelot.Provider.Eureka.csproj" />
     <ProjectReference Include="..\..\src\Ocelot.Provider.Polly\Ocelot.Provider.Polly.csproj" />
     <ProjectReference Include="..\..\src\Ocelot.Tracing.Butterfly\Ocelot.Tracing.Butterfly.csproj" />
+    <ProjectReference Include="..\Ocelot.Testing\Ocelot.Testing.csproj" />
   </ItemGroup>
   <ItemGroup>
     <Service Include="{82a7f48d-3b50-4b1e-b82e-3ada8210c358}" />