From d9ee56f63a993c637e83e152cbef00b528809129 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Fri, 27 Oct 2023 10:54:38 +0200 Subject: [PATCH 1/2] impr: Stop sending empty thread names Set the thread name to nil when retrieving the thread name from the machine context wrapper in case it fails or the thread name is empty. This slightly reduces the payload size. --- CHANGELOG.md | 4 +++ .../SentryCrashDefaultMachineContextWrapper.m | 9 ++++-- Sources/Sentry/SentrySpan.m | 5 +--- Sources/Sentry/SentryThreadInspector.m | 21 ++++++++----- .../SentryCrashMachineContextWrapper.h | 2 +- .../Sentry/include/SentryThreadInspector.h | 2 +- .../Protocol/SentryThreadTests.swift | 9 ++++++ .../SentryThreadInspectorTests.swift | 30 +++++++++++++++---- 8 files changed, 61 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e9fac15162d..942e8300930 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ - Add thread id and name to span data (#3359) +### Improvements + +- Stop sending empty thread names (#3361) + ## 8.14.2 ### Features diff --git a/Sources/Sentry/SentryCrashDefaultMachineContextWrapper.m b/Sources/Sentry/SentryCrashDefaultMachineContextWrapper.m index b077d673eb0..9babc479b13 100644 --- a/Sources/Sentry/SentryCrashDefaultMachineContextWrapper.m +++ b/Sources/Sentry/SentryCrashDefaultMachineContextWrapper.m @@ -41,11 +41,16 @@ - (SentryCrashThread)getThread:(struct SentryCrashMachineContext *)context withI return thread; } -- (void)getThreadName:(const SentryCrashThread)thread +- (BOOL)getThreadName:(const SentryCrashThread)thread andBuffer:(char *const)buffer andBufLength:(int)bufLength; { - sentrycrashthread_getThreadName(thread, buffer, bufLength); + bool result = sentrycrashthread_getThreadName(thread, buffer, bufLength); + if (result == true) { + return YES; + } else { + return NO; + } } - (BOOL)isMainThread:(SentryCrashThread)thread diff --git a/Sources/Sentry/SentrySpan.m b/Sources/Sentry/SentrySpan.m index 3c8e629a2e1..7cc57f96225 100644 --- a/Sources/Sentry/SentrySpan.m +++ b/Sources/Sentry/SentrySpan.m @@ -43,11 +43,8 @@ - (instancetype)initWithContext:(SentrySpanContext *)context if ([NSThread isMainThread]) { _data[SPAN_DATA_THREAD_NAME] = @"main"; } else { - NSString *threadName = [SentryDependencyContainer.sharedInstance.threadInspector + _data[SPAN_DATA_THREAD_NAME] = [SentryDependencyContainer.sharedInstance.threadInspector getThreadName:currentThread]; - if (threadName.length > 0) { - _data[SPAN_DATA_THREAD_NAME] = threadName; - } } _tags = [[NSMutableDictionary alloc] init]; diff --git a/Sources/Sentry/SentryThreadInspector.m b/Sources/Sentry/SentryThreadInspector.m index bdbf51b6566..e9b828c317c 100644 --- a/Sources/Sentry/SentryThreadInspector.m +++ b/Sources/Sentry/SentryThreadInspector.m @@ -202,17 +202,24 @@ - (SentryStacktrace *)stacktraceForCurrentThreadAsyncUnsafe return threads; } -- (NSString *)getThreadName:(SentryCrashThread)thread +- (nullable NSString *)getThreadName:(SentryCrashThread)thread { - char buffer[128]; + int bufferLength = 128; + char buffer[bufferLength]; char *const pBuffer = buffer; - [self.machineContextWrapper getThreadName:thread andBuffer:pBuffer andBufLength:128]; - NSString *threadName = [NSString stringWithCString:pBuffer encoding:NSUTF8StringEncoding]; - if (nil == threadName) { - threadName = @""; + BOOL didGetThreadNameSucceed = [self.machineContextWrapper getThreadName:thread + andBuffer:pBuffer + andBufLength:bufferLength]; + + if (didGetThreadNameSucceed == YES) { + NSString *threadName = [NSString stringWithCString:pBuffer encoding:NSUTF8StringEncoding]; + if (threadName.length > 0) { + return threadName; + } } - return threadName; + + return nil; } @end diff --git a/Sources/Sentry/include/SentryCrashMachineContextWrapper.h b/Sources/Sentry/include/SentryCrashMachineContextWrapper.h index 96138f9fb32..abf60cde9de 100644 --- a/Sources/Sentry/include/SentryCrashMachineContextWrapper.h +++ b/Sources/Sentry/include/SentryCrashMachineContextWrapper.h @@ -14,7 +14,7 @@ NS_ASSUME_NONNULL_BEGIN - (SentryCrashThread)getThread:(struct SentryCrashMachineContext *)context withIndex:(int)index; -- (void)getThreadName:(const SentryCrashThread)thread +- (BOOL)getThreadName:(const SentryCrashThread)thread andBuffer:(char *const)buffer andBufLength:(int)bufLength; diff --git a/Sources/Sentry/include/SentryThreadInspector.h b/Sources/Sentry/include/SentryThreadInspector.h index fd028fb7c36..b493b4f2948 100644 --- a/Sources/Sentry/include/SentryThreadInspector.h +++ b/Sources/Sentry/include/SentryThreadInspector.h @@ -31,7 +31,7 @@ SENTRY_NO_INIT */ - (NSArray *)getCurrentThreadsWithStackTrace; -- (NSString *)getThreadName:(SentryCrashThread)thread; +- (nullable NSString *)getThreadName:(SentryCrashThread)thread; @end diff --git a/Tests/SentryTests/Protocol/SentryThreadTests.swift b/Tests/SentryTests/Protocol/SentryThreadTests.swift index 4951e01994a..888051c74f0 100644 --- a/Tests/SentryTests/Protocol/SentryThreadTests.swift +++ b/Tests/SentryTests/Protocol/SentryThreadTests.swift @@ -18,6 +18,15 @@ class SentryThreadTests: XCTestCase { XCTAssertTrue(actual["main"] as! Bool) } + func testSerialize_ThreadNameNil() { + let thread = TestData.thread + thread.name = nil + + let actual = thread.serialize() + + XCTAssertNil(actual["name"]) + } + func testSerialize_Bools() { let thread = SentryThread(threadId: 0) diff --git a/Tests/SentryTests/SentryCrash/SentryThreadInspectorTests.swift b/Tests/SentryTests/SentryCrash/SentryThreadInspectorTests.swift index 6c90ee537eb..2014700a1d2 100644 --- a/Tests/SentryTests/SentryCrash/SentryThreadInspectorTests.swift +++ b/Tests/SentryTests/SentryCrash/SentryThreadInspectorTests.swift @@ -182,15 +182,27 @@ class SentryThreadInspectorTests: XCTestCase { XCTAssertEqual(threadName, actual[0].name) } - func testThreadNameIsNull() { - fixture.testMachineContextWrapper.threadName = nil + func testGetThreadName_EmptyThreadName() { + fixture.testMachineContextWrapper.threadName = "" fixture.testMachineContextWrapper.threadCount = 1 let actual = fixture.getSut().getCurrentThreads() XCTAssertEqual(1, actual.count) let thread = actual[0] - XCTAssertEqual("", thread.name) + XCTAssertNil(thread.name) + } + + func testGetThreadNameFails() { + fixture.testMachineContextWrapper.threadName = "" + fixture.testMachineContextWrapper.getThreadNameSucceeds = false + fixture.testMachineContextWrapper.threadCount = 1 + + let actual = fixture.getSut().getCurrentThreads() + XCTAssertEqual(1, actual.count) + + let thread = actual[0] + XCTAssertNil(thread.name) } func testLongThreadName() { @@ -270,16 +282,22 @@ private class TestMachineContextWrapper: NSObject, SentryCrashMachineContextWrap mockThreads?[Int(index)].threadId ?? 0 } - var threadName: String? = "" - func getThreadName(_ thread: SentryCrashThread, andBuffer buffer: UnsafeMutablePointer, andBufLength bufLength: Int32) { + var threadName: String = "" + var getThreadNameSucceeds = true + func getThreadName(_ thread: SentryCrashThread, andBuffer buffer: UnsafeMutablePointer, andBufLength bufLength: Int32) -> Bool { if let mocks = mockThreads, let index = mocks.firstIndex(where: { $0.threadId == thread }) { strcpy(buffer, mocks[index].name) - } else if threadName != nil { + return true + } + + if getThreadNameSucceeds { strcpy(buffer, threadName) + return true } else { _ = Array(repeating: 0, count: Int(bufLength)).withUnsafeBufferPointer { bufferPointer in strcpy(buffer, bufferPointer.baseAddress) } + return false } } From 191adbe7884386d4d02329d36053357f9980a4d8 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Mon, 30 Oct 2023 14:21:26 +0100 Subject: [PATCH 2/2] Update Sources/Sentry/SentryCrashDefaultMachineContextWrapper.m Co-authored-by: Dhiogo Brustolin --- Sources/Sentry/SentryCrashDefaultMachineContextWrapper.m | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/Sources/Sentry/SentryCrashDefaultMachineContextWrapper.m b/Sources/Sentry/SentryCrashDefaultMachineContextWrapper.m index 9babc479b13..88b7e2de62c 100644 --- a/Sources/Sentry/SentryCrashDefaultMachineContextWrapper.m +++ b/Sources/Sentry/SentryCrashDefaultMachineContextWrapper.m @@ -45,12 +45,7 @@ - (BOOL)getThreadName:(const SentryCrashThread)thread andBuffer:(char *const)buffer andBufLength:(int)bufLength; { - bool result = sentrycrashthread_getThreadName(thread, buffer, bufLength); - if (result == true) { - return YES; - } else { - return NO; - } + return sentrycrashthread_getThreadName(thread, buffer, bufLength) == true; } - (BOOL)isMainThread:(SentryCrashThread)thread