.NET profiler improvements & test fixes #2968

vaind · 2023-12-13T12:25:34Z

Make profiler startup non-blocking by default, with an option to block up to the given timeout.
Tune tests to reduce flakiness
null-check on TraceEvent thread()
if option profiling sampling is on, but no integration was added or ios aot, diagnostic warn user telling them how to fix.

#skip-changelog

bruno-garcia · 2023-12-20T16:42:54Z

src/Sentry.Profiling/SampleProfilerSession.cs


-        return new SampleProfilerSession(stopWatch, session, eventSource, logger);
+            // Process() blocks until the session is stopped so we need to run it on a separate thread.
+            Task.Factory.StartNew(eventSource.Process, TaskCreationOptions.LongRunning);


Do we need to observe any exception of this and log? I think right this it'll end up on UnobservedExceptionHandler

it's logged below on line 74

I've added a test case Profiler_ThrowingOnSessionStartup_DoesntBreakSentryInit to make sure an exception thrown inside profiler session init doesn't break Sentry init

Sorry I don't get it (line 74 is above this). But I mean if the callback running concurrently throws (eventSource.Process, if that throws? We can ContinuesWith at the end of StartNew for example, or have a callback with a trycatch before we call eentSource.Process?

Oh, I get what you mean now. I was confused at first because this bit of code hasn't actually changed so I thought you're talking about sth else.

In case Process() throws: yes, I think it'd get handled by UnobservedExceptionHandler.

I've added ContinueWith()

src/Sentry.Profiling/SampleProfilerSession.cs

src/Sentry.Profiling/SamplingTransactionProfiler.cs

src/Sentry.Profiling/SamplingTransactionProfilerFactory.cs

src/Sentry.Profiling/ProfilingIntegration.cs

src/Sentry.Profiling/SampleProfilerSession.cs

bruno-garcia · 2023-12-22T00:30:22Z

src/Sentry.Profiling/SampleProfilerSession.cs


-        return new SampleProfilerSession(stopWatch, session, eventSource, logger);
+            // Process() blocks until the session is stopped so we need to run it on a separate thread.
+            Task.Factory.StartNew(eventSource.Process, TaskCreationOptions.LongRunning);


Sorry I don't get it (line 74 is above this). But I mean if the callback running concurrently throws (eventSource.Process, if that throws? We can ContinuesWith at the end of StartNew for example, or have a callback with a trycatch before we call eentSource.Process?

bruno-garcia · 2023-12-22T00:37:38Z

test/Sentry.Profiling.Tests/SamplingTransactionProfilerTests.cs

-            var frame = profile.Frames[profileToVerify.Stacks[0][i]];
-            frame.Module = frame.Module.Replace("System.Private.CoreLib.il", "System.Private.CoreLib");
-            profileToVerify.Frames.Add(frame);
+            factory._sessionTask.Wait(60_000);


Waits up to a minute for one test and still times out? This is def far from ideal. Skipping in CI is OK but is this going to be flaky locally?
Hard to believe it can take more than 1 minute to startup

test/Sentry.Profiling.Tests/SamplingTransactionProfilerTests.cs

src/Sentry.Profiling/SampleProfileBuilder.cs

src/Sentry.Profiling/SampleProfilerSession.cs

bruno-garcia · 2023-12-22T13:09:10Z

src/Sentry.Profiling/SampleProfilerSession.cs

+                {
+                    if (_.Exception?.InnerException is { } e)
+                    {
+                        logger?.LogWarning("Error during sampler profiler EventPipeSession processing.", e);


Will conflict now I imagine, since we merged ur PR reordering

Actually, I should have looked for usages on the other PR too... All the usages passing an exception got picked up by the generic method so they didn't fail. I've fixed it in this branch with hope it gets merged soon also those fixes are unrelated. If it doesn't get merged, I can cherry-pick the commit to a new PR.

bruno-garcia · 2023-12-22T13:12:58Z

test/Sentry.Profiling.Tests/SamplingTransactionProfilerTests.cs

+            }
+            catch (Exception e)
+            {
+                _testOutputLogger.LogWarning("Caught an exception in a test block that is allowed to fail when in CI.", e);


Do we know what this exception will be? That would allow us to validate this in CI (timeout is OK but else it's not?)

Co-authored-by: Bruno Garcia <[email protected]>

* Set in_foreground app context * Update Changelog * Check OS * Update verify tests * Update remaining verify tests * Remove redundant code. * Update app context tests * Catch IsOSPlatform failure * Update CHANGELOG.md Co-authored-by: Bruno Garcia <[email protected]> * Refactor active window check. * Add a comment --------- Co-authored-by: James Crosswell <[email protected]> Co-authored-by: Bruno Garcia <[email protected]>

* Attach screenshot * Add AttachScreenshots option default value test * Update changelog * Update changelog * Add AttachScreenshots to bindable options. * Add AttachScreenshots to verify * Fix verify test * Remove default for AttachScreenshots * Use hint for screenshot attachments * Update CHANGELOG.md Co-authored-by: Bruno Garcia <[email protected]> * Add screenshot processor on options * Add comment, skip on Windows * Move try/catch outside #if * Don't add screenshot processor to scope. * Log if screenshot cannot be captured. * Add tests * Find envelope by id. * Assert attachment item only on mobile. * Initialize ActivityStateManager * Capture screen on UI thread. * Init platform * Move init to MainApplication * Use extension methods on SentryOptions for logging Co-authored-by: Bruno Garcia <[email protected]> * Add missing using * Init platform in lifecycle events * Remove MainApplication from device tests * Write to logcat * Rename Property * Pass AttachScreenshot to native sdk options. * Move CaptureFailed outside of #Debug * Remove leftover code * Remove Logcat logging * Update CHANGELOG.md Co-authored-by: Bruno Garcia <[email protected]> * Use blocking call on main thread * Set debug mode to make logs work * Update src/Sentry.Maui/SentryMauiOptions.cs Co-authored-by: Bruno Garcia <[email protected]> * Remove try catch from screen capture * Rename file to match class name --------- Co-authored-by: Bruno Garcia <[email protected]>

Co-authored-by: GitHub <[email protected]>

update profiler tests

c0009f9

vaind requested review from bitsandfoxes, jamescrosswell and Giorgi as code owners December 13, 2023 12:25

vaind marked this pull request as draft December 13, 2023 19:47

logging

c39d96c

vaind changed the title ~~.NET profiler fixes~~ .NET profiler test fixes Dec 14, 2023

vaind added 14 commits December 14, 2023 15:28

improve profiler session startup

aaba231

make profiling startup async

00c75de

introduce profiler startup timeout

c3d413a

improve tests

812874c

speedup tests

f7fbfc0

debug assert

47c826b

Merge branch 'main' into chore/profiling-fixes

d86affa

improvements

0d3576b

improvements

fb5c9fd

add async startup test

5e99a61

improve tests

6c12446

Merge branch 'main' into chore/profiling-fixes

2703013

deflake profiler tests

c5e8513

custom test params for CI

ce98216

vaind changed the title ~~.NET profiler test fixes~~ .NET profiler improvements & test fixes Dec 18, 2023

fixups

7e87cb5

vaind force-pushed the chore/profiling-fixes branch from e3b0d8d to 7e87cb5 Compare December 18, 2023 19:54

vaind added 4 commits December 18, 2023 20:58

Remove SparseScalarArray usage

85c4c33

Merge branch 'main' into chore/profiling-fixes

6d9e927

warn if profiling is misconfigured

b4b913e

fixup

1924359

bruno-garcia reviewed Dec 20, 2023

View reviewed changes

cleanup CreateStackFrame()

ae3eeaf

vaind added 2 commits December 21, 2023 15:09

fix tests

c99df63

catch init exceptions

a929523

vaind force-pushed the chore/profiling-fixes branch from 81f7ae6 to a929523 Compare December 21, 2023 16:08

vaind added 2 commits December 21, 2023 19:40

try to fix CI

03b586d

avoid logging errors multiple times

cb2bf9a

bruno-garcia reviewed Dec 22, 2023

View reviewed changes

vaind mentioned this pull request Dec 22, 2023

consolidate exception position in logger callbacks #2986

Closed

vaind added 3 commits December 22, 2023 08:18

review changes

7102696

allow select profiler assertions to skip the test in CI.

44fec5a

cleanups

8a75ab3

vaind marked this pull request as ready for review December 22, 2023 12:18

bruno-garcia reviewed Dec 22, 2023

View reviewed changes

bruno-garcia removed the request for review from Giorgi December 22, 2023 13:21

vaind and others added 9 commits December 22, 2023 16:39

Update src/Sentry.Profiling/SampleProfileBuilder.cs

a826783

Co-authored-by: Bruno Garcia <[email protected]>

Default User.IpAddress to {{auto}} (#2981)

cc2a691

chore(deps): update CLI to v2.23.1 (#2985)

7b13d1d

Co-authored-by: GitHub <[email protected]>

Remove Giorgi from CODEOWNERS (#2989)

91467a0

chore: align LogWarning with LogError (#2987)

12971e3

fixup LogWarning usages

f2c64b1

use Environment.ProcessId

be2c18c

vaind force-pushed the chore/profiling-fixes branch from 3277d18 to be2c18c Compare December 22, 2023 15:57

vaind added 2 commits December 22, 2023 17:53

ci test fixups

ba9a511

Merge remote-tracking branch 'origin/main' into chore/profiling-fixes

1290992

bruno-garcia approved these changes Dec 22, 2023

View reviewed changes

bruno-garcia merged commit b734248 into main Dec 22, 2023
18 checks passed

bruno-garcia deleted the chore/profiling-fixes branch December 22, 2023 22:36

vaind added a commit that referenced this pull request Dec 27, 2023

.NET profiler improvements & test fixes (#2968)

96fdee9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

.NET profiler improvements & test fixes #2968

.NET profiler improvements & test fixes #2968

vaind commented Dec 13, 2023 •

edited

Loading

bruno-garcia Dec 20, 2023

vaind Dec 21, 2023

bruno-garcia Dec 22, 2023 •

edited by vaind

Loading

vaind Dec 22, 2023

bruno-garcia Dec 22, 2023 •

edited by vaind

Loading

bruno-garcia Dec 22, 2023

bruno-garcia Dec 22, 2023

vaind Dec 22, 2023

bruno-garcia Dec 22, 2023

.NET profiler improvements & test fixes #2968

.NET profiler improvements & test fixes #2968

Conversation

vaind commented Dec 13, 2023 • edited Loading

bruno-garcia Dec 20, 2023

Choose a reason for hiding this comment

vaind Dec 21, 2023

Choose a reason for hiding this comment

bruno-garcia Dec 22, 2023 • edited by vaind Loading

Choose a reason for hiding this comment

vaind Dec 22, 2023

Choose a reason for hiding this comment

bruno-garcia Dec 22, 2023 • edited by vaind Loading

Choose a reason for hiding this comment

bruno-garcia Dec 22, 2023

Choose a reason for hiding this comment

bruno-garcia Dec 22, 2023

Choose a reason for hiding this comment

vaind Dec 22, 2023

Choose a reason for hiding this comment

bruno-garcia Dec 22, 2023

Choose a reason for hiding this comment

vaind commented Dec 13, 2023 •

edited

Loading

bruno-garcia Dec 22, 2023 •

edited by vaind

Loading

bruno-garcia Dec 22, 2023 •

edited by vaind

Loading