revert: profiling changes

CHANGELOG.md

@@ -8,8 +8,6 @@
 - Native SIGSEGV errors resulting from managed NullReferenceExceptions are now suppressed on Android ([#3903](https://github.com/getsentry/sentry-dotnet/pull/3903))
 - OTel activities that are marked as not recorded are no longer sent to Sentry ([#3890](https://github.com/getsentry/sentry-dotnet/pull/3890))
 - Fixed envelopes with oversized attachments getting stuck in __processing ([#3938](https://github.com/getsentry/sentry-dotnet/pull/3938))
-- Unknown stack frames in profiles on .NET 8+ ([#3942](https://github.com/getsentry/sentry-dotnet/pull/3942))
-- Deduplicate profiling stack frames ([#3941](https://github.com/getsentry/sentry-dotnet/pull/3941))
 - Ignore null value on CocoaScopeObserver.SetTag ([#3948](https://github.com/getsentry/sentry-dotnet/pull/3948))
 
 ## 5.1.0

src/Sentry.Profiling/SampleProfileBuilder.cs

@@ -16,12 +16,8 @@ internal class SampleProfileBuilder
     // Output profile being built.
     public readonly SampleProfile Profile = new();
 
-    // A sparse array that maps from CodeAddressIndex to an index in the output Profile.frames.
-    private readonly Dictionary<int, int> _frameIndexesByCodeAddressIndex = new();
-
-    // A sparse array that maps from MethodIndex to an index in the output Profile.frames.
-    // This deduplicates frames that map to the same method but have a different CodeAddressIndex.
-    private readonly Dictionary<int, int> _frameIndexesByMethodIndex = new();
+    // A sparse array that maps from StackSourceFrameIndex to an index in the output Profile.frames.
+    private readonly Dictionary<int, int> _frameIndexes = new();
 
     // A dictionary from a CallStackIndex to an index in the output Profile.stacks.
     private readonly Dictionary<int, int> _stackIndexes = new();
@@ -89,14 +85,13 @@ private int AddStackTrace(CallStackIndex callstackIndex)
     {
         var key = (int)callstackIndex;
 
-        if (!_stackIndexes.TryGetValue(key, out var value))
+        if (!_stackIndexes.ContainsKey(key))
         {
             Profile.Stacks.Add(CreateStackTrace(callstackIndex));
-            value = Profile.Stacks.Count - 1;
-            _stackIndexes[key] = value;
+            _stackIndexes[key] = Profile.Stacks.Count - 1;
         }
 
-        return value;
+        return _stackIndexes[key];
     }
 
     private Internal.GrowableArray<int> CreateStackTrace(CallStackIndex callstackIndex)
@@ -121,71 +116,21 @@ private Internal.GrowableArray<int> CreateStackTrace(CallStackIndex callstackInd
         return stackTrace;
     }
 
-    private int PushNewFrame(SentryStackFrame frame)
-    {
-        Profile.Frames.Add(frame);
-        return Profile.Frames.Count - 1;
-    }
-
     /// <summary>
     /// Check if the frame is already stored in the output Profile, or adds it.
     /// </summary>
     /// <returns>The index to the output Profile frames array.</returns>
     private int AddStackFrame(CodeAddressIndex codeAddressIndex)
     {
-        if (_frameIndexesByCodeAddressIndex.TryGetValue((int)codeAddressIndex, out var value))
-        {
-            return value;
-        }
+        var key = (int)codeAddressIndex;
 
-        var methodIndex = _traceLog.CodeAddresses.MethodIndex(codeAddressIndex);
-        if (methodIndex != MethodIndex.Invalid)
-        {
-            value = AddStackFrame(methodIndex);
-            _frameIndexesByCodeAddressIndex[(int)codeAddressIndex] = value;
-            return value;
-        }
-
-        // Fall back if the method info is unknown, see more info on Symbol resolution in
-        // https://github.com/getsentry/perfview/blob/031250ffb4f9fcadb9263525d6c9f274be19ca51/src/PerfView/SupportFiles/UsersGuide.htm#L7745-L7784
-        if (_traceLog.CodeAddresses[codeAddressIndex] is { } codeAddressInfo)
+        if (!_frameIndexes.ContainsKey(key))
         {
-            var frame = new SentryStackFrame
-            {
-                InstructionAddress = (long?)codeAddressInfo.Address,
-                Module = codeAddressInfo.ModuleFile?.Name,
-            };
-            frame.ConfigureAppFrame(_options);
-
-            return _frameIndexesByCodeAddressIndex[(int)codeAddressIndex] = PushNewFrame(frame);
+            Profile.Frames.Add(CreateStackFrame(codeAddressIndex));
+            _frameIndexes[key] = Profile.Frames.Count - 1;
         }
 
-        // If all else fails, it's a completely unknown frame.
-        // TODO check this - maybe we would be able to resolve it later in the future?
-        return PushNewFrame(new SentryStackFrame { InApp = false });
-    }
-
-    /// <summary>
-    /// Check if the frame is already stored in the output Profile, or adds it.
-    /// </summary>
-    /// <returns>The index to the output Profile frames array.</returns>
-    private int AddStackFrame(MethodIndex methodIndex)
-    {
-        if (_frameIndexesByMethodIndex.TryGetValue((int)methodIndex, out var value))
-        {
-            return value;
-        }
-
-        var method = _traceLog.CodeAddresses.Methods[methodIndex];
-
-        var frame = new SentryStackFrame
-        {
-            Function = method.FullMethodName,
-            Module = method.MethodModuleFile?.Name
-        };
-        frame.ConfigureAppFrame(_options);
-
-        return _frameIndexesByMethodIndex[(int)methodIndex] = PushNewFrame(frame);
+        return _frameIndexes[key];
     }
 
     /// <summary>
@@ -196,17 +141,52 @@ private int AddThread(TraceThread thread)
     {
         var key = (int)thread.ThreadIndex;
 
-        if (!_threadIndexes.TryGetValue(key, out var value))
+        if (!_threadIndexes.ContainsKey(key))
         {
             Profile.Threads.Add(new()
             {
                 Name = thread.ThreadInfo ?? $"Thread {thread.ThreadID}",
             });
-            value = Profile.Threads.Count - 1;
-            _threadIndexes[key] = value;
+            _threadIndexes[key] = Profile.Threads.Count - 1;
             _downsampler.NewThreadAdded(_threadIndexes[key]);
         }
 
-        return value;
+        return _threadIndexes[key];
+    }
+
+    private SentryStackFrame CreateStackFrame(CodeAddressIndex codeAddressIndex)
+    {
+        var frame = new SentryStackFrame();
+
+        var methodIndex = _traceLog.CodeAddresses.MethodIndex(codeAddressIndex);
+        if (_traceLog.CodeAddresses.Methods[methodIndex] is { } method)
+        {
+            frame.Function = method.FullMethodName;
+
+            if (method.MethodModuleFile is { } moduleFile)
+            {
+                frame.Module = moduleFile.Name;
+            }
+
+            frame.ConfigureAppFrame(_options);
+        }
+        else
+        {
+            // Fall back if the method info is unknown, see more info on Symbol resolution in
+            // https://github.com/getsentry/perfview/blob/031250ffb4f9fcadb9263525d6c9f274be19ca51/src/PerfView/SupportFiles/UsersGuide.htm#L7745-L7784
+            frame.InstructionAddress = (long?)_traceLog.CodeAddresses.Address(codeAddressIndex);
+
+            if (_traceLog.CodeAddresses.ModuleFile(codeAddressIndex) is { } moduleFile)
+            {
+                frame.Module = moduleFile.Name;
+                frame.ConfigureAppFrame(_options);
+            }
+            else
+            {
+                frame.InApp = false;
+            }
+        }
+
+        return frame;
     }
 }

src/Sentry.Profiling/SampleProfilerSession.cs

@@ -12,32 +12,31 @@ namespace Sentry.Profiling;
 internal class SampleProfilerSession : IDisposable
 {
     private readonly EventPipeSession _session;
+    private readonly TraceLogEventSource _eventSource;
     private readonly SampleProfilerTraceEventParser _sampleEventParser;
     private readonly IDiagnosticLogger? _logger;
     private readonly SentryStopwatch _stopwatch;
     private bool _stopped = false;
-    private Task _processing;
 
-    private SampleProfilerSession(SentryStopwatch stopwatch, EventPipeSession session, TraceLogEventSource eventSource, Task processing, IDiagnosticLogger? logger)
+    private SampleProfilerSession(SentryStopwatch stopwatch, EventPipeSession session, TraceLogEventSource eventSource, IDiagnosticLogger? logger)
     {
         _session = session;
         _logger = logger;
-        EventSource = eventSource;
-        _sampleEventParser = new SampleProfilerTraceEventParser(EventSource);
+        _eventSource = eventSource;
+        _sampleEventParser = new SampleProfilerTraceEventParser(_eventSource);
         _stopwatch = stopwatch;
-        _processing = processing;
     }
 
     // Exposed only for benchmarks.
     internal static EventPipeProvider[] Providers = new[]
     {
-        new EventPipeProvider(ClrTraceEventParser.ProviderName, EventLevel.Verbose, (long) (
-            ClrTraceEventParser.Keywords.Jit
-            | ClrTraceEventParser.Keywords.NGen
-            | ClrTraceEventParser.Keywords.Loader
-            | ClrTraceEventParser.Keywords.Binder
-            | ClrTraceEventParser.Keywords.JittedMethodILToNativeMap
-            )),
+        // Note: all events we need issued by "DotNETRuntime" provider are at "EventLevel.Informational"
+        // see https://learn.microsoft.com/en-us/dotnet/fundamentals/diagnostics/runtime-events
+        // TODO replace Keywords.Default with a subset. Currently it is:
+        //   Default = GC | Type | GCHeapSurvivalAndMovement | Binder | Loader | Jit | NGen | SupressNGen
+        //                | StopEnumeration | Security | AppDomainResourceManagement | Exception | Threading | Contention | Stack | JittedMethodILToNativeMap
+        //                | ThreadTransfer | GCHeapAndTypeNames | Codesymbols | Compilation,
+        new EventPipeProvider(ClrTraceEventParser.ProviderName, EventLevel.Informational, (long) ClrTraceEventParser.Keywords.Default),
         new EventPipeProvider(SampleProfilerTraceEventParser.ProviderName, EventLevel.Informational),
         // new EventPipeProvider(TplEtwProviderTraceEventParser.ProviderName, EventLevel.Informational, (long) TplEtwProviderTraceEventParser.Keywords.Default)
     };
@@ -47,14 +46,11 @@ private SampleProfilerSession(SentryStopwatch stopwatch, EventPipeSession sessio
     // need a large buffer if we're connecting righ away. Leaving it too large increases app memory usage.
     internal static int CircularBufferMB = 16;
 
-    // Exposed for tests
-    internal TraceLogEventSource EventSource { get; }
-
     public SampleProfilerTraceEventParser SampleEventParser => _sampleEventParser;
 
     public TimeSpan Elapsed => _stopwatch.Elapsed;
 
-    public TraceLog TraceLog => EventSource.TraceLog;
+    public TraceLog TraceLog => _eventSource.TraceLog;
 
     // default is false, set 1 for true.
     private static int _throwOnNextStartupForTests = 0;
@@ -90,7 +86,7 @@ public static SampleProfilerSession StartNew(IDiagnosticLogger? logger = null)
             var eventSource = TraceLog.CreateFromEventPipeSession(session, TraceLog.EventPipeRundownConfiguration.Enable(client));
 
             // Process() blocks until the session is stopped so we need to run it on a separate thread.
-            var processing = Task.Factory.StartNew(eventSource.Process, TaskCreationOptions.LongRunning)
+            Task.Factory.StartNew(eventSource.Process, TaskCreationOptions.LongRunning)
                 .ContinueWith(_ =>
                 {
                     if (_.Exception?.InnerException is { } e)
@@ -99,7 +95,7 @@ public static SampleProfilerSession StartNew(IDiagnosticLogger? logger = null)
                     }
                 }, TaskContinuationOptions.OnlyOnFaulted);
 
-            return new SampleProfilerSession(stopWatch, session, eventSource, processing, logger);
+            return new SampleProfilerSession(stopWatch, session, eventSource, logger);
         }
         catch (Exception ex)
         {
@@ -112,15 +108,15 @@ public async Task WaitForFirstEventAsync(CancellationToken cancellationToken = d
     {
         var tcs = new TaskCompletionSource();
         var cb = (TraceEvent _) => { tcs.TrySetResult(); };
-        EventSource.AllEvents += cb;
+        _eventSource.AllEvents += cb;
         try
         {
             // Wait for the first event to be processed.
             await tcs.Task.WaitAsync(cancellationToken).ConfigureAwait(false);
         }
         finally
         {
-            EventSource.AllEvents -= cb;
+            _eventSource.AllEvents -= cb;
         }
     }
 
@@ -132,9 +128,8 @@ public void Stop()
             {
                 _stopped = true;
                 _session.Stop();
-                _processing.Wait();
                 _session.Dispose();
-                EventSource.Dispose();
+                _eventSource.Dispose();
             }
             catch (Exception ex)
             {

src/Sentry.Profiling/Sentry.Profiling.csproj

@@ -14,7 +14,7 @@
 
   <ItemGroup>
     <ProjectReference Include="..\..\src\Sentry\Sentry.csproj" />
-    <PackageReference Include="Microsoft.Diagnostics.NETCore.Client" Version="0.2.553101" />
+    <PackageReference Include="Microsoft.Diagnostics.NETCore.Client" Version="0.2.510501" />
     <!--  This triggers the build of this project and its dependencies. We don't need all of them but this is the easiest way -->
     <!--  to make sure the project builds/cleans etc in tandem with this. Packaging copies the 2 DLLs we need below -->
     <ProjectReference Include="../../modules/perfview/src/TraceEvent/TraceEvent.csproj" PrivateAssets="all" />

test/Sentry.Profiling.Tests/SamplingTransactionProfilerTests.cs

@@ -1,5 +1,4 @@
-using Microsoft.Diagnostics.Tracing;
-using Microsoft.Diagnostics.Tracing.Parsers.Clr;
+using System.IO.Abstractions.TestingHelpers;
 using Sentry.Internal.Http;
 
 namespace Sentry.Profiling.Tests;
@@ -181,40 +180,6 @@ public async Task Profiler_AfterTimeout_Stops()
         }
     }
 
-    [SkippableFact]
-    public async Task EventPipeSession_ReceivesExpectedCLREvents()
-    {
-        Skip.If(TestEnvironment.IsGitHubActions, "Flaky on CI");
-
-        var session = SampleProfilerSession.StartNew(_testOutputLogger);
-        using (session)
-        {
-            var eventsReceived = new HashSet<string>();
-            session!.EventSource.Clr.All += (TraceEvent ev) => eventsReceived.Add(ev.EventName);
-
-            var loadedMethods = new HashSet<string>();
-            session!.EventSource.Clr.MethodLoadVerbose += (MethodLoadUnloadVerboseTraceData ev) => loadedMethods.Add(ev.MethodName);
-
-            await session.WaitForFirstEventAsync(CancellationToken.None);
-            var limitMs = 50;
-            var sut = new SamplingTransactionProfiler(_testSentryOptions, session, limitMs, CancellationToken.None);
-            RunForMs(limitMs * 5);
-            MethodToBeLoaded(100);
-            RunForMs(limitMs * 5);
-            sut.Finish();
-
-            Assert.Contains("Method/LoadVerbose", eventsReceived);
-            Assert.Contains("Method/ILToNativeMap", eventsReceived);
-
-            Assert.Contains("MethodToBeLoaded", loadedMethods);
-        }
-    }
-
-    private static long MethodToBeLoaded(int n)
-    {
-        return -n;
-    }
-
     [SkippableTheory]
     [InlineData(true)]
     [InlineData(false)]