From 1fea4cc01b1da79335bb6ba33343baefd8146d2f Mon Sep 17 00:00:00 2001 From: Adam Hathcock Date: Thu, 27 Feb 2025 15:44:35 +0000 Subject: [PATCH] Add a logging statement for when batch size was larger than expected (25 megs) (#232) --- .../Serialization/BatchExtensions.cs | 5 ++-- .../Serialization/ChannelExtensions.cs | 2 ++ .../Serialization/ChannelSaver.cs | 4 ++-- .../SizeBatchingChannelReader.cs | 3 ++- .../Serialisation/V2/Send/SerializeProcess.cs | 10 +++++--- .../Serialisation/BatchTests.cs | 24 ++++++++++++++----- 6 files changed, 34 insertions(+), 14 deletions(-) diff --git a/src/Speckle.Sdk.Dependencies/Serialization/BatchExtensions.cs b/src/Speckle.Sdk.Dependencies/Serialization/BatchExtensions.cs index db388b9a..6867204c 100644 --- a/src/Speckle.Sdk.Dependencies/Serialization/BatchExtensions.cs +++ b/src/Speckle.Sdk.Dependencies/Serialization/BatchExtensions.cs @@ -1,4 +1,4 @@ -using System.Buffers; +using System.Buffers; namespace Speckle.Sdk.Serialisation.V2.Send; @@ -19,12 +19,13 @@ public static class BatchExtensions public static void AddBatchItem(this IMemoryOwner batch, T item) where T : IHasByteSize => ((Batch)batch).Add(item); - public static int GetBatchSize(this IMemoryOwner batch, int maxBatchSize) + public static int GetBatchSize(this IMemoryOwner batch, Action logAsWarning, int maxBatchSize) where T : IHasByteSize { var currentSize = ((Batch)batch).BatchByteSize; if (currentSize > maxBatchSize) { + logAsWarning($"Batch size exceeded. Current size: {currentSize} bytes. Max size: {maxBatchSize} bytes."); return maxBatchSize; } diff --git a/src/Speckle.Sdk.Dependencies/Serialization/ChannelExtensions.cs b/src/Speckle.Sdk.Dependencies/Serialization/ChannelExtensions.cs index 4133f2eb..f8f3524c 100644 --- a/src/Speckle.Sdk.Dependencies/Serialization/ChannelExtensions.cs +++ b/src/Speckle.Sdk.Dependencies/Serialization/ChannelExtensions.cs @@ -8,6 +8,7 @@ public static class ChannelExtensions { public static BatchingChannelReader> BatchByByteSize( this ChannelReader source, + Action logAsWarning, int batchSize, bool singleReader = false, bool allowSynchronousContinuations = false @@ -15,6 +16,7 @@ public static class ChannelExtensions where T : IHasByteSize => new SizeBatchingChannelReader( source ?? throw new ArgumentNullException(nameof(source)), + logAsWarning, batchSize, singleReader, allowSynchronousContinuations diff --git a/src/Speckle.Sdk.Dependencies/Serialization/ChannelSaver.cs b/src/Speckle.Sdk.Dependencies/Serialization/ChannelSaver.cs index 83289109..5bb65dba 100644 --- a/src/Speckle.Sdk.Dependencies/Serialization/ChannelSaver.cs +++ b/src/Speckle.Sdk.Dependencies/Serialization/ChannelSaver.cs @@ -5,7 +5,7 @@ using Speckle.Sdk.Serialisation.V2.Send; namespace Speckle.Sdk.Dependencies.Serialization; -public abstract class ChannelSaver(CancellationToken cancellationToken) +public abstract class ChannelSaver(Action logAsWarning, CancellationToken cancellationToken) where T : IHasByteSize { private const int SEND_CAPACITY = 500; @@ -32,7 +32,7 @@ public abstract class ChannelSaver(CancellationToken cancellationToken) public Task Start() => _checkCacheChannel - .Reader.BatchByByteSize(HTTP_SEND_CHUNK_SIZE) + .Reader.BatchByByteSize(logAsWarning, HTTP_SEND_CHUNK_SIZE) .WithTimeout(HTTP_BATCH_TIMEOUT) .PipeAsync( MAX_PARALLELISM_HTTP, diff --git a/src/Speckle.Sdk.Dependencies/Serialization/SizeBatchingChannelReader.cs b/src/Speckle.Sdk.Dependencies/Serialization/SizeBatchingChannelReader.cs index bf09e65d..5596edd4 100644 --- a/src/Speckle.Sdk.Dependencies/Serialization/SizeBatchingChannelReader.cs +++ b/src/Speckle.Sdk.Dependencies/Serialization/SizeBatchingChannelReader.cs @@ -11,6 +11,7 @@ public interface IHasByteSize public sealed class SizeBatchingChannelReader( ChannelReader source, + Action logAsWarning, int batchSize, bool singleReader, bool syncCont = false @@ -33,5 +34,5 @@ public sealed class SizeBatchingChannelReader( protected override void AddBatchItem(IMemoryOwner batch, T item) => batch.AddBatchItem(item); - protected override int GetBatchSize(IMemoryOwner batch) => batch.GetBatchSize(_batchSize); + protected override int GetBatchSize(IMemoryOwner batch) => batch.GetBatchSize(logAsWarning, _batchSize); } diff --git a/src/Speckle.Sdk/Serialisation/V2/Send/SerializeProcess.cs b/src/Speckle.Sdk/Serialisation/V2/Send/SerializeProcess.cs index f26bb5e4..bcddbe92 100644 --- a/src/Speckle.Sdk/Serialisation/V2/Send/SerializeProcess.cs +++ b/src/Speckle.Sdk/Serialisation/V2/Send/SerializeProcess.cs @@ -35,9 +35,13 @@ public sealed class SerializeProcess( ILoggerFactory loggerFactory, CancellationToken cancellationToken, SerializeProcessOptions? options = null -#pragma warning disable CS9107 // Parameter is captured into the state of the enclosing type and its value is also passed to the base constructor. The value might be captured by the base class as well. -) : ChannelSaver(cancellationToken), ISerializeProcess -#pragma warning restore CS9107 // Parameter is captured into the state of the enclosing type and its value is also passed to the base constructor. The value might be captured by the base class as well. +#pragma warning disable CS9107 +#pragma warning disable CA2254 +) + : ChannelSaver(x => loggerFactory.CreateLogger().LogWarning(x), cancellationToken), + ISerializeProcess +#pragma warning restore CA2254 +#pragma warning restore CS9107 { //async dispose [SuppressMessage("Usage", "CA2213:Disposable fields should be disposed")] diff --git a/tests/Speckle.Sdk.Tests.Unit/Serialisation/BatchTests.cs b/tests/Speckle.Sdk.Tests.Unit/Serialisation/BatchTests.cs index a23d44b2..de6f38a0 100644 --- a/tests/Speckle.Sdk.Tests.Unit/Serialisation/BatchTests.cs +++ b/tests/Speckle.Sdk.Tests.Unit/Serialisation/BatchTests.cs @@ -12,6 +12,8 @@ public class BatchTests public int ByteSize { get; } = size; } + private static readonly Action EMPTY_LOGGER = _ => { }; + [Fact] public void TestBatchSize_Calc() { @@ -22,6 +24,16 @@ public class BatchTests batch.BatchByteSize.Should().Be(3); } + [Fact] + public void Ensure_logging() + { + using var batch = new Batch(); + batch.AddBatchItem(new BatchItem(2)); + bool called = false; + batch.GetBatchSize(x => called = true, 1); + called.Should().BeTrue(); + } + [Fact] public void TestBatchSize_Trim() { @@ -64,13 +76,13 @@ public class BatchTests using var batch = BatchExtensions.CreateBatch(); batch.AddBatchItem(new BatchItem(2)); - bool full = batch.GetBatchSize(MAX_BATCH_SIZE) == MAX_BATCH_SIZE; + bool full = batch.GetBatchSize(EMPTY_LOGGER, MAX_BATCH_SIZE) == MAX_BATCH_SIZE; full.Should().BeFalse(); batch.AddBatchItem(new BatchItem(2)); - full = batch.GetBatchSize(MAX_BATCH_SIZE) == MAX_BATCH_SIZE; + full = batch.GetBatchSize(EMPTY_LOGGER, MAX_BATCH_SIZE) == MAX_BATCH_SIZE; full.Should().BeFalse(); batch.AddBatchItem(new BatchItem(2)); - full = batch.GetBatchSize(MAX_BATCH_SIZE) == MAX_BATCH_SIZE; + full = batch.GetBatchSize(EMPTY_LOGGER, MAX_BATCH_SIZE) == MAX_BATCH_SIZE; full.Should().BeTrue(); } @@ -81,7 +93,7 @@ public class BatchTests using var batch = BatchExtensions.CreateBatch(); batch.AddBatchItem(new BatchItem(63)); - bool full = batch.GetBatchSize(MAX_BATCH_SIZE) == MAX_BATCH_SIZE; + bool full = batch.GetBatchSize(EMPTY_LOGGER, MAX_BATCH_SIZE) == MAX_BATCH_SIZE; full.Should().BeTrue(); } @@ -92,10 +104,10 @@ public class BatchTests using var batch = BatchExtensions.CreateBatch(); batch.AddBatchItem(new BatchItem(2)); - bool full = batch.GetBatchSize(MAX_BATCH_SIZE) == MAX_BATCH_SIZE; + bool full = batch.GetBatchSize(EMPTY_LOGGER, MAX_BATCH_SIZE) == MAX_BATCH_SIZE; full.Should().BeFalse(); batch.AddBatchItem(new BatchItem(63)); - full = batch.GetBatchSize(MAX_BATCH_SIZE) == MAX_BATCH_SIZE; + full = batch.GetBatchSize(EMPTY_LOGGER, MAX_BATCH_SIZE) == MAX_BATCH_SIZE; full.Should().BeTrue(); } }