fix: promote non-durable sink parameter 'queueLimitBytes' (#247)

Fixes #245
FantasticFiasco · Feb 20, 2022 · 2aee400 · 2aee400
1 parent 67fdde7
commit 2aee400
Show file tree

Hide file tree

Showing 9 changed files with 31 additions and 93 deletions.
diff --git a/.all-contributorsrc b/.all-contributorsrc
@@ -141,6 +141,15 @@
       "contributions": [
         "doc"
       ]
+    },
+    {
+      "login": "seruminar",
+      "name": "Yuriy Sountsov",
+      "avatar_url": "https://avatars.githubusercontent.com/u/35008875?v=4",
+      "profile": "https://github.com/seruminar",
+      "contributions": [
+        "ideas"
+      ]
     }
   ],
   "contributorsPerLine": 7

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -12,83 +12,9 @@ This project adheres to [Semantic Versioning](http://semver.org/) and is followi
 
 **Migration guide**
 
-By far the easiest way to migrate your code is to stop using positional arguments and instead use named arguments.
+The parameter `batchSizeLimitBytes` has been introduced to the methods `Http`, `DurableHttpUsingFileSizeRolledBuffers` and `DurableHttpUsingTimeRolledBuffers`. Please verify that the arguments pass by you to these methods still align with your intentions.
 
-If you use the non-durable sink please make the following changes to your code.
-
-```csharp
-// Before migration
-log = new LoggerConfiguration()
-  .WriteTo.Http("https://www.mylogs.com", 500, 500)
-  .CreateLogger();
-
-// After migration
-log = new LoggerConfiguration()
-  .WriteTo.Http(
-    requestUri: "https://www.mylogs.com",
-    batchPostingLimit: 500,
-    // the new argument batchSizeLimitBytes is positioned here
-    queueLimit: 500)
-  .CreateLogger();
-```
-
-If you use the durable file size rolled sink please make the following changes to your code.
-
-```csharp
-// Before migration
-log = new LoggerConfiguration()
-  .WriteTo.DurableHttpUsingFileSizeRolledBuffers(
-      "https://www.mylogs.com",
-      "MyBuffer",
-      ByteSize.GB,
-      false,
-      10,
-      500,
-      TimeSpan.FromSeconds(10))
-  .CreateLogger();
-
-// After migration
-log = new LoggerConfiguration()
-  .WriteTo.DurableHttpUsingFileSizeRolledBuffers(
-      requestUri: "https://www.mylogs.com",
-      bufferBaseFileName: "MyBuffer",
-      bufferFileSizeLimitBytes: ByteSize.GB,
-      bufferFileShared: false,
-      retainedBufferFileCountLimit: 10,
-      batchPostingLimit: 500,
-      // the new argument batchSizeLimitBytes is positioned here
-      period: TimeSpan.FromSeconds(10))
-  .CreateLogger();
-```
-
-If you use the durable time rolled sink please make the following changes to your code.
-
-```csharp
-// Before migration
-log = new LoggerConfiguration()
-  .WriteTo.DurableHttpUsingTimeRolledBuffers(
-      "https://www.mylogs.com",
-      "MyBuffer-{Date}.json",
-      ByteSize.GB,
-      false,
-      10,
-      500,
-      TimeSpan.FromSeconds(10))
-  .CreateLogger();
-
-// After migration
-log = new LoggerConfiguration()
-  .WriteTo.DurableHttpUsingTimeRolledBuffers(
-      requestUri: "https://www.mylogs.com",
-      bufferPathFormat: "MyBuffer-{Date}.json",
-      bufferFileSizeLimitBytes: ByteSize.GB,
-      bufferFileShared: false,
-      retainedBufferFileCountLimit: 10,
-      batchPostingLimit: 500,
-      // the new argument batchSizeLimitBytes is positioned here
-      period: TimeSpan.FromSeconds(10))
-  .CreateLogger();
-```
+To automatically mitigate this kind of *new parameter issue* in the future would be to move from using positional arguments to use named arguments.
 
 - [#166](https://github.com/FantasticFiasco/serilog-sinks-http/issues/166) Support for content encoding [Gzip](https://en.wikipedia.org/wiki/Gzip) using HTTP client `JsonGzipHttpClient` (contribution by [@vaibhavepatel](https://github.com/vaibhavepatel), [@KalininAndreyVictorovich](https://github.com/KalininAndreyVictorovich) and [@AntonSmolkov](https://github.com/AntonSmolkov))
 - [#166](https://github.com/FantasticFiasco/serilog-sinks-http/issues/166) Support for specifying `HttpClient` when creating `JsonHttpClient` and `JsonGzipHttpClient`
@@ -195,7 +121,7 @@ Given you are configuring the sink in application configuration you should apply
 ```
 
 - [#206](https://github.com/FantasticFiasco/serilog-sinks-http/issues/206) [BREAKING CHANGE] Argument `bufferFileSizeLimitBytes` to extension methods `DurableHttpUsingFileSizeRolledBuffers` and `DurableHttpUsingTimeRolledBuffers` no longer accepts `0` as value
-- [#203](https://github.com/FantasticFiasco/serilog-sinks-http/issues/203) [BREAKING CHANGE] Non-durable sink has changed from having its maximum queue size defined as number of events into number of bytes, making it far easier to reason about memory consumption.
+- [#203](https://github.com/FantasticFiasco/serilog-sinks-http/issues/203), [#245](https://github.com/FantasticFiasco/serilog-sinks-http/issues/245) [BREAKING CHANGE] Non-durable sink has changed from having its maximum queue size defined as number of events into number of bytes, making it far easier to reason about memory consumption. It's importance to the behavior of the sink was also the reasoning for promoting it from being optional to being mandatory. (proposed by [@seruminar](https://github.com/seruminar))
 
 Given you are configuring the sink in code you should do the following changes.
 

diff --git a/README-v8.md b/README-v8.md
@@ -43,7 +43,7 @@ In the following example, the sink will POST log events to `http://www.mylogs.co
 ```csharp
 ILogger log = new LoggerConfiguration()
   .MinimumLevel.Verbose()
-  .WriteTo.Http(requestUri: "https://www.mylogs.com")
+  .WriteTo.Http(requestUri: "https://www.mylogs.com", queueLimitBytes: null)
   .CreateLogger();
 
 log.Information("Logging {@Heartbeat} from {Computer}", heartbeat, computer);
@@ -59,7 +59,8 @@ Used in conjunction with [Serilog.Settings.Configuration](https://github.com/ser
       {
         "Name": "Http",
         "Args": {
-          "requestUri": "https://www.mylogs.com"
+          "requestUri": "https://www.mylogs.com",
+          "queueLimitBytes": null
         }
       }
     ]

diff --git a/README.md b/README.md
@@ -166,6 +166,7 @@ The following users have made significant contributions to this project. Thank y
     <td align="center"><a href="https://github.com/tipasergio"><img src="https://avatars.githubusercontent.com/u/6435956?v=4?s=100" width="100px;" alt=""/><br /><sub><b>Sergios</b></sub></a><br /><a href="https://github.com/FantasticFiasco/serilog-sinks-http/issues?q=author%3Atipasergio" title="Bug reports">🐛</a></td>
     <td align="center"><a href="https://github.com/AntonSmolkov"><img src="https://avatars.githubusercontent.com/u/5318028?v=4?s=100" width="100px;" alt=""/><br /><sub><b>Anton Smolkov</b></sub></a><br /><a href="https://github.com/FantasticFiasco/serilog-sinks-http/commits?author=AntonSmolkov" title="Code">💻</a> <a href="https://github.com/FantasticFiasco/serilog-sinks-http/issues?q=author%3AAntonSmolkov" title="Bug reports">🐛</a></td>
     <td align="center"><a href="https://github.com/Siphonophora"><img src="https://avatars.githubusercontent.com/u/32316111?v=4?s=100" width="100px;" alt=""/><br /><sub><b>Michael J Conrad</b></sub></a><br /><a href="https://github.com/FantasticFiasco/serilog-sinks-http/commits?author=Siphonophora" title="Documentation">📖</a></td>
+    <td align="center"><a href="https://github.com/seruminar"><img src="https://avatars.githubusercontent.com/u/35008875?v=4?s=100" width="100px;" alt=""/><br /><sub><b>Yuriy Sountsov</b></sub></a><br /><a href="#ideas-seruminar" title="Ideas, Planning, & Feedback">🤔</a></td>
   </tr>
 </table>
 

diff --git a/src/Serilog.Sinks.Http/LoggerSinkConfigurationExtensions.cs b/src/Serilog.Sinks.Http/LoggerSinkConfigurationExtensions.cs
@@ -43,6 +43,10 @@ public static class LoggerSinkConfigurationExtensions
         /// </summary>
         /// <param name="sinkConfiguration">The logger configuration.</param>
         /// <param name="requestUri">The URI the request is sent to.</param>
+        /// <param name="queueLimitBytes">
+        /// The maximum size, in bytes, of events stored in memory, waiting to be sent over the
+        /// network. Specify null for no limit.
+        /// </param>
         /// <param name="logEventLimitBytes">
         /// The maximum size, in bytes, for a serialized representation of a log event. Log events
         /// exceeding this size will be dropped. Specify null for no limit. Default value is null.
@@ -67,10 +71,6 @@ public static class LoggerSinkConfigurationExtensions
         /// <para/>
         /// Default value is null.
         /// </param>
-        /// <param name="queueLimitBytes">
-        /// The maximum size, in bytes, of events stored in memory, waiting to be sent over the
-        /// network. Specify null for no limit. Default value is null.
-        /// </param>
         /// <param name="period">
         /// The time to wait between checking for event batches. Default value is 2 seconds.
         /// </param>
@@ -100,10 +100,10 @@ public static class LoggerSinkConfigurationExtensions
         public static LoggerConfiguration Http(
             this LoggerSinkConfiguration sinkConfiguration,
             string requestUri,
+            long? queueLimitBytes,
             long? logEventLimitBytes = null,
             int? logEventsInBatchLimit = 1000,
             long? batchSizeLimitBytes = null,
-            long? queueLimitBytes = null,
             TimeSpan? period = null,
             ITextFormatter? textFormatter = null,
             IBatchFormatter? batchFormatter = null,
@@ -127,10 +127,10 @@ public static LoggerConfiguration Http(
 
             var sink = new HttpSink(
                 requestUri: requestUri,
+                queueLimitBytes: queueLimitBytes,
                 logEventLimitBytes: logEventLimitBytes,
                 logEventsInBatchLimit: logEventsInBatchLimit,
                 batchSizeLimitBytes: batchSizeLimitBytes,
-                queueLimitBytes: queueLimitBytes,
                 period: period.Value,
                 textFormatter: textFormatter,
                 batchFormatter: batchFormatter,

diff --git a/src/Serilog.Sinks.Http/Sinks/Http/Private/NonDurable/HttpSink.cs b/src/Serilog.Sinks.Http/Sinks/Http/Private/NonDurable/HttpSink.cs
@@ -43,10 +43,10 @@ public class HttpSink : ILogEventSink, IDisposable
 
         public HttpSink(
             string requestUri,
+            long? queueLimitBytes,
             long? logEventLimitBytes,
             int? logEventsInBatchLimit,
             long? batchSizeLimitBytes,
-            long? queueLimitBytes,
             TimeSpan period,
             ITextFormatter textFormatter,
             IBatchFormatter batchFormatter,

diff --git a/test/Serilog.Sinks.HttpTests/HttpSinkGivenCodeConfigurationShould.cs b/test/Serilog.Sinks.HttpTests/HttpSinkGivenCodeConfigurationShould.cs
@@ -39,9 +39,9 @@ public async Task WriteLogEvent(LogEventLevel level)
                 .WriteTo
                 .Http(
                     requestUri: webServerFixture.RequestUri(testId),
+                    queueLimitBytes: ByteSize.MB,
                     logEventsInBatchLimit: 100,
                     batchSizeLimitBytes: ByteSize.MB,
-                    queueLimitBytes: ByteSize.MB,
                     period: TimeSpan.FromMilliseconds(1),
                     textFormatter: new NormalRenderedTextFormatter(),
                     batchFormatter: new ArrayBatchFormatter(),
@@ -71,9 +71,9 @@ public async Task WriteBatches(int numberOfEvents)
                 .WriteTo
                 .Http(
                     requestUri: webServerFixture.RequestUri(testId),
+                    queueLimitBytes: null,
                     logEventsInBatchLimit: 100,
                     batchSizeLimitBytes: ByteSize.MB,
-                    queueLimitBytes: null,
                     period: TimeSpan.FromMilliseconds(1),
                     textFormatter: new NormalRenderedTextFormatter(),
                     batchFormatter: new ArrayBatchFormatter(),
@@ -104,9 +104,9 @@ public async Task OvercomeNetworkFailure()
                 .WriteTo
                 .Http(
                     requestUri: webServerFixture.RequestUri(testId),
+                    queueLimitBytes: ByteSize.MB,
                     logEventsInBatchLimit: 100,
                     batchSizeLimitBytes: ByteSize.MB,
-                    queueLimitBytes: ByteSize.MB,
                     period: TimeSpan.FromMilliseconds(1),
                     textFormatter: new NormalRenderedTextFormatter(),
                     batchFormatter: new ArrayBatchFormatter(),
@@ -136,6 +136,7 @@ public void ConfigureHttpClient()
                 .WriteTo
                 .Http(
                     requestUri: "https://www.mylogs.com",
+                    queueLimitBytes: null,
                     httpClient: httpClient,
                     configuration: configuration)
                 .CreateLogger();

diff --git a/test/Serilog.Sinks.HttpTests/Sinks/Http/Private/NonDurable/HttpSinkShould.cs b/test/Serilog.Sinks.HttpTests/Sinks/Http/Private/NonDurable/HttpSinkShould.cs
@@ -29,10 +29,10 @@ public async Task StayIdleGivenNoLogEvents()
 
             using (new HttpSink(
                 requestUri: webServerFixture.RequestUri(testId),
+                queueLimitBytes: null,
                 logEventLimitBytes: null,
                 logEventsInBatchLimit: null,
                 batchSizeLimitBytes: null,
-                queueLimitBytes: null,
                 period: period,
                 textFormatter: new NormalTextFormatter(),
                 batchFormatter: new ArrayBatchFormatter(),
@@ -56,10 +56,10 @@ public async Task RespectLogEventLimitBytes()
 
             using var sink = new HttpSink(
                 requestUri: webServerFixture.RequestUri(testId),
+                queueLimitBytes: null,
                 logEventLimitBytes: 1, // Is lower than emitted log event
                 logEventsInBatchLimit: null,
                 batchSizeLimitBytes: null,
-                queueLimitBytes: null,
                 period: period,
                 textFormatter: new NormalTextFormatter(),
                 batchFormatter: new ArrayBatchFormatter(),
@@ -91,10 +91,10 @@ public async Task RespectQueueLimitBytes()
 
             using var sink = new HttpSink(
                 requestUri: webServerFixture.RequestUri(testId),
+                queueLimitBytes: 134, // Queue only holds the first event, which allocates 134 bytes
                 logEventLimitBytes: null,
                 logEventsInBatchLimit: null,
                 batchSizeLimitBytes: null,
-                queueLimitBytes: 134, // Queue only holds the first event, which allocates 134 bytes
                 period: period,
                 textFormatter: new NormalTextFormatter(),
                 batchFormatter: new ArrayBatchFormatter(),

diff --git a/test/Serilog.Sinks.HttpTests/appsettings_http.json b/test/Serilog.Sinks.HttpTests/appsettings_http.json
@@ -6,9 +6,9 @@
         "Name": "Http",
         "Args": {
           "requestUri": "http://placeholder.com",
+          "queueLimitBytes": null,
           "logEventsInBatchLimit": 100,
           "batchSizeLimitBytes": 1048576,
-          "queueLimitBytes": null,
           "period": "00:00:00.001",
           "textFormatter": "Serilog.Sinks.Http.TextFormatters.NormalRenderedTextFormatter, Serilog.Sinks.Http",
           "batchFormatter": "Serilog.Sinks.Http.BatchFormatters.ArrayBatchFormatter, Serilog.Sinks.Http",