Skip to content

Commit

Permalink
revert: profiling changes (#3962)
Browse files Browse the repository at this point in the history 
* Revert "Skip flaky test (EventPipeSession_ReceivesExpectedCLREvents) (#3949)"

This reverts commit 50e5232.

* Revert "fix: net8 unknown stack trace methods for JIT methods (#3942)"

This reverts commit d197cb2.

* Revert "chore: profiling improvements (#3941)"

This reverts commit bea672a.

* chore: revert changelog

* Verify tests

* Windows verify tests

---------

Co-authored-by: James Crosswell <[email protected]>
  • Loading branch information
@vaind @jamescrosswell
vaind and jamescrosswell authored Feb 13, 2025
1 parent ee62bb9 commit bb10659
Show file tree
Hide file tree
Showing 18 changed files with 386 additions and 303 deletions.
118 changes: 49 additions & 69 deletions src/Sentry.Profiling/SampleProfileBuilder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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)
Expand All @@ -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>
Expand All @@ -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;
}
}
39 changes: 17 additions & 22 deletions src/Sentry.Profiling/SampleProfilerSession.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
};
Expand All @@ -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;
Expand Down Expand Up @@ -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)
Expand All @@ -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)
{
Expand All @@ -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;
}
}

Expand All @@ -132,9 +128,8 @@ public void Stop()
{
_stopped = true;
_session.Stop();
_processing.Wait();
_session.Dispose();
EventSource.Dispose();
_eventSource.Dispose();
}
catch (Exception ex)
{
Expand Down
2 changes: 1 addition & 1 deletion src/Sentry.Profiling/Sentry.Profiling.csproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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" />
Expand Down
4 changes: 0 additions & 4 deletions test/Sentry.AspNetCore.Tests/WebIntegrationTests.Versioning.DotNet8_0.verified.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,6 @@
Category: Microsoft.AspNetCore.Routing.EndpointMiddleware
}
],
Extra: {
http.request.method: GET,
http.response.status_code: 200
},
Tags: {
ActionId: Guid_2,
ActionName: Sentry.AspNetCore.Tests.WebIntegrationTests+VersionController.Method (Sentry.AspNetCore.Tests),
Expand Down
4 changes: 0 additions & 4 deletions test/Sentry.AspNetCore.Tests/WebIntegrationTests.Versioning.DotNet9_0.verified.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,6 @@
Category: Microsoft.AspNetCore.Routing.EndpointMiddleware
}
],
Extra: {
http.request.method: GET,
http.response.status_code: 200
},
Tags: {
ActionId: Guid_2,
ActionName: Sentry.AspNetCore.Tests.WebIntegrationTests+VersionController.Method (Sentry.AspNetCore.Tests),
Expand Down
4 changes: 2 additions & 2 deletions ...ry.DiagnosticSource.IntegrationTests/SqlListenerTests.LoggingAsync.DotNet8_0.verified.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
Description: SqlListenerTests.verify_LoggingAsync,
Status: Ok,
IsSampled: true,
Extra: {
Data: {
bytes_sent : 376,
db.connection_id: Guid_2,
db.name: SqlListenerTests.verify_LoggingAsync,
Expand All @@ -49,7 +49,7 @@ VALUES (@p0);
,
Status: Ok,
IsSampled: true,
Extra: {
Data: {
db.connection_id: Guid_2,
db.name: SqlListenerTests.verify_LoggingAsync,
db.operation_id: Guid_4,
Expand Down
4 changes: 2 additions & 2 deletions ...ry.DiagnosticSource.IntegrationTests/SqlListenerTests.LoggingAsync.DotNet9_0.verified.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
Description: SqlListenerTests.verify_LoggingAsync,
Status: Ok,
IsSampled: true,
Extra: {
Data: {
bytes_sent : 376,
db.connection_id: Guid_2,
db.name: SqlListenerTests.verify_LoggingAsync,
Expand All @@ -49,7 +49,7 @@ VALUES (@p0);
,
Status: Ok,
IsSampled: true,
Extra: {
Data: {
db.connection_id: Guid_2,
db.name: SqlListenerTests.verify_LoggingAsync,
db.operation_id: Guid_4,
Expand Down
8 changes: 4 additions & 4 deletions ....DiagnosticSource.IntegrationTests/SqlListenerTests.RecordsEfAsync.DotNet8_0.verified.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@
Description: SqlListenerTests.verify_RecordsEfAsync,
Status: Ok,
IsSampled: true,
Extra: {
Data: {
bytes_received: 225,
bytes_sent : 570,
db.connection_id: Guid_2,
Expand All @@ -84,7 +84,7 @@ VALUES (@p0);
,
Status: Ok,
IsSampled: true,
Extra: {
Data: {
db.connection_id: Guid_2,
db.name: SqlListenerTests.verify_RecordsEfAsync,
db.operation_id: Guid_4,
Expand All @@ -97,7 +97,7 @@ VALUES (@p0);
Description: 'DbSet<TestEntity>()',
Status: Ok,
IsSampled: true,
Extra: {
Data: {
db.system: mssql
}
},
Expand All @@ -109,7 +109,7 @@ SELECT [t].[Id], [t].[Property]
FROM [TestEntities] AS [t],
Status: Ok,
IsSampled: true,
Extra: {
Data: {
db.connection_id: Guid_2,
db.name: SqlListenerTests.verify_RecordsEfAsync,
db.operation_id: Guid_5,
Expand Down
Loading

0 comments on commit bb10659

Please sign in to comment.