Improve code coverage for Metrics (#3094)

Author: jamescrosswell

src/Sentry/MetricAggregator.cs

@@ -8,9 +8,15 @@ namespace Sentry;
 
 internal class MetricAggregator : IMetricAggregator
 {
+    internal const string DisposingMessage = "Disposing MetricAggregator.";
+    internal const string AlreadyDisposedMessage = "Already disposed MetricAggregator.";
+    internal const string CancelledMessage = "Stopping the Metric Aggregator due to a cancellation.";
+    internal const string ShutdownScheduledMessage = "Shutdown scheduled. Stopping by: {0}.";
+    internal const string ShutdownImmediatelyMessage = "Exiting immediately due to 0 shutdown timeout.";
+    internal const string FlushShutdownMessage = "Shutdown token triggered. Exiting metric aggregator.";
+
     private readonly SentryOptions _options;
     private readonly IMetricHub _metricHub;
-    private readonly TimeSpan _flushInterval;
 
     private readonly SemaphoreSlim _codeLocationLock = new(1,1);
     private readonly ReaderWriterLockSlim _bucketsLock = new ReaderWriterLockSlim();
@@ -26,20 +32,18 @@ internal class MetricAggregator : IMetricAggregator
     private readonly Lazy<Dictionary<long, ConcurrentDictionary<string, Metric>>> _buckets
         = new(() => new Dictionary<long, ConcurrentDictionary<string, Metric>>());
 
-    private long _lastClearedStaleLocations = DateTimeOffset.UtcNow.GetDayBucketKey();
-    private readonly ConcurrentDictionary<long, HashSet<MetricResourceIdentifier>> _seenLocations = new();
-    private Dictionary<long, Dictionary<MetricResourceIdentifier, SentryStackFrame>> _pendingLocations = new();
+    internal long _lastClearedStaleLocations = DateTimeOffset.UtcNow.GetDayBucketKey();
+    internal readonly ConcurrentDictionary<long, HashSet<MetricResourceIdentifier>> _seenLocations = new();
+    internal Dictionary<long, Dictionary<MetricResourceIdentifier, SentryStackFrame>> _pendingLocations = new();
 
-    private readonly Task _loopTask;
+    internal readonly Task _loopTask;
 
     internal MetricAggregator(SentryOptions options, IMetricHub metricHub,
-        CancellationTokenSource? shutdownSource = null,
-        bool disableLoopTask = false, TimeSpan? flushInterval = null)
+        CancellationTokenSource? shutdownSource = null, bool disableLoopTask = false)
     {
         _options = options;
         _metricHub = metricHub;
         _shutdownSource = shutdownSource ?? new CancellationTokenSource();
-        _flushInterval = flushInterval ?? TimeSpan.FromSeconds(5);
 
         if (disableLoopTask)
         {
@@ -157,7 +161,7 @@ public void Set(string key,
     }
 
     /// <inheritdoc cref="IMetricAggregator.Timing"/>
-    public void Timing(string key,
+    public virtual void Timing(string key,
         double value,
         MeasurementUnit.Duration unit = MeasurementUnit.Duration.Second,
         IDictionary<string, string>? tags = null,
@@ -321,12 +325,12 @@ private async Task RunLoopAsync()
                 // If the cancellation was signaled, run until the end of the queue or shutdownTimeout
                 try
                 {
-                    await Task.Delay(_flushInterval, _shutdownSource.Token).ConfigureAwait(false);
+                    await Task.Delay(_options.ShutdownTimeout, _shutdownSource.Token).ConfigureAwait(false);
                 }
                 // Cancellation requested and no timeout allowed, so exit even if there are more items
                 catch (OperationCanceledException) when (_options.ShutdownTimeout == TimeSpan.Zero)
                 {
-                    _options.LogDebug("Exiting immediately due to 0 shutdown timeout.");
+                    _options.LogDebug(ShutdownImmediatelyMessage);
 
                     await shutdownTimeout.CancelAsync().ConfigureAwait(false);
 
@@ -335,9 +339,7 @@ private async Task RunLoopAsync()
                 // Cancellation requested, scheduled shutdown
                 catch (OperationCanceledException)
                 {
-                    _options.LogDebug(
-                        "Shutdown scheduled. Stopping by: {0}.",
-                        _options.ShutdownTimeout);
+                    _options.LogDebug(ShutdownScheduledMessage, _options.ShutdownTimeout);
 
                     shutdownTimeout.CancelAfterSafe(_options.ShutdownTimeout);
 
@@ -407,15 +409,20 @@ public async Task FlushAsync(bool force = true, CancellationToken cancellationTo
         }
         catch (OperationCanceledException)
         {
-            _options.LogInfo("Shutdown token triggered. Exiting metric aggregator.");
+            _options.LogInfo(FlushShutdownMessage);
         }
         catch (Exception exception)
         {
             _options.LogError(exception, "Error processing metrics.");
         }
         finally
         {
-            _flushLock.Release();
+            // If the shutdown token was cancelled before we start this method, we can get here
+            // without the _flushLock.CurrentCount (i.e. available threads) having been decremented
+            if (_flushLock.CurrentCount < 1)
+            {
+                _flushLock.Release();
+            }
         }
     }
 
@@ -483,9 +490,9 @@ private Dictionary<long, Dictionary<MetricResourceIdentifier, SentryStackFrame>>
     /// <summary>
     /// Clear out stale seen locations once a day
     /// </summary>
-    private void ClearStaleLocations()
+    internal void ClearStaleLocations(DateTimeOffset? testNow = null)
     {
-        var now = DateTimeOffset.UtcNow;
+        var now = testNow ?? DateTimeOffset.UtcNow;
         var today = now.GetDayBucketKey();
         if (_lastClearedStaleLocations == today)
         {
@@ -511,11 +518,11 @@ private void ClearStaleLocations()
     /// <inheritdoc cref="IAsyncDisposable.DisposeAsync"/>
     public async ValueTask DisposeAsync()
     {
-        _options.LogDebug("Disposing MetricAggregator.");
+        _options.LogDebug(DisposingMessage);
 
         if (_disposed)
         {
-            _options.LogDebug("Already disposed MetricAggregator.");
+            _options.LogDebug(AlreadyDisposedMessage);
             return;
         }
 
@@ -534,7 +541,7 @@ public async ValueTask DisposeAsync()
         }
         catch (OperationCanceledException)
         {
-            _options.LogDebug("Stopping the Metric Aggregator due to a cancellation.");
+            _options.LogDebug(CancelledMessage);
         }
         catch (Exception exception)
         {

src/Sentry/MetricHelper.cs

@@ -5,8 +5,9 @@ namespace Sentry;
 internal static partial class MetricHelper
 {
     private static readonly RandomValuesFactory Random = new SynchronizedRandomValuesFactory();
-
     private const int RollupInSeconds = 10;
+    private const string InvalidKeyCharactersPattern = @"[^a-zA-Z0-9_/.-]+";
+    private const string InvalidValueCharactersPattern = @"[^\w\d_:/@\.\{\}\[\]$-]+";
 
 #if NET6_0_OR_GREATER
     private static readonly DateTimeOffset UnixEpoch = DateTimeOffset.UnixEpoch;
@@ -40,17 +41,17 @@ internal static DateTimeOffset GetCutoff() => DateTimeOffset.UtcNow
         .Subtract(TimeSpan.FromMilliseconds(FlushShift));
 
 #if NET7_0_OR_GREATER
-    [GeneratedRegex(@"[^a-zA-Z0-9_/.-]+", RegexOptions.Compiled)]
+    [GeneratedRegex(InvalidKeyCharactersPattern, RegexOptions.Compiled)]
     private static partial Regex InvalidKeyCharacters();
     internal static string SanitizeKey(string input) => InvalidKeyCharacters().Replace(input, "_");
 
-    [GeneratedRegex(@"[^\w\d_:/@\.\{\}\[\]$-]+", RegexOptions.Compiled)]
+    [GeneratedRegex(InvalidValueCharactersPattern, RegexOptions.Compiled)]
     private static partial Regex InvalidValueCharacters();
     internal static string SanitizeValue(string input) => InvalidValueCharacters().Replace(input, "_");
 #else
-    private static readonly Regex InvalidKeyCharacters = new(@"[^a-zA-Z0-9_/.-]+", RegexOptions.Compiled);
+    private static readonly Regex InvalidKeyCharacters = new(InvalidKeyCharactersPattern, RegexOptions.Compiled);
     internal static string SanitizeKey(string input) => InvalidKeyCharacters.Replace(input, "_");
-    private static readonly Regex InvalidValueCharacters = new(@"[^\w\d_:/@\.\{\}\[\]$-]+", RegexOptions.Compiled);
+    private static readonly Regex InvalidValueCharacters = new(InvalidValueCharactersPattern, RegexOptions.Compiled);
     internal static string SanitizeValue(string input) => InvalidValueCharacters.Replace(input, "_");
 #endif
 }

src/Sentry/Timing.cs

@@ -14,32 +14,31 @@ namespace Sentry;
 /// </example>
 internal class Timing : IDisposable
 {
-    private readonly IMetricHub _metricHub;
+    internal const string OperationName = "metric.timing";
+
     private readonly SentryOptions _options;
     private readonly MetricAggregator _metricAggregator;
     private readonly string _key;
     private readonly MeasurementUnit.Duration _unit;
     private readonly IDictionary<string, string>? _tags;
-    private readonly Stopwatch _stopwatch = new();
+    internal readonly Stopwatch _stopwatch = new();
     private readonly ISpan _span;
-    private readonly DateTime _startTime = DateTime.UtcNow;
+    internal readonly DateTime _startTime = DateTime.UtcNow;
 
     /// <summary>
     /// Creates a new <see cref="Timing"/> instance.
     /// </summary>
     internal Timing(MetricAggregator metricAggregator, IMetricHub metricHub, SentryOptions options,
         string key, MeasurementUnit.Duration unit, IDictionary<string, string>? tags, int stackLevel)
     {
-        _metricHub = metricHub;
         _options = options;
         _metricAggregator = metricAggregator;
         _key = key;
         _unit = unit;
         _tags = tags;
         _stopwatch.Start();
 
-
-        _span = metricHub.StartSpan("metric.timing", key);
+        _span = metricHub.StartSpan(OperationName, key);
         if (tags is not null)
         {
             _span.SetTags(tags);
@@ -53,19 +52,23 @@ internal Timing(MetricAggregator metricAggregator, IMetricHub metricHub, SentryO
     public void Dispose()
     {
         _stopwatch.Stop();
+        DisposeInternal(_stopwatch.Elapsed);
+    }
 
+    internal void DisposeInternal(TimeSpan elapsed)
+    {
         try
         {
             var value = _unit switch
             {
-                MeasurementUnit.Duration.Week => _stopwatch.Elapsed.TotalDays / 7,
-                MeasurementUnit.Duration.Day => _stopwatch.Elapsed.TotalDays,
-                MeasurementUnit.Duration.Hour => _stopwatch.Elapsed.TotalHours,
-                MeasurementUnit.Duration.Minute => _stopwatch.Elapsed.TotalMinutes,
-                MeasurementUnit.Duration.Second => _stopwatch.Elapsed.TotalSeconds,
-                MeasurementUnit.Duration.Millisecond => _stopwatch.Elapsed.TotalMilliseconds,
-                MeasurementUnit.Duration.Microsecond => _stopwatch.Elapsed.TotalMilliseconds * 1000,
-                MeasurementUnit.Duration.Nanosecond => _stopwatch.Elapsed.TotalMilliseconds * 1000000,
+                MeasurementUnit.Duration.Week => elapsed.TotalDays / 7,
+                MeasurementUnit.Duration.Day => elapsed.TotalDays,
+                MeasurementUnit.Duration.Hour => elapsed.TotalHours,
+                MeasurementUnit.Duration.Minute => elapsed.TotalMinutes,
+                MeasurementUnit.Duration.Second => elapsed.TotalSeconds,
+                MeasurementUnit.Duration.Millisecond => elapsed.TotalMilliseconds,
+                MeasurementUnit.Duration.Microsecond => elapsed.TotalMilliseconds * 1000,
+                MeasurementUnit.Duration.Nanosecond => elapsed.TotalMilliseconds * 1000000,
                 _ => throw new ArgumentOutOfRangeException(nameof(_unit), _unit, null)
             };
             _metricAggregator.Timing(_key, value, _unit, _tags, _startTime);