Replaced ConcurrentBag<T> with SynchronizedCollection<T>

CHANGELOG.md

@@ -1,6 +1,9 @@
 # Changelog
 
-## Unreleased
+## 4.9.0-sync.collection.2
+
+### Experimental
+- This release includes experimental use of a `SynchronizedCollection` instead of a ConcurrentBag to see if this has any impact on high memory usage experienced by some SDK users. 
 
 ### Fixes
 - Race condition in `SentryMessageHandler` ([#3477](https://github.com/getsentry/sentry-dotnet/pull/3477))

Directory.Build.props

@@ -1,7 +1,7 @@
 <Project>
 
   <PropertyGroup>
-    <VersionPrefix>4.9.0</VersionPrefix>
+    <VersionPrefix>4.9.0-sync.collection.2</VersionPrefix>
     <LangVersion>12</LangVersion>
     <AccelerateBuildsInVisualStudio>true</AccelerateBuildsInVisualStudio>
     <TreatWarningsAsErrors>true</TreatWarningsAsErrors>

benchmarks/Sentry.Benchmarks/BenchmarkDotNet.Artifacts/results/Sentry.Benchmarks.TransactionBenchmarks-report-github.md

@@ -0,0 +1,20 @@
+```
+
+BenchmarkDotNet v0.13.12, macOS Sonoma 14.5 (23F79) [Darwin 23.5.0]
+Apple M2 Max, 1 CPU, 12 logical and 12 physical cores
+.NET SDK 8.0.204
+  [Host]     : .NET 6.0.22 (6.0.2223.42425), Arm64 RyuJIT AdvSIMD
+  Job-IGCIGQ : .NET 6.0.22 (6.0.2223.42425), Arm64 RyuJIT AdvSIMD
+
+InvocationCount=10  IterationCount=10  LaunchCount=1  
+UnrollFactor=1  WarmupCount=1  
+
+```
+| Method         | SpanCount | Mean      | Error     | StdDev   | Allocated |
+|--------------- |---------- |----------:|----------:|---------:|----------:|
+| **ConcurrentBag**  | **1**         |  **15.57 μs** |  **3.013 μs** | **1.793 μs** |  **12.69 KB** |
+| SyncCollection | 1         |  14.71 μs |  2.396 μs | 1.426 μs |  12.02 KB |
+| **ConcurrentBag**  | **10**        |  **29.02 μs** |  **3.721 μs** | **2.214 μs** |  **26.17 KB** |
+| SyncCollection | 10        |  26.50 μs |  3.180 μs | 1.663 μs |  25.68 KB |
+| **ConcurrentBag**  | **100**       | **157.62 μs** |  **9.686 μs** | **5.764 μs** | **150.19 KB** |
+| SyncCollection | 100       | 137.18 μs | 11.170 μs | 6.647 μs | 149.33 KB |

benchmarks/Sentry.Benchmarks/TransactionBenchmarks.cs

@@ -2,24 +2,29 @@
 
 namespace Sentry.Benchmarks;
 
+[SimpleJob(launchCount: 1, warmupCount: 1, invocationCount: 10, iterationCount: 10)]
 public class TransactionBenchmarks
 {
     private const string Operation = "Operation";
     private const string Name = "Name";
 
-    [Params(1, 10, 100, 1000)]
+    [Params(1, 10, 100)]
     public int SpanCount;
 
     private IDisposable _sdk;
 
-    [GlobalSetup(Target = nameof(CreateTransaction))]
-    public void EnabledSdk() => _sdk = SentrySdk.Init(Constants.ValidDsn);
+    [GlobalSetup]
+    public void EnabledSdk() => _sdk = SentrySdk.Init(o =>
+    {
+        o.Dsn = Constants.ValidDsn;
+        o.TracesSampleRate = 1.0;
+    });
 
-    [GlobalCleanup(Target = nameof(CreateTransaction))]
+    [GlobalCleanup]
     public void DisableDsk() => _sdk.Dispose();
 
-    [Benchmark(Description = "Creates a Transaction")]
-    public void CreateTransaction()
+    [Benchmark]
+    public void SynchronizedCollection()
     {
         var transaction = SentrySdk.StartTransaction(Name, Operation);
 

src/Sentry/Internal/Wcf/ATTRIBUTION.txt

@@ -0,0 +1,28 @@
+Parts of the code in this subdirectory have been adapted from
+https://github.com/dotnet/wcf
+
+The original license is as follows:
+
+The MIT License (MIT)
+
+Copyright (c) .NET Foundation and Contributors
+
+All rights reserved.
+
+Permission is hereby granted, free of charge, to any person obtaining a copy
+of this software and associated documentation files (the "Software"), to deal
+in the Software without restriction, including without limitation the rights
+to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+copies of the Software, and to permit persons to whom the Software is
+furnished to do so, subject to the following conditions:
+
+The above copyright notice and this permission notice shall be included in all
+copies or substantial portions of the Software.
+
+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+SOFTWARE.

src/Sentry/Internal/Wcf/SynchronizedCollection.cs

@@ -0,0 +1,151 @@
+// Adapted from https://github.com/dotnet/wcf/blob/852a6098b270771f21d13824877fce59d3279046/src/System.ServiceModel.Primitives/src/System/ServiceModel/SynchronizedCollection.cs
+namespace Sentry.Internal.Wcf;
+
+/// <summary>
+/// <para>
+/// Provides a thread-safe collection that contains objects of a type specified by the generic parameter as elements.
+/// </para>
+/// <para>
+/// See https://learn.microsoft.com/en-us/dotnet/api/system.collections.generic.synchronizedcollection-1?view=net-8.0
+/// </para>
+/// </summary>
+internal class SynchronizedCollection<T>
+{
+    private readonly object _sync = new();
+
+    public IReadOnlyCollection<T> ToReadOnlyCollection()
+    {
+        lock (_sync)
+        {
+            // This will be pig slow but ensures that the collection is thread-safe for IEnumerable operations.
+            // This is just to run an experiment - DO NOT MERGE THIS INTO THE MAIN BRANCH!!!
+            return new ReadOnlyCollection<T>(Items.ToArray());
+        }
+    }
+
+    public int Count
+    {
+        get { lock (_sync) { return Items.Count; } }
+    }
+
+    private List<T> Items { get; } = [];
+
+    public T this[int index]
+    {
+        get
+        {
+            lock (_sync)
+            {
+                return Items[index];
+            }
+        }
+        set
+        {
+            lock (_sync)
+            {
+                if (index < 0 || index >= Items.Count)
+                {
+                    throw new ArgumentOutOfRangeException();
+                }
+
+                Items[index] = value;
+            }
+        }
+    }
+
+    public void Add(T item)
+    {
+        lock (_sync)
+        {
+            var index = Items.Count;
+            Items.Insert(index, item);
+        }
+    }
+
+    public void Clear()
+    {
+        lock (_sync)
+        {
+            Items.Clear();
+        }
+    }
+
+    public void CopyTo(T[] array, int index)
+    {
+        lock (_sync)
+        {
+            Items.CopyTo(array, index);
+        }
+    }
+
+    public bool Contains(T item)
+    {
+        lock (_sync)
+        {
+            return Items.Contains(item);
+        }
+    }
+
+    public int IndexOf(T item)
+    {
+        lock (_sync)
+        {
+            return InternalIndexOf(item);
+        }
+    }
+
+    public void Insert(int index, T item)
+    {
+        lock (_sync)
+        {
+            if (index < 0 || index > Items.Count)
+            {
+                throw new ArgumentOutOfRangeException();
+            }
+
+            Items.Insert(index, item);
+        }
+    }
+
+    private int InternalIndexOf(T item)
+    {
+        var count = Items.Count;
+
+        for (var i = 0; i < count; i++)
+        {
+            if (Equals(Items[i], item))
+            {
+                return i;
+            }
+        }
+        return -1;
+    }
+
+    public bool Remove(T item)
+    {
+        lock (_sync)
+        {
+            var index = InternalIndexOf(item);
+            if (index < 0)
+            {
+                return false;
+            }
+
+            Items.RemoveAt(index);
+            return true;
+        }
+    }
+
+    public void RemoveAt(int index)
+    {
+        lock (_sync)
+        {
+            if (index < 0 || index >= Items.Count)
+            {
+                throw new ArgumentOutOfRangeException();
+            }
+
+            Items.RemoveAt(index);
+        }
+    }
+}

src/Sentry/TransactionTracer.cs

@@ -1,5 +1,7 @@
+using System;
 using Sentry.Extensibility;
 using Sentry.Internal;
+using Sentry.Internal.Wcf;
 using Sentry.Protocol;
 
 namespace Sentry;
@@ -167,14 +169,10 @@ public IReadOnlyList<string> Fingerprint
     /// <inheritdoc />
     public IReadOnlyDictionary<string, string> Tags => _tags;
 
-#if NETSTANDARD2_1_OR_GREATER
-    private readonly ConcurrentBag<ISpan> _spans = new();
-#else
-    private ConcurrentBag<ISpan> _spans = new();
-#endif
+    private readonly SynchronizedCollection<ISpan> _spans = new();
 
     /// <inheritdoc />
-    public IReadOnlyCollection<ISpan> Spans => _spans;
+    public IReadOnlyCollection<ISpan> Spans => _spans.ToReadOnlyCollection();
 
     private readonly ConcurrentDictionary<string, Measurement> _measurements = new();
 
@@ -388,7 +386,7 @@ public void Finish()
         _hub.CaptureTransaction(new SentryTransaction(this));
 
         // Release tracked spans
-        ReleaseSpans();
+        _activeSpanTracker.Clear();
     }
 
     /// <inheritdoc />
@@ -418,14 +416,4 @@ public string? Origin
         get => Contexts.Trace.Origin;
         internal set => Contexts.Trace.Origin = value;
     }
-
-    private void ReleaseSpans()
-    {
-#if NETSTANDARD2_1_OR_GREATER
-        _spans.Clear();
-#else
-        _spans = new ConcurrentBag<ISpan>();
-#endif
-        _activeSpanTracker.Clear();
-    }
 }

test/Sentry.DiagnosticSource.Tests/Integration/SQLite/SentryDiagnosticListenerTests.cs

@@ -70,7 +70,6 @@ public void EfCoreIntegration_RunSynchronousQueryWithIssue_TransactionWithSpans(
         // Arrange
         var hub = _fixture.Hub;
         var transaction = hub.StartTransaction("test", "test");
-        var spans = transaction.Spans;
         var context = _fixture.NewContext();
         Exception exception = null;
 
@@ -86,6 +85,7 @@ public void EfCoreIntegration_RunSynchronousQueryWithIssue_TransactionWithSpans(
 
         // Assert
         Assert.NotNull(exception);
+        var spans = transaction.Spans;
 #if NETFRAMEWORK
         Assert.Single(spans); //1 command
 #else
@@ -104,13 +104,13 @@ public void EfCoreIntegration_RunSynchronousQuery_TransactionWithSpans()
         // Arrange
         var hub = _fixture.Hub;
         var transaction = hub.StartTransaction("test", "test");
-        var spans = transaction.Spans;
         var context = _fixture.NewContext();
 
         // Act
         var result = context.Items.FromSqlRaw("SELECT * FROM Items").ToList();
 
         // Assert
+        var spans = transaction.Spans;
         Assert.Equal(3, result.Count);
 #if NETFRAMEWORK
         Assert.Single(spans); //1 command
@@ -146,7 +146,6 @@ public async Task EfCoreIntegration_RunAsyncQuery_TransactionWithSpansWithOneCom
 
         var hub = _fixture.Hub;
         var transaction = hub.StartTransaction("test", "test");
-        var spans = transaction.Spans;
         var itemsList = new ConcurrentBag<List<Item>>();
 
         // Act
@@ -158,6 +157,7 @@ public async Task EfCoreIntegration_RunAsyncQuery_TransactionWithSpansWithOneCom
         await Task.WhenAll(tasks);
 
         // Assert
+        var spans = transaction.Spans;
         Assert.Equal(totalCommands, itemsList.Count);
         Assert.Equal(totalCommands, spans.Count(s => s.Operation == "db.query"));
 #if NET5_0_OR_GREATER
@@ -180,7 +180,6 @@ public async Task EfCoreIntegration_RunAsyncQuery_TransactionWithSpans()
         var context = _fixture.NewContext();
         var hub = _fixture.Hub;
         var transaction = hub.StartTransaction("test", "test");
-        var spans = transaction.Spans;
 
         // Act
         var result = new[]
@@ -193,6 +192,7 @@ public async Task EfCoreIntegration_RunAsyncQuery_TransactionWithSpans()
         await Task.WhenAll(result);
 
         // Assert
+        var spans = transaction.Spans;
         Assert.Equal(3, result[0].Result.Count);
         Assert.Equal(4, spans.Count(s => s.Operation == "db.query"));
 #if NET5_0_OR_GREATER

test/Sentry.DiagnosticSource.Tests/SentrySqlListenerTests.cs

@@ -119,10 +119,10 @@ public void OnNext_KnownKey_GetSpanInvoked(string key, bool addConnectionSpan)
                 }));
 
         // Assert
-        var spans = _fixture.Spans.Where(s => s.Operation != "abc");
+        var spans = _fixture.Spans.Where(s => s.Operation != "abc").ToList();
         Assert.NotEmpty(spans);
 
-        var firstSpan = _fixture.Spans.OrderByDescending(x => x.StartTimestamp).First();
+        var firstSpan = spans.OrderByDescending(x => x.StartTimestamp).First();
         Assert.True(GetValidator(key)(firstSpan));
     }