From 201247af4449d18077ca7a76894a02b1110be7b7 Mon Sep 17 00:00:00 2001 From: Andrew Lock Date: Thu, 8 Apr 2021 09:36:26 +0100 Subject: [PATCH 1/2] Simplify moving average calculator (#1350) * Simplify moving average calculator Was previously going a bit overboard with keeping things atomic, when in reality we only need the current buckets and keep rate to be atomic * Update src/Datadog.Trace/Agent/MovingAverageKeepRateCalculator.cs Co-authored-by: Lucas Pimentel-Ordyna * Apply Kevin-level optimisations Co-authored-by: Kevin Gosse Co-authored-by: Lucas Pimentel-Ordyna Co-authored-by: Kevin Gosse --- .../Agent/MovingAverageKeepRateCalculator.cs | 53 +++++++------------ 1 file changed, 18 insertions(+), 35 deletions(-) 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); - } } } From e2a5e12548bb396f85c54ebf9782fe915eab6053 Mon Sep 17 00:00:00 2001 From: Andrew Lock Date: Thu, 8 Apr 2021 15:13:27 +0100 Subject: [PATCH 2/2] Update Uri cleaning algorithm to be more aggressive (#1327) * Update Uri cleaning algorithm to be more aggressive Replaces any hex-ish (alpha or `-`) string with `?`. If it's numbers and dashes only, always replaces If it contains alpha, must be at least 16 chars long and contain a number Added in back-compat way, that avoids changing internal (public) APIs * Conditionally enable the new URI cleaning algorithm in ASP.NET Enabled when the new route names flag is enabled Added a note that we're currently using the _legacy_ algorithm still in the generic scope factory for _outbound_ requests. We may want to update this, but wasn't sure if we should do it under the same flag, as it's _kinda_ unrelated * Simplify implementation * Mark older UriHelpers methods Obsolete (kept for back compat) Add documentation to CleanUri method Update tests to use non-obsolete methods * Use new Uri cleaning in AspNetCore resources when feature flag enabled * Remove the feature-flag behaviour i.e. always use the new Id replacement algorithm We decided to take this approach as it's simpler, and in practice the cleaning algorithm should only be used when there's a 404 anyway. If customers find the new algorithm is causing them problems, opting in to the new route-based resource names should fix it. The one other place this cleaning algorithm is used is in the ScopeFactory for outbound HTTP requests. There's no workaround there, but the chances of issues are small, so we're ok with the tradeoff for simplicity and performance --- src/Datadog.Trace/Util/UriHelpers.cs | 123 +++++++++--------- .../Util/UriHelpersTests.cs | 17 ++- 2 files changed, 71 insertions(+), 69 deletions(-) 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)