Downgrade the Warning to Information on missing Content-Length head…

…er in `MultiplexingMiddleware` (#2146) * fix: downgrade the warning to information on missing content-length header * chore: add route name to logs * test: fixing multiplexing middleware tests * Code review by @raman-m --------- Co-authored-by: Paul Roy <[email protected]> Co-authored-by: Raman Maksimchuk <[email protected]>
ThreeMammals · Sep 14, 2024 · c502e4f · c502e4f
1 parent 19a8e2f
commit c502e4f
Show file tree

Hide file tree

Showing 5 changed files with 27 additions and 23 deletions.
diff --git a/src/Ocelot.Provider.Polly/PollyQoSResiliencePipelineProvider.cs b/src/Ocelot.Provider.Polly/PollyQoSResiliencePipelineProvider.cs
@@ -38,11 +38,7 @@ public PollyQoSResiliencePipelineProvider(
     };
 
     protected virtual HashSet<HttpStatusCode> ServerErrorCodes { get; } = DefaultServerErrorCodes;
-
-    protected virtual string GetRouteName(DownstreamRoute route)
-        => string.IsNullOrWhiteSpace(route.ServiceName)
-            ? route.UpstreamPathTemplate?.Template ?? route.DownstreamPathTemplate?.Value ?? string.Empty
-            : route.ServiceName;
+    protected virtual string GetRouteName(DownstreamRoute route) => route.Name();
 
     /// <summary>
     /// Gets Polly V8 resilience pipeline (applies QoS feature) for the route.
@@ -57,9 +53,8 @@ public ResiliencePipeline<HttpResponseMessage> GetResiliencePipeline(DownstreamR
             return ResiliencePipeline<HttpResponseMessage>.Empty; // shortcut -> No QoS
         }
 
-        var currentRouteName = GetRouteName(route);
         return _registry.GetOrAddPipeline<HttpResponseMessage>(
-            key: new OcelotResiliencePipelineKey(currentRouteName),
+            key: new OcelotResiliencePipelineKey(GetRouteName(route)),
             configure: (builder) => ConfigureStrategies(builder, route));
     }
 
@@ -78,7 +73,7 @@ protected virtual ResiliencePipelineBuilder<HttpResponseMessage> ConfigureCircui
         }
 
         var options = route.QosOptions;
-        var info = $"Circuit Breaker for Route: {GetRouteName(route)}: ";
+        var info = $"Circuit Breaker for the route: {GetRouteName(route)}: ";
         var strategyOptions = new CircuitBreakerStrategyOptions<HttpResponseMessage>
         {
             FailureRatio = 0.8,
@@ -127,7 +122,7 @@ protected virtual ResiliencePipelineBuilder<HttpResponseMessage> ConfigureTimeou
             Timeout = TimeSpan.FromMilliseconds(options.TimeoutValue),
             OnTimeout = _ =>
             {
-                _logger.LogInformation($"Timeout for Route: {GetRouteName(route)}");
+                _logger.LogInformation(() => $"Timeout for the route: {GetRouteName(route)}");
                 return ValueTask.CompletedTask;
             },
         };

diff --git a/src/Ocelot/Configuration/DownstreamRoute.cs b/src/Ocelot/Configuration/DownstreamRoute.cs
@@ -91,7 +91,6 @@ public DownstreamRoute(
         public string ServiceName { get; }
         public string ServiceNamespace { get; }
         public HttpHandlerOptions HttpHandlerOptions { get; }
-        public bool UseServiceDiscovery { get; }
         public bool EnableEndpointEndpointRateLimiting { get; }
         public QoSOptions QosOptions { get; }
         public string DownstreamScheme { get; }
@@ -130,6 +129,13 @@ public DownstreamRoute(
         /// </remarks>
         public HttpVersionPolicy DownstreamHttpVersionPolicy { get; }
         public Dictionary<string, UpstreamHeaderTemplate> UpstreamHeaders { get; }
+        public bool UseServiceDiscovery { get; }
         public MetadataOptions MetadataOptions { get; }
+
+        /// <summary>Gets the route name depending on whether the service discovery mode is enabled or disabled.</summary>
+        /// <returns>A <see cref="string"/> object with the name.</returns>
+        public string Name() => string.IsNullOrEmpty(ServiceName) && !UseServiceDiscovery
+            ? UpstreamPathTemplate?.Template ?? DownstreamPathTemplate?.Value ?? "?"
+            : string.Join(':', ServiceNamespace, ServiceName, UpstreamPathTemplate?.Template);
     }
 }
diff --git a/src/Ocelot/Multiplexer/MultiplexingMiddleware.cs b/src/Ocelot/Multiplexer/MultiplexingMiddleware.cs
@@ -183,7 +183,7 @@ private IEnumerable<Task<HttpContext>> ProcessRouteWithComplexAggregation(Aggreg
     /// <returns>The cloned Http context.</returns>
     private async Task<HttpContext> ProcessRouteAsync(HttpContext sourceContext, DownstreamRoute route, List<PlaceholderNameAndValue> placeholders = null)
     {
-        var newHttpContext = await CreateThreadContextAsync(sourceContext);
+        var newHttpContext = await CreateThreadContextAsync(sourceContext, route);
         CopyItemsToNewContext(newHttpContext, sourceContext, placeholders);
         newHttpContext.Items.UpsertDownstreamRoute(route);
 
@@ -200,17 +200,18 @@ private static void CopyItemsToNewContext(HttpContext target, HttpContext source
         target.Items.SetIInternalConfiguration(source.Items.IInternalConfiguration());
         target.Items.UpsertTemplatePlaceholderNameAndValues(placeholders ??
                                                             source.Items.TemplatePlaceholderNameAndValues());
-    }
-
+    }
+
     /// <summary>
     /// Creates a new HttpContext based on the source.
     /// </summary>
-    /// <param name="source">The base http context.</param>
+    /// <param name="source">The base http context.</param>
+    /// <param name="route">Downstream route.</param>
     /// <returns>The cloned context.</returns>
-    protected virtual async Task<HttpContext> CreateThreadContextAsync(HttpContext source)
+    protected virtual async Task<HttpContext> CreateThreadContextAsync(HttpContext source, DownstreamRoute route)
     {
         var from = source.Request;
-        var bodyStream = await CloneRequestBodyAsync(from, source.RequestAborted);
+        var bodyStream = await CloneRequestBodyAsync(from, route, source.RequestAborted);
         var target = new DefaultHttpContext
         {
             Request =
@@ -245,7 +246,7 @@ protected virtual async Task<HttpContext> CreateThreadContextAsync(HttpContext s
         // Once the downstream request is completed and the downstream response has been read, the downstream response object can dispose of the body's Stream object
         target.Response.RegisterForDisposeAsync(bodyStream); // manage Stream lifetime by HttpResponse object
         return target;
-    }
+    }
 
     protected virtual Task MapAsync(HttpContext httpContext, Route route, List<HttpContext> contexts)
     {
@@ -258,12 +259,12 @@ protected virtual Task MapAsync(HttpContext httpContext, Route route, List<HttpC
         return aggregator.Aggregate(route, httpContext, contexts);
     }
 
-    protected virtual async Task<Stream> CloneRequestBodyAsync(HttpRequest request, CancellationToken aborted)
+    protected virtual async Task<Stream> CloneRequestBodyAsync(HttpRequest request, DownstreamRoute route, CancellationToken aborted)
     {
         request.EnableBuffering();
         if (request.Body.Position != 0)
         {
-            Logger.LogWarning("Ocelot does not support body copy without stream in initial position 0");
+            Logger.LogWarning(() => $"Ocelot does not support body copy without stream in initial position 0 for the route {route.Name()}.");
             return request.Body;
         }
 
@@ -276,7 +277,7 @@ protected virtual async Task<Stream> CloneRequestBodyAsync(HttpRequest request,
         }
         else
         {
-            Logger.LogWarning("Aggregation does not support body copy without Content-Length header!");
+            Logger.LogInformation(() => $"Aggregation does not support body copy without Content-Length header, skipping body copy for the route {route.Name()}.");
         }
 
         return targetBuffer;

diff --git a/src/Ocelot/ServiceDiscovery/ServiceDiscoveryProviderFactory.cs b/src/Ocelot/ServiceDiscovery/ServiceDiscoveryProviderFactory.cs
@@ -25,8 +25,7 @@ public Response<IServiceDiscoveryProvider> Get(ServiceProviderConfiguration serv
         {
             if (route.UseServiceDiscovery)
             {
-                var routeName = route.UpstreamPathTemplate?.Template ?? route.ServiceName ?? string.Empty;
-                _logger.LogInformation(() => $"The {nameof(DownstreamRoute.UseServiceDiscovery)} mode of the route '{routeName}' is enabled.");
+                _logger.LogInformation(() => $"The {nameof(DownstreamRoute.UseServiceDiscovery)} mode of the route '{route.Name()}' is enabled.");
                 return GetServiceDiscoveryProvider(serviceConfig, route);
             }
 

diff --git a/test/Ocelot.UnitTests/Multiplexing/MultiplexingMiddlewareTests.cs b/test/Ocelot.UnitTests/Multiplexing/MultiplexingMiddlewareTests.cs
@@ -63,12 +63,14 @@ public void should_not_multiplex()
         [Trait("Bug", "1396")]
         public async Task CreateThreadContextAsync_CopyUser_ToTarget()
         {
+            var route = new DownstreamRouteBuilder().Build();
+
             // Arrange
             GivenUser("test", "Copy", nameof(CreateThreadContextAsync_CopyUser_ToTarget));
 
             // Act
             var method = _middleware.GetType().GetMethod("CreateThreadContextAsync", BindingFlags.NonPublic | BindingFlags.Instance);
-            var actual = await (Task<HttpContext>)method.Invoke(_middleware, new object[] { _httpContext });
+            var actual = await (Task<HttpContext>)method.Invoke(_middleware, new object[] { _httpContext, route });
 
             // Assert
             AssertUsers(actual);
@@ -234,6 +236,7 @@ public async Task Should_Call_CloneRequestBodyAsync_Each_Time_Per_Requests(int n
             mock.Protected().Verify<Task<Stream>>("CloneRequestBodyAsync",
                 numberOfRoutes > 1 ? Times.Exactly(numberOfRoutes) : Times.Never(),
                 ItExpr.IsAny<HttpRequest>(),
+                ItExpr.IsAny<DownstreamRoute>(),
                 ItExpr.IsAny<CancellationToken>());
         }