feat: allow profiling from hybrid SDKs #3194

vaind · 2023-07-31T19:03:11Z

📜 Description

💡 Motivation and Context

Cocoa SDK built-in native profiler can be used to support profiling Flutter apps, see getsentry/sentry-dart#1106

This PR refactors some profiler APIs so that they don't depend on the cocoa Tracer & Transaction and instead expect only the necessary arguments.

#skip-changelog

💚 How did you test it?

All previous tests pass & adding new tests for new hybrid SDK APIs

📝 Checklist

You have to check all boxes before merging:

I reviewed the submitted code.
I added tests to verify the changes.
No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
I updated the docs if needed.
Review from the native team if needed.
No breaking change or entry added to the changelog.
No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

codecov · 2023-07-31T19:11:35Z

Codecov Report

Merging #3194 (22df0d7) into main (279841c) will increase coverage by 0.003%.
The diff coverage is 86.896%.

Additional details and impacted files

@@              Coverage Diff              @@
##              main     #3194       +/-   ##
=============================================
+ Coverage   89.179%   89.182%   +0.003%     
=============================================
  Files          502       502               
  Lines        54026     54060       +34     
  Branches     19379     19398       +19     
=============================================
+ Hits         48180     48212       +32     
+ Misses        4994      4880      -114     
- Partials       852       968      +116

Files Changed	Coverage Δ
Sources/Sentry/include/SentryInternalDefines.h	`35.294% <ø> (ø)`
Sources/Sentry/SentryProfileTimeseries.mm	`49.056% <28.571%> (-0.067%)`	⬇️
Sources/Sentry/SentryMetricProfiler.mm	`94.957% <75.000%> (ø)`
Sources/Sentry/SentryProfiler.mm	`81.055% <80.327%> (+0.990%)`	⬆️
Sources/Sentry/PrivateSentrySDKOnly.m	`87.500% <100.000%> (+2.314%)`	⬆️
...entry/Profiling/SentryProfiledTracerConcurrency.mm	`94.736% <100.000%> (ø)`
Sources/Sentry/SentryEvent.m	`98.260% <100.000%> (ø)`
Sources/Sentry/SentryTracer.m	`96.709% <100.000%> (+0.365%)`	⬆️
Tests/SentryProfilerTests/SentryProfilerTests.mm	`98.214% <100.000%> (-0.120%)`	⬇️
...ance/FramesTracking/SentryFramesTrackerTests.swift	`98.742% <100.000%> (ø)`
... and 1 more

... and 31 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 279841c...22df0d7. Read the comment docs.

github-actions · 2023-08-01T12:47:29Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1222.59 ms	1230.14 ms	7.55 ms
Size	22.84 KiB	403.14 KiB	380.29 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`302ee8b`	1194.02 ms	1223.34 ms	29.32 ms
`1e065bc`	1239.69 ms	1258.18 ms	18.49 ms
`be51b56`	1220.84 ms	1249.36 ms	28.52 ms
`32c4446`	1225.00 ms	1231.29 ms	6.29 ms
`3277f18`	1229.29 ms	1248.92 ms	19.63 ms
`15b8c61`	1223.16 ms	1244.83 ms	21.67 ms
`794f87f`	1225.78 ms	1243.46 ms	17.68 ms
`98a8c16`	1206.40 ms	1232.14 ms	25.74 ms
`1db04d8`	1250.20 ms	1258.12 ms	7.92 ms
`c2ec420`	1256.27 ms	1269.24 ms	12.97 ms

App size

Revision	Plain	With Sentry	Diff
`302ee8b`	20.76 KiB	419.62 KiB	398.87 KiB
`1e065bc`	20.76 KiB	425.78 KiB	405.01 KiB
`be51b56`	20.76 KiB	432.20 KiB	411.44 KiB
`32c4446`	22.84 KiB	403.24 KiB	380.39 KiB
`3277f18`	22.84 KiB	402.88 KiB	380.03 KiB
`15b8c61`	20.76 KiB	419.67 KiB	398.91 KiB
`794f87f`	20.76 KiB	401.37 KiB	380.61 KiB
`98a8c16`	20.76 KiB	431.00 KiB	410.24 KiB
`1db04d8`	20.76 KiB	435.50 KiB	414.74 KiB
`c2ec420`	20.76 KiB	401.65 KiB	380.89 KiB

Previous results on branch: feat/hybrid-sdk-profiling

Startup times

Revision	Plain	With Sentry	Diff
`03a521f`	1253.02 ms	1255.46 ms	2.44 ms
`bbe7566`	1224.92 ms	1233.66 ms	8.74 ms
`194680d`	1254.00 ms	1269.65 ms	15.65 ms
`ca7bde3`	1231.17 ms	1256.17 ms	25.00 ms
`1c02a7b`	1219.43 ms	1232.71 ms	13.29 ms

App size

Revision	Plain	With Sentry	Diff
`03a521f`	22.84 KiB	403.13 KiB	380.29 KiB
`bbe7566`	22.84 KiB	403.14 KiB	380.29 KiB
`194680d`	22.84 KiB	403.14 KiB	380.29 KiB
`ca7bde3`	22.84 KiB	403.14 KiB	380.29 KiB
`1c02a7b`	22.84 KiB	403.14 KiB	380.29 KiB

github-actions · 2023-08-02T07:36:27Z

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 22df0d7

brustolin

This looks ok to me, but I would like for @armcknight to also review this.

vaind · 2023-08-01T13:03:58Z

Sources/Sentry/SentryProfiler.mm

-            @"elapsed_since_start_ns" : sentry_stringForUInt64(
-                getDurationNs(transaction.startSystemTime, sample.absoluteTimestamp)),
+            @"elapsed_since_start_ns" :
+                sentry_stringForUInt64(getDurationNs(startSystemTime, sample.absoluteTimestamp)),


Unrelated to this PR: this should be an integer, not a string, right? https://develop.sentry.dev/sdk/profiles/

No, it must be a string because the backend JSON implementation can't support unsigned 64 bit integers. Unless that has changed, cc @phacops

If it has changed, consider the impact on self hosted Sentry though. We prefer adding breaking changes (ideally never) on. SDK major releases and be clear on changelog which self hosted version is required.

I haven't changed any of this here, just noticed and pointed out that the timestamp is sent as a string here but an integer in (some of the) other SDKs

vaind · 2023-08-01T13:06:24Z

Sources/Sentry/SentryProfiler.mm

-        SENTRY_LOG_WARN(@"Expected a profiler for tracer id %@ but none was found",
-            transaction.trace.traceId.sentryIdString);


The warning is already part of the profilerForFinishedTracer() under the SENTRY_CASSERT_RETURN macro

Nice find 👍🏻

vaind · 2023-08-01T14:21:37Z

Sources/Sentry/PrivateSentrySDKOnly.m

+                        onHub:[SentrySDK currentHub]];
+
+    if (payload != nil) {
+        payload[@"platform"] = @"cocoa";


In the usual profiler code path, this is copied from transaction (a.k.a. SentryEvent). Ideally, there would be a constant we could use in both places but there doesn't seem to be one. I wasn't sure about a place to move it to so I didn't...

Yeah, this is hardcoded the same way in SentryEvent.m. We could define it in SentryInternalDefines.h and use that constant in all three places, are you able to import that header here?

…tion

armcknight

The refactor of the profiling code to expose the +[collectProfileBetween:...] looks fine, the part I'm confused about is why things were changed from passing a SentryTracer or SentryTransaction, in favor more parameters that are all encapsulated by them already? There is a SentryTracer and SentryTransaction in the dart codebase, so I assume we'd be able to call a function like +[PrivateSentrySDKOnly startProfilingForTrace:(SentryTracer *)] instead of +[PrivateSentrySDKOnly startProfilingForTrace:(SentryId *)]. Let me know if I'm missing something there, I don't have any experience writing Dart or with our SDK for it.

armcknight · 2023-08-08T00:16:17Z

Sources/Sentry/PrivateSentrySDKOnly.m

+                        onHub:[SentrySDK currentHub]];
+
+    if (payload != nil) {
+        payload[@"platform"] = @"cocoa";


Yeah, this is hardcoded the same way in SentryEvent.m. We could define it in SentryInternalDefines.h and use that constant in all three places, are you able to import that header here?

Sources/Sentry/PrivateSentrySDKOnly.m

armcknight · 2023-08-08T00:19:37Z

Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm

@@ -55,12 +54,12 @@
 std::mutex _gStateLock;

 void
-trackProfilerForTracer(SentryProfiler *profiler, SentryTracer *tracer)


I purposely used SentryTracer here to keep type safety. SentryEvent, SentrySpan and SentryTracer all have a SentryId property, but they're not interchangeable, so I did this to prevent any mistakes.

armcknight · 2023-08-08T00:21:41Z

Sources/Sentry/SentryMetricProfiler.mm

@@ -46,23 +46,22 @@ @implementation SentryMetricReading
 */
 SentrySerializedMetricEntry *_Nullable serializeValuesWithNormalizedTime(
    NSArray<SentryMetricReading *> *absoluteTimestampValues, NSString *unit,
-    SentryTransaction *transaction)


Same thing here with type safety, in addition to cutting down the amount of parameters. Unless this is required for this PR due to some nuance with compilation for hybrid platforms, I'd prefer this be reverted.

armcknight · 2023-08-08T00:25:28Z

Sources/Sentry/include/SentryProfiler.h

@@ -43,7 +44,7 @@ SENTRY_EXTERN_C_END
 /**
 * Start a profiler, if one isn't already running.
 */
-+ (void)startWithTracer:(SentryTracer *)tracer;


Same note for type safety here.

armcknight · 2023-08-08T00:32:59Z

Sources/Sentry/SentryProfiler.mm

-        SENTRY_LOG_WARN(@"Expected a profiler for tracer id %@ but none was found",
-            transaction.trace.traceId.sentryIdString);


Nice find 👍🏻

armcknight · 2023-08-08T00:35:23Z

Sources/Sentry/SentryProfiler.mm

+    const auto payload = [self collectProfileBetween:transaction.startSystemTime
+                                                 and:transaction.endSystemTime
+                                            forTrace:transaction.trace.traceId
+                                               onHub:transaction.trace.hub];


Similar thought here w.r.t. parameters, why not just pass the transaction? This is more repetitive and verbose.

vaind · 2023-08-08T06:15:41Z

There is a SentryTracer and SentryTransaction in the dart codebase, so I assume we'd be able to call a function like +[PrivateSentrySDKOnly startProfilingForTrace:(SentryTracer *)] instead of +[PrivateSentrySDKOnly startProfilingForTrace:(SentryId *)].

Yes, in Dart, there are types named the same. However, they are not actually the same types, they're not objective-c SentryTracer and SentryTransaction. So in order to still use those objects, we would need to create them "artificially" in the bridging code and they still would not represent actual objective-c tracer and transaction conceptually.

Instead, it seems a better option to decouple obj-c Profiler from SentryTracer and SentryTransaction, which indeed are there just as wrappers for the string identifier & two timestamps, respectively. To me, this is a case of separation of concerns. Profiler doesn't require a transaction to be running, so it should not depend on it in its argument. What it actually needs is a way to identify a time slice when giving out a profiler slice - and that is achieved with start & end timestamps just fine.

I hope that makes sense to you.

Co-authored-by: Andrew McKnight <[email protected]>

armcknight · 2023-08-09T01:04:20Z

Thanks for the thoughtful reply @vaind . There is an issue though:

Profiler doesn't require a transaction to be running, so it should not depend on it in its argument

This is not true for our other platforms right now, so it would require backend changes to make it so. We currently must always have a transaction associated to each profile for dynamic sampling reasons, and profiles are uploaded as attachment items in transaction envelopes. We can't wind up with a profile which has had its transaction DS'd out, for instance. So we also can't have a profile that never had a transaction in the first place.

The only way for profiler to start currently is when a tracer starts:

sentry-cocoa/Sources/Sentry/SentryTracer.m

Lines 164 to 170 in 279841c

    
           #if SENTRY_TARGET_PROFILING_SUPPORTED 
        
               if (_configuration.profilesSamplerDecision.decision == kSentrySampleDecisionYes) { 
        
                   _isProfiling = YES; 
        
                   _startSystemTime = SentryDependencyContainer.sharedInstance.dateProvider.systemTime; 
        
                   [SentryProfiler startWithTracer:self]; 
        
               } 
        
           #endif // SENTRY_TARGET_PROFILING_SUPPORTED

And then when a tracer ends is the only place a profile can possibly be uploaded:

sentry-cocoa/Sources/Sentry/SentryTracer.m

Lines 526 to 552 in 279841c

    
           #if SENTRY_TARGET_PROFILING_SUPPORTED 
        
               if (self.isProfiling) { 
        
                   [self captureTransactionWithProfile:transaction]; 
        
                   return; 
        
               } 
        
           #endif // SENTRY_TARGET_PROFILING_SUPPORTED 
        
               [_hub captureTransaction:transaction withScope:_hub.scope]; 
        
           } 
        
           #if SENTRY_TARGET_PROFILING_SUPPORTED 
        
           - (void)captureTransactionWithProfile:(SentryTransaction *)transaction 
        
           { 
        
               SentryEnvelopeItem *profileEnvelopeItem = 
        
                   [SentryProfiler createProfilingEnvelopeItemForTransaction:transaction]; 
        
               if (!profileEnvelopeItem) { 
        
                   [_hub captureTransaction:transaction withScope:_hub.scope]; 
        
                   return; 
        
               } 
        
               SENTRY_LOG_DEBUG(@"Capturing transaction with profiling data attached."); 
        
               [_hub captureTransaction:transaction 
        
                                 withScope:_hub.scope 
        
                   additionalEnvelopeItems:@[ profileEnvelopeItem ]]; 
        
           } 
        
           #endif // SENTRY_TARGET_PROFILING_SUPPORTED

FYI, we do want to get to a place where we can separate profiles from transactions, but that is a rough plan for the future.

Does this change how you see the state of things?

vaind · 2023-08-09T15:22:20Z

This is not true for our other platforms right now, so it would require backend changes to make it so. We currently must always have a transaction associated to each profile for dynamic sampling reasons, and profiles are uploaded as attachment items in transaction envelopes. We can't wind up with a profile which has had its transaction DS'd out, for instance. So we also can't have a profile that never had a transaction in the first place.
...
Does this change how you see the state of things?

No, not really, I am aware how profiling works, from other SDKs I've implemented it in.

In case of Flutter, we need to capture a native profile. Therefore, the profile will be collected by the native (cocoa) profiler and then handed over to Flutter where it will be serialized and attached (as an additional envelope item) to the same envelope as the transaction. This is an important point - the transaction attached to the profile is a Dart (Flutter) transaction, being created directly in Dart and going through all the handling, user callbacks, etc. in Dart user code. This transaction has nothing to do with objective-c SentryTransaction & SentryTracer. Therefore, the profiler should be able to run & collect profiles for given periods of time regardless of a running (objective-c) transaction. Also note the distinction: I'm not saying it should run without a running transaction when triggered from objective-c; I'm just saying it should be able to be launched without a transaction, and this is exposed in the "private" hybrid SDK API. This PR changes nothing for native apps.

armcknight

OK, I thought that might be the case. Thanks for explaining @vaind 👍🏻

vaind force-pushed the feat/hybrid-sdk-profiling branch from 4b7a717 to 16507cc Compare August 1, 2023 12:33

vaind force-pushed the feat/hybrid-sdk-profiling branch from 8e0cb3d to 18bb7bd Compare August 2, 2023 07:11

vaind marked this pull request as ready for review August 2, 2023 07:35

vaind requested review from philipphofmann, brustolin and armcknight as code owners August 2, 2023 07:35

vaind changed the title ~~WIP: refactor: allow profiling from hybrid SDK~~ feat: allow profiling from hybrid SDK Aug 2, 2023

vaind changed the title ~~feat: allow profiling from hybrid SDK~~ feat: allow profiling from hybrid SDKs Aug 2, 2023

brustolin reviewed Aug 2, 2023

View reviewed changes

vaind commented Aug 3, 2023

View reviewed changes

vaind added 9 commits August 3, 2023 14:55

refactor: remove Tracer dependency from Profiler

b1c1152

refactor: remove Transaction dependency from profile data slicing

791a94f

refactor: remove Transaction dependency from MetricProfiler serializa…

216a6a0

…tion

refactor: profile envelope item serialization

884f5e2

feat: expose profiler in PrivateSentrySDKOnly

649e10b

fix compilation

455b208

minor changes

4061b68

fix compilation

c50f536

test fix

b21637e

vaind force-pushed the feat/hybrid-sdk-profiling branch from 18bb7bd to b21637e Compare August 3, 2023 12:56

armcknight reviewed Aug 8, 2023

View reviewed changes

Update Sources/Sentry/PrivateSentrySDKOnly.m

c37514a

Co-authored-by: Andrew McKnight <[email protected]>

armcknight approved these changes Aug 10, 2023

View reviewed changes

refactor: extract SentryPlatformName constant

4a8e713

Merge branch 'main' into feat/hybrid-sdk-profiling

22df0d7

vaind force-pushed the feat/hybrid-sdk-profiling branch from e8fdfdc to 22df0d7 Compare August 10, 2023 07:27

vaind merged commit 020745f into main Aug 10, 2023
65 of 66 checks passed

vaind deleted the feat/hybrid-sdk-profiling branch August 10, 2023 08:12

This was referenced Aug 20, 2023

fix: binding missing method getsentry/sentry-dotnet#2558

Merged

Fix CI and dependency bump for iOS getsentry/sentry-dotnet#2559

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow profiling from hybrid SDKs #3194

feat: allow profiling from hybrid SDKs #3194

vaind commented Jul 31, 2023 •

edited

Loading

codecov bot commented Jul 31, 2023 •

edited

Loading

github-actions bot commented Aug 1, 2023 •

edited

Loading

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/hybrid-sdk-profiling

Startup times

App size

github-actions bot commented Aug 2, 2023 •

edited

Loading

brustolin left a comment

vaind Aug 1, 2023

armcknight Aug 7, 2023

bruno-garcia Aug 8, 2023

vaind Aug 8, 2023

vaind Aug 1, 2023

armcknight Aug 8, 2023

vaind Aug 1, 2023

armcknight Aug 8, 2023

armcknight left a comment

armcknight Aug 8, 2023

armcknight Aug 8, 2023

armcknight Aug 8, 2023

armcknight Aug 8, 2023

armcknight Aug 8, 2023

armcknight Aug 8, 2023

vaind commented Aug 8, 2023

armcknight commented Aug 9, 2023

vaind commented Aug 9, 2023 •

edited

Loading

armcknight left a comment

		SENTRY_LOG_WARN(@"Expected a profiler for tracer id %@ but none was found",
		transaction.trace.traceId.sentryIdString);

feat: allow profiling from hybrid SDKs #3194

feat: allow profiling from hybrid SDKs #3194

Conversation

vaind commented Jul 31, 2023 • edited Loading

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

codecov bot commented Jul 31, 2023 • edited Loading

Codecov Report

github-actions bot commented Aug 1, 2023 • edited Loading

Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/hybrid-sdk-profiling

Startup times

App size

github-actions bot commented Aug 2, 2023 • edited Loading

brustolin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armcknight left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaind commented Aug 8, 2023

armcknight commented Aug 9, 2023

vaind commented Aug 9, 2023 • edited Loading

armcknight left a comment

Choose a reason for hiding this comment

vaind commented Jul 31, 2023 •

edited

Loading

codecov bot commented Jul 31, 2023 •

edited

Loading

github-actions bot commented Aug 1, 2023 •

edited

Loading

github-actions bot commented Aug 2, 2023 •

edited

Loading

vaind commented Aug 9, 2023 •

edited

Loading