App Hangs - Be able to attach Main thread stack trace to non fatal app hangs

[Angelodaniel]

Problem Statement

Sentry's non-fatal error reporting for app hangs only captures the stack trace of the calling thread, not the main thread, which is crucial for debugging app hangs.

So it only captures the stack trace of that specific thread but it would be beneficial to have it from the main thread.

Right now customer has forked their own SDK version but I'd like to remove the need.

Solution Brainstorm

Customer has added a boolean flag attachMainStackTrace which let them decide if they either call getCurrentThreads or getCurrentThreadsWithStackTrace for non fatal ANR's

Are you willing to submit a PR?

No response

[linear]

COCOA-1089

[philipphofmann]

@Angelodaniel, I'm a bit confused. The app hang integration captures all threads when detecting an app hang.

sentry-cocoa/Sources/Sentry/SentryANRTrackingIntegration.m

Line 133 in 7162be8

NSArray<SentryThread *> *threads = [self.threadInspector getCurrentThreadsWithStackTrace];

Then calls into

sentry-cocoa/Sources/Sentry/SentryDefaultThreadInspector.m

Lines 119 to 198 in 7162be8

	/**
	* We are not sharing code with 'getCurrentThreads' because both methods use different approaches.
	* This method retrieves thread information from the suspend method
	* while the other retrieves information from the machine context.
	* Having both approaches in the same method can lead to inconsistency between the number of
	* threads, and while there is suspended threads we can't call into obj-c, so the previous approach
	* wont work for retrieving stacktrace information for every thread.
	*/
	- (NSArray<SentryThread *> *)getCurrentThreadsWithStackTrace
	{
	NSMutableArray<SentryThread *> *threads = [NSMutableArray new];
	@synchronized(self) {
	SentryCrashMC_NEW_CONTEXT(context);
	SentryCrashThread currentThread = sentrycrashthread_self();
	thread_act_array_t suspendedThreads = NULL;
	mach_msg_type_number_t numSuspendedThreads = 0;
	// SentryThreadInspector is crashing when there is too many threads.
	// We add a limit of 70 threads because in test with up to 100 threads it seems fine.
	// We are giving it an extra safety margin.
	sentrycrashmc_suspendEnvironment_upToMaxSupportedThreads(
	&suspendedThreads, &numSuspendedThreads, 70);
	// DANGER: Do not try to allocate memory in the heap or call Objective-C code in this
	// section Doing so when the threads are suspended may lead to deadlocks or crashes.
	// If no threads was suspended we don't need to do anything.
	// This may happen if there is more than max amount of threads (70).
	if (numSuspendedThreads == 0) {
	return threads;
	}
	SentryThreadInfo threadsInfos[numSuspendedThreads];
	for (int i = 0; i < numSuspendedThreads; i++) {
	if (suspendedThreads[i] != currentThread) {
	int numberOfEntries = getStackEntriesFromThread(suspendedThreads[i], context,
	threadsInfos[i].stackEntries, MAX_STACKTRACE_LENGTH);
	threadsInfos[i].stackLength = numberOfEntries;
	} else {
	// We can't use 'getStackEntriesFromThread' to retrieve stack frames from the
	// current thread. We are using the stackTraceBuilder to retrieve this information
	// later.
	threadsInfos[i].stackLength = 0;
	}
	threadsInfos[i].thread = suspendedThreads[i];
	}
	sentrycrashmc_resumeEnvironment(suspendedThreads, numSuspendedThreads);
	// DANGER END: You may call Objective-C code again or allocate memory.
	for (int i = 0; i < numSuspendedThreads; i++) {
	SentryThread *sentryThread = [[SentryThread alloc] initWithThreadId:@(i)];
	sentryThread.isMain = [NSNumber numberWithBool:i == 0];
	sentryThread.name = [self getThreadName:threadsInfos[i].thread];
	sentryThread.crashed = @NO;
	bool isCurrent = threadsInfos[i].thread == currentThread;
	sentryThread.current = @(isCurrent);
	if (isCurrent) {
	sentryThread.stacktrace = [self.stacktraceBuilder buildStacktraceForCurrentThread];
	} else {
	sentryThread.stacktrace = [self.stacktraceBuilder
	buildStackTraceFromStackEntries:threadsInfos[i].stackEntries
	amount:threadsInfos[i].stackLength];
	}
	// We need to make sure the main thread is always the first thread in the result
	if ([self.machineContextWrapper isMainThread:threadsInfos[i].thread])
	[threads insertObject:sentryThread atIndex:0];
	else
	[threads addObject:sentryThread];
	}
	}
	return threads;
	}

I just tried this with our sample app and got all threads for a non fatal app hang event: https://sentry-sdks.sentry.io/issues/7113221505/?project=5428557&query=is%3Aunresolved&referrer=issue-stream

Which SDK version are these customers using? Do you have some example events that show only the stacktrace of a single thread for a non-fatal app hang?

[sepbehroozi]

Thanks @philipphofmann for the details. Indeed that's the case if the AppHang gets detected by the SentrySDK because it uses the [threadInspector getCurrentThreadsWithStackTrace] method to capture the stacktrace of all threads.
Maybe a better problem statement would be that we'd like the ability to capture all thread's stacktrace when reporting a normal non-fatal error. Yes we report an Error object when our custom AppHang detection logic detects a hang. But having all thread's stacktrace when reporting a non-fatal could be handy in other use cases as well.

It's the behaviour of the latest version of the SDK since it uses a different method of the threadInspector that captures only the caller thread's stacktrace.

[philipphofmann]

Thanks for the context @sepbehroozi. Now I get it, thanks. We already have an issue for adding an option attachThreads(#1870 ) that would attach all threads for all capture event invocations, including captureError. Would that work for you, or would you like to have a more fine-grained control to only add all threads for specific invocations?

[sepbehroozi]

That's amazing to hear, thanks @philipphofmann.
It depends on the potential impact on performance, network I/O, and (most importantly) the issue grouping on Sentry.
If I understood correctly, Sentry prioritizes event grouping based on the client fingerprint, then in-app Stacktrace, then the exception details (error domain and code). If toggling the attackThreads causes all of our non-fatal issues to regroup due to grouping hash change, then I'd vote for more granular stacktrace capture control for each error report invocation.
Happy Christmas and new year in advance 🎄

[philipphofmann]

If toggling the attackThreads causes all of our non-fatal issues to regroup due to grouping hash change, then I'd vote for more granular stacktrace capture control for each error report invocation.

Yes, it would. So a more fine-grained approach totally makes sense, yes.

Thanks, also Merry Christmas and a happy new year in advance 🎄 😃