diff --git a/src/Datadog.Trace/Agent/MovingAverageKeepRateCalculator.cs b/src/Datadog.Trace/Agent/MovingAverageKeepRateCalculator.cs index 55df23eb15d6..d8746a5a169f 100644 --- a/src/Datadog.Trace/Agent/MovingAverageKeepRateCalculator.cs +++ b/src/Datadog.Trace/Agent/MovingAverageKeepRateCalculator.cs @@ -26,7 +26,9 @@ internal class MovingAverageKeepRateCalculator : IKeepRateCalculator private readonly TaskCompletionSource _processExit = new TaskCompletionSource(); private int _index = 0; - private long _totals = 0; + private ulong _sumDrops = 0; + private ulong _sumCreated = 0; + private double _keepRate = 0; private long _latestDrops = 0; private long _latestKeeps = 0; @@ -84,14 +86,8 @@ public void IncrementDrops(int count) /// public double GetKeepRate() { - var totals = Interlocked.Read(ref _totals); - UnpackBits(totals, out var totalDropped, out var totalCreated); - if (totalCreated == 0) - { - return 0; - } - - return 1 - ((double)totalDropped / totalCreated); + // Essentially Interlock.Read(double) + return Volatile.Read(ref _keepRate); } /// @@ -111,21 +107,23 @@ internal void UpdateBucket() var previousDropped = _dropped[index]; var previousCreated = _created[index]; - var latestDropped = Math.Min(Interlocked.Exchange(ref _latestDrops, 0), uint.MaxValue); - var latestCreated = Math.Min(Interlocked.Exchange(ref _latestKeeps, 0) + latestDropped, uint.MaxValue); + // Cap at uint.MaxValue events. Very unlikely to reach this value! + var latestDropped = (uint)Math.Min(Interlocked.Exchange(ref _latestDrops, 0), uint.MaxValue); + var latestCreated = (uint)Math.Min(Interlocked.Exchange(ref _latestKeeps, 0) + latestDropped, uint.MaxValue); - var totals = _totals; - UnpackBits(totals, out var previousTotalDropped, out var previousTotalCreated); + var newSumDropped = _sumDrops - previousDropped + latestDropped; + var newSumCreated = _sumCreated - previousCreated + latestCreated; - var newTotalDropped = (uint)Math.Min(((long)previousTotalDropped) - previousDropped + latestDropped, uint.MaxValue); - var newTotalCreated = (uint)Math.Min(((long)previousTotalCreated) - previousCreated + latestCreated, uint.MaxValue); + var keepRate = newSumCreated == 0 + ? 0 + : 1 - ((double)newSumDropped / newSumCreated); - // Pack the totals into a single value we can read and write atomically - var packedTotal = PackBits(newTotalDropped, newTotalCreated); - Interlocked.Exchange(ref _totals, packedTotal); + Volatile.Write(ref _keepRate, keepRate); - _dropped[index] = (uint)latestDropped; - _created[index] = (uint)latestCreated; + _sumDrops = newSumDropped; + _sumCreated = newSumCreated; + _dropped[index] = latestDropped; + _created[index] = latestCreated; _index = (index + 1) % _windowSize; } @@ -156,20 +154,5 @@ await Task.WhenAny( #endif } } - - [MethodImpl(MethodImplOptions.AggressiveInlining)] - private long PackBits(uint hits, uint total) - { - long result = hits; - result = result << 32; - return result | (long)total; - } - - [MethodImpl(MethodImplOptions.AggressiveInlining)] - private void UnpackBits(long bits, out uint hits, out uint total) - { - hits = (uint)(bits >> 32); - total = (uint)(bits & 0xffffffffL); - } } } diff --git a/src/Datadog.Trace/Util/UriHelpers.cs b/src/Datadog.Trace/Util/UriHelpers.cs index d0f2a4bb5355..677fe96ee855 100644 --- a/src/Datadog.Trace/Util/UriHelpers.cs +++ b/src/Datadog.Trace/Util/UriHelpers.cs @@ -1,10 +1,16 @@ using System; -using System.Runtime.CompilerServices; namespace Datadog.Trace.Util { internal static class UriHelpers { + /// + /// Remove the querystring, user information, and fragment from a URL. + /// Optionally reduce cardinality by replacing segments that look like IDs with ?. + /// + /// The URI to clean + /// Should the scheme be removed? + /// Should IDs be replaced with ? public static string CleanUri(Uri uri, bool removeScheme, bool tryRemoveIds) { var path = tryRemoveIds ? GetCleanUriPath(uri.AbsolutePath) : uri.AbsolutePath; @@ -22,17 +28,17 @@ public static string CleanUri(Uri uri, bool removeScheme, bool tryRemoveIds) } [Obsolete("This method is deprecated and will be removed. Use GetCleanUriPath() instead. " + - "Kept for backwards compatability where there is a version mismatch between manual and automatic instrumentation")] + "Kept for backwards compatibility where there is a version mismatch between manual and automatic instrumentation")] public static string GetRelativeUrl(Uri uri, bool tryRemoveIds) => GetRelativeUrl(uri.AbsolutePath, tryRemoveIds); [Obsolete("This method is deprecated and will be removed. Use GetCleanUriPath() instead. " + - "Kept for backwards compatability where there is a version mismatch between manual and automatic instrumentation")] + "Kept for backwards compatibility where there is a version mismatch between manual and automatic instrumentation")] public static string GetRelativeUrl(string uri, bool tryRemoveIds) => tryRemoveIds ? GetCleanUriPath(uri) : uri; [Obsolete("This method is deprecated and will be removed. Use GetCleanUriPath() instead. " + - "Kept for backwards compatability where there is a version mismatch between manual and automatic instrumentation")] + "Kept for backwards compatibility where there is a version mismatch between manual and automatic instrumentation")] public static string CleanUriSegment(string absolutePath) => GetCleanUriPath(absolutePath); @@ -48,33 +54,34 @@ public static string GetCleanUriPath(string absolutePath) return absolutePath; } -#if NETCOREAPP - ReadOnlySpan absPath = absolutePath.AsSpan(); - // Sanitized url will be at worse as long as the original - var sb = StringBuilderCache.Acquire(absPath.Length); + var sb = StringBuilderCache.Acquire(absolutePath.Length); int previousIndex = 0; int index; + int segmentLength; do { - ReadOnlySpan nStart = absPath.Slice(previousIndex); - index = nStart.IndexOf('/'); - ReadOnlySpan segment = index == -1 ? nStart : nStart.Slice(0, index); - - // replace path segments that look like numbers or guid - // GUID format N "d85b1407351d4694939203acc5870eb1" length: 32 - // GUID format D "d85b1407-351d-4694-9392-03acc5870eb1" length: 36 with dashes in indices 8, 13, 18 and 23. - if (long.TryParse(segment, out _) || - (segment.Length == 32 && IsAGuid(segment, "N")) || - (segment.Length == 36 && IsAGuid(segment, "D"))) + index = absolutePath.IndexOf('/', previousIndex); + + if (index == -1) + { + // Last segment + segmentLength = absolutePath.Length - previousIndex; + } + else + { + segmentLength = index - previousIndex; + } + + if (IsIdentifierSegment(absolutePath, previousIndex, segmentLength)) { sb.Append('?'); } else { - sb.Append(segment); + sb.Append(absolutePath, previousIndex, segmentLength); } if (index != -1) @@ -82,65 +89,51 @@ public static string GetCleanUriPath(string absolutePath) sb.Append('/'); } - previousIndex += index + 1; + previousIndex = index + 1; } while (index != -1); return StringBuilderCache.GetStringAndRelease(sb); -#else - // Sanitized url will be at worse as long as the original - var sb = StringBuilderCache.Acquire(absolutePath.Length); - - int previousIndex = 0; - int index = 0; + } - do + private static bool IsIdentifierSegment(string absolutePath, int startIndex, int segmentLength) + { + if (segmentLength == 0) { - index = absolutePath.IndexOf('/', previousIndex); - - string segment; - - if (index == -1) - { - // Last segment - segment = absolutePath.Substring(previousIndex); - } - else - { - segment = absolutePath.Substring(previousIndex, index - previousIndex); - } + return false; + } - // replace path segments that look like numbers or guid - // GUID format N "d85b1407351d4694939203acc5870eb1" length: 32 - // GUID format D "d85b1407-351d-4694-9392-03acc5870eb1" length: 36 with dashes in indices 8, 13, 18 and 23. - if (long.TryParse(segment, out _) || - (segment.Length == 32 && IsAGuid(segment, "N")) || - (segment.Length == 36 && IsAGuid(segment, "D"))) - { - segment = "?"; - } + int lastIndex = startIndex + segmentLength; + var containsNumber = false; - sb.Append(segment); + for (int index = startIndex; index < lastIndex && index < absolutePath.Length; index++) + { + char c = absolutePath[index]; - if (index != -1) + switch (c) { - sb.Append("/"); + case >= '0' and <= '9': + containsNumber = true; + continue; + case >= 'a' and <= 'f': + case >= 'A' and <= 'F': + if (segmentLength < 16) + { + // don't be too aggressive replacing + // short hex segments like "/a" or "/cab", + // they are likely not ids + return false; + } + + continue; + case '-': + continue; + default: + return false; } - - previousIndex = index + 1; } - while (index != -1); - return StringBuilderCache.GetStringAndRelease(sb); -#endif + return containsNumber; } - - [MethodImpl(MethodImplOptions.NoInlining)] - private static bool IsAGuid(string segment, string format) => Guid.TryParseExact(segment, format, out _); - -#if NETCOREAPP - [MethodImpl(MethodImplOptions.NoInlining)] - private static bool IsAGuid(ReadOnlySpan segment, string format) => Guid.TryParseExact(segment, format, out _); -#endif } } diff --git a/test/Datadog.Trace.Tests/Util/UriHelpersTests.cs b/test/Datadog.Trace.Tests/Util/UriHelpersTests.cs index 2bac5c004cdb..3bc545d9a247 100644 --- a/test/Datadog.Trace.Tests/Util/UriHelpersTests.cs +++ b/test/Datadog.Trace.Tests/Util/UriHelpersTests.cs @@ -9,10 +9,19 @@ public class UriHelpersTests [InlineData("/", "/")] [InlineData("/controller/action/b37855d4bae34bd3b3357fc554ad334e", "/controller/action/?")] [InlineData("/controller/action/14bb2eed-34f0-4aa2-b2c3-09c0e2166d4d", "/controller/action/?")] - [InlineData("/controller/action/14bb2eed-34f0X4aa2-b2c3-09c0e2166d4d", "/controller/action/14bb2eed-34f0X4aa2-b2c3-09c0e2166d4d")] - [InlineData("/controller/action/14bb2eed-34f0-4aa2Xb2c3-09c0e2166d4d", "/controller/action/14bb2eed-34f0-4aa2Xb2c3-09c0e2166d4d")] - [InlineData("/controller/action/14bb2eed-34f0A4aa2Bb2c3C09c0e2166d4d", "/controller/action/14bb2eed-34f0A4aa2Bb2c3C09c0e2166d4d")] - [InlineData("/DataDog/dd-trace-dotnet/blob/e2d83dec7d6862d4181937776ddaf72819e291ce/src/Datadog.Trace/Util/UriHelpers.cs", "/DataDog/dd-trace-dotnet/blob/e2d83dec7d6862d4181937776ddaf72819e291ce/src/Datadog.Trace/Util/UriHelpers.cs")] + [InlineData("/controller/action/14bb2eed-34f0X4aa2-b2c3-09c0e2166d4d", "/controller/action/14bb2eed-34f0X4aa2-b2c3-09c0e2166d4d")] // contains non-hex letters + [InlineData("/controller/action/14bb2eed-34f0-4aa2Xb2c3-09c0e2166d4d", "/controller/action/14bb2eed-34f0-4aa2Xb2c3-09c0e2166d4d")] // contains non-hex letters + [InlineData("/controller/action/14bb2eed-34f0A4aa2Bb2c3C09c0e2166d4d", "/controller/action/?")] + [InlineData("/controller/action/12345678901234567890123456789012345678901234567890", "/controller/action/?")] + [InlineData("/controller/action/eeeee123", "/controller/action/eeeee123")] // Too short + [InlineData("/controller/action/0123456789ABCDE", "/controller/action/0123456789ABCDE")] // Too short + [InlineData("/controller/action/01234567890ABCDEFGH", "/controller/action/01234567890ABCDEFGH")] // Contains non-hex letters + [InlineData("/controller/action/0123456789ABCDEF", "/controller/action/?")] + [InlineData("/controller/action/0123456789ABCDEF0", "/controller/action/?")] + [InlineData("/controller/action/01234567_89ABCDEF", "/controller/action/01234567_89ABCDEF")] // only hyphen '-' allowed other than hex + [InlineData("/controller/action/123-456-789", "/controller/action/?")] + [InlineData("/controller/action/eeeeeeeeeeeeeeeee", "/controller/action/eeeeeeeeeeeeeeeee")] // No numbers + [InlineData("/DataDog/dd-trace-dotnet/blob/e2d83dec7d6862d4181937776ddaf72819e291ce/src/Datadog.Trace/Util/UriHelpers.cs", "/DataDog/dd-trace-dotnet/blob/?/src/Datadog.Trace/Util/UriHelpers.cs")] [InlineData("/controller/action/2022", "/controller/action/?")] [InlineData("/controller/action/", "/controller/action/")] public void GetCleanUriPath_ShouldRemoveIdsFromPaths(string url, string expected)