Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: update profiling API for hybrid SDKs #3255

Merged
merged 2 commits into from
Aug 28, 2023
Merged

chore: update profiling API for hybrid SDKs #3255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ab3e172
chore: update profiling API for hybrid SDKs
vaind Aug 25, 2023
26003e6
add thread ID to hybrid SDK profile payload
vaind Aug 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions Sentry.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@
7B6C5F8126034354007F7DFF /* SentryWatchdogTerminationLogic.h in Headers */ = {isa = PBXBuildFile; fileRef = 7B6C5F8026034354007F7DFF /* SentryWatchdogTerminationLogic.h */; };
7B6C5F8726034395007F7DFF /* SentryWatchdogTerminationLogic.m in Sources */ = {isa = PBXBuildFile; fileRef = 7B6C5F8626034395007F7DFF /* SentryWatchdogTerminationLogic.m */; };
7B6CC50224EE5A42001816D7 /* SentryHubTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7B6CC50124EE5A42001816D7 /* SentryHubTests.swift */; };
7B6D1261265F784000C9BE4B /* PrivateSentrySDKOnly.m in Sources */ = {isa = PBXBuildFile; fileRef = 7B6D1260265F784000C9BE4B /* PrivateSentrySDKOnly.m */; };
7B6D1261265F784000C9BE4B /* PrivateSentrySDKOnly.mm in Sources */ = {isa = PBXBuildFile; fileRef = 7B6D1260265F784000C9BE4B /* PrivateSentrySDKOnly.mm */; };
7B6D1263265F7CC600C9BE4B /* PrivateSentrySDKOnlyTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7B6D1262265F7CC600C9BE4B /* PrivateSentrySDKOnlyTests.swift */; };
7B6D135C27F4605D00331ED2 /* TestEnvelopeRateLimitDelegate.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7B6D135B27F4605D00331ED2 /* TestEnvelopeRateLimitDelegate.swift */; };
7B6D98E924C6D336005502FA /* SentrySdkInfo+Equality.m in Sources */ = {isa = PBXBuildFile; fileRef = 7B6D98E824C6D336005502FA /* SentrySdkInfo+Equality.m */; };
Expand Down Expand Up @@ -1277,7 +1277,7 @@
7B6C5F8026034354007F7DFF /* SentryWatchdogTerminationLogic.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = SentryWatchdogTerminationLogic.h; path = include/SentryWatchdogTerminationLogic.h; sourceTree = "<group>"; };
7B6C5F8626034395007F7DFF /* SentryWatchdogTerminationLogic.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SentryWatchdogTerminationLogic.m; sourceTree = "<group>"; };
7B6CC50124EE5A42001816D7 /* SentryHubTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryHubTests.swift; sourceTree = "<group>"; };
7B6D1260265F784000C9BE4B /* PrivateSentrySDKOnly.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = PrivateSentrySDKOnly.m; sourceTree = "<group>"; };
7B6D1260265F784000C9BE4B /* PrivateSentrySDKOnly.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = PrivateSentrySDKOnly.mm; sourceTree = "<group>"; };
7B6D1262265F7CC600C9BE4B /* PrivateSentrySDKOnlyTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PrivateSentrySDKOnlyTests.swift; sourceTree = "<group>"; };
7B6D135B27F4605D00331ED2 /* TestEnvelopeRateLimitDelegate.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TestEnvelopeRateLimitDelegate.swift; sourceTree = "<group>"; };
7B6D98E724C6D336005502FA /* SentrySdkInfo+Equality.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = "SentrySdkInfo+Equality.h"; sourceTree = "<group>"; };
Expand Down Expand Up @@ -2286,7 +2286,7 @@
63AA76931EB9C1C200D153DE /* Sentry.h */,
D8BBD32628FD9FBF0011F850 /* SentrySwift.h */,
D81A346B291AECC7005A27A9 /* PrivateSentrySDKOnly.h */,
7B6D1260265F784000C9BE4B /* PrivateSentrySDKOnly.m */,
7B6D1260265F784000C9BE4B /* PrivateSentrySDKOnly.mm */,
63AA76941EB9C1C200D153DE /* SentryClient.h */,
63AA75ED1EB8B3C400D153DE /* SentryClient.m */,
7B85DC1C24EFAFCD007D01D2 /* SentryClient+Private.h */,
Expand Down Expand Up @@ -4092,7 +4092,7 @@
7BE1E33424F7E3CB009D3AD0 /* SentryMigrateSessionInit.m in Sources */,
15E0A8F22411A45A00F044E3 /* SentrySession.m in Sources */,
844EDCE62947DC3100C86F34 /* SentryNSTimerFactory.m in Sources */,
7B6D1261265F784000C9BE4B /* PrivateSentrySDKOnly.m in Sources */,
7B6D1261265F784000C9BE4B /* PrivateSentrySDKOnly.mm in Sources */,
63BE85711ECEC6DE00DC44F5 /* NSDate+SentryExtras.m in Sources */,
7BD4BD4927EB2A5D0071F4FF /* SentryDiscardedEvent.m in Sources */,
03F84D3827DD4191008FE43F /* SentryBacktrace.cpp in Sources */,
Expand Down
29 changes: 21 additions & 8 deletions Sources/Sentry/PrivateSentrySDKOnly.m → Sources/Sentry/PrivateSentrySDKOnly.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@
#import "SentryInstallation.h"
#import "SentryInternalDefines.h"
#import "SentryMeta.h"
#import "SentryProfiledTracerConcurrency.h"
#import "SentryProfiler.h"
#import "SentrySDK+Private.h"
#import "SentrySerialization.h"
#import "SentryThreadHandle.hpp"
#import "SentryUser+Private.h"
#import "SentryViewHierarchy.h"
#import <SentryBreadcrumb.h>
Expand Down Expand Up @@ -121,27 +123,38 @@ + (NSDictionary *)getExtraContext
}

#if SENTRY_TARGET_PROFILING_SUPPORTED
+ (uint64_t)startProfilingForTrace:(SentryId *)traceId;
+ (uint64_t)startProfilerForTrace:(SentryId *)traceId;
{
[SentryProfiler startWithTracer:traceId];
return SentryDependencyContainer.sharedInstance.dateProvider.systemTime;
}

+ (nullable NSDictionary<NSString *, id> *)collectProfileForTrace:(SentryId *)traceId
since:(uint64_t)startSystemTime;
+ (nullable NSDictionary<NSString *, id> *)collectProfileBetween:(uint64_t)startSystemTime
and:(uint64_t)endSystemTime
forTrace:(SentryId *)traceId;
{
NSMutableDictionary<NSString *, id> *payload = [SentryProfiler
collectProfileBetween:startSystemTime
and:SentryDependencyContainer.sharedInstance.dateProvider.systemTime
forTrace:traceId
onHub:[SentrySDK currentHub]];
NSMutableDictionary<NSString *, id> *payload =
[SentryProfiler collectProfileBetween:startSystemTime
and:endSystemTime
forTrace:traceId
onHub:[SentrySDK currentHub]];

if (payload != nil) {
payload[@"platform"] = SentryPlatformName;
payload[@"transaction"] = @{
@"active_thread_id" :
[NSNumber numberWithLongLong:sentry::profiling::ThreadHandle::current()->tid()]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to get the thread ID at the time that the profile is being processed, why are we overwriting the one previously set at the time the trace was started with this one? I imagine it's possible to have a different active thread for each of the 3 times: 1) when the profile is started, 2) ended and 3) being processed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the one previously set at the time the trace was started

We are not, there's no thread ID stored when the profiler is started. There is one stored when a transaction is started, but that doesn't apply here, where we don't start objective-c transactions. And I'm aware this may be different in those scenarios, and am not completely certain it's going to be correct the way it works in Flutter but this was the simplest approach and if it works, than no need to complicate things. If it doesn't, I'll follow up.

It's not great that I cannot test this in Flutter without the cocoa SDK being released (or without messing up with the flutter plugin build setup) - so it's a bit of a guesswork and needs a couple of iterations 🤷

};
}

return payload;
}

+ (void)discardProfilerForTrace:(SentryId *)traceId;
{
discardProfilerForTracer(traceId);
}

#endif // SENTRY_TARGET_PROFILING_SUPPORTED

#if SENTRY_HAS_UIKIT
Expand Down
14 changes: 11 additions & 3 deletions Sources/Sentry/include/HybridPublic/PrivateSentrySDKOnly.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,13 +85,21 @@ typedef void (^SentryOnAppStartMeasurementAvailable)(
* Start a profiler session associated with the given @c SentryId.
* @return The system time when the profiler session started.
*/
+ (uint64_t)startProfilingForTrace:(SentryId *)traceId;
+ (uint64_t)startProfilerForTrace:(SentryId *)traceId;

/**
* Collect a profiler session data associated with the given @c SentryId.
* This also discards the profiler.
*/
+ (nullable NSDictionary<NSString *, id> *)collectProfileForTrace:(SentryId *)traceId
since:(uint64_t)startSystemTime;
+ (nullable NSDictionary<NSString *, id> *)collectProfileBetween:(uint64_t)startSystemTime
and:(uint64_t)endSystemTime
forTrace:(SentryId *)traceId;
Comment on lines +94 to +96
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

h: Need to change the function signature to follow our pattern.

Suggested change
+ (nullable NSDictionary<NSString *, id> *)collectProfileBetween:(uint64_t)startSystemTime
and:(uint64_t)endSystemTime
forTrace:(SentryId *)traceId;
+ (nullable NSDictionary<NSString *, id> *)collectProfileBetweenStartTime:(uint64_t)startSystemTime
endTime:(uint64_t)endSystemTime
forTrace:(SentryId *)traceId;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. in general method parameter names like and:/with: are discouraged in ObjC; they should always contain a noun.

Copy link
Collaborator Author

@vaind vaind Aug 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't know that. I've done it the same way as in the forwarded function:

sentry-cocoa/Sources/Sentry/SentryProfiler.mm

Lines 370 to 373 in b9d59f7

+ (nullable NSMutableDictionary<NSString *, id> *)collectProfileBetween:(uint64_t)startSystemTime
and:(uint64_t)endSystemTime
forTrace:(SentryId *)traceId
onHub:(SentryHub *)hub;


/**
* Discard profiler session data associated with the given @c SentryId.
* This only needs to be called in case you haven't collected the profile (and don't intend to).
*/
+ (void)discardProfilerForTrace:(SentryId *)traceId;
#endif

@property (class, nullable, nonatomic, copy)
Expand Down
30 changes: 27 additions & 3 deletions Tests/SentryTests/PrivateSentrySDKOnlyTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,18 +124,18 @@ class PrivateSentrySDKOnlyTests: XCTestCase {
/**
* Smoke Tests profiling via PrivateSentrySDKOnly. Actual profiling unit tests are done elsewhere.
*/
func testProfiling() {
func testProfilingStartAndCollect() {
let options = Options()
options.dsn = TestConstants.dsnAsString(username: "SentryFramesTrackingIntegrationTests")
let client = TestClient(options: options)
SentrySDK.setCurrentHub(TestHub(client: client, andScope: nil))

let traceIdA = SentryId()

let startTime = PrivateSentrySDKOnly.startProfiling(forTrace: traceIdA)
let startTime = PrivateSentrySDKOnly.startProfiler(forTrace: traceIdA)
XCTAssertGreaterThan(startTime, 0)
Thread.sleep(forTimeInterval: 0.2)
let payload = PrivateSentrySDKOnly.collectProfile(forTrace: traceIdA, since: startTime)
let payload = PrivateSentrySDKOnly.collectProfileBetween(startTime, and: startTime + 200_000_000, forTrace: traceIdA)
XCTAssertNotNil(payload)
XCTAssertEqual(payload?["platform"] as? String, "cocoa")
XCTAssertNotNil(payload?["debug_meta"])
Expand All @@ -146,6 +146,30 @@ class PrivateSentrySDKOnlyTests: XCTestCase {
XCTAssertNotNil(profile?["samples"])
XCTAssertNotNil(profile?["stacks"])
XCTAssertNotNil(profile?["frames"])
let transactionInfo = payload?["transaction"] as? NSDictionary
XCTAssertNotNil(transactionInfo)
XCTAssertGreaterThan(transactionInfo?["active_thread_id"] as! Int64, 0)
}

func testProfilingDiscard() {
let options = Options()
options.dsn = TestConstants.dsnAsString(username: "SentryFramesTrackingIntegrationTests")
let client = TestClient(options: options)
SentrySDK.setCurrentHub(TestHub(client: client, andScope: nil))

let traceIdA = SentryId()

let startTime = PrivateSentrySDKOnly.startProfiler(forTrace: traceIdA)
XCTAssertGreaterThan(startTime, 0)
Thread.sleep(forTimeInterval: 0.2)
PrivateSentrySDKOnly.discardProfiler(forTrace: traceIdA)
// how can we test that that this fails with an NCAssert failure?
// XCTAssertThrowsError(
// PrivateSentrySDKOnly.collectProfileBetween(
// startTime, and: startTime + 200_000_000, forTrace: traceIdA)
// ) { error in
// XCTAssertTrue(error is NSException)
// }
}
#endif

Expand Down
Loading