-
-
Notifications
You must be signed in to change notification settings - Fork 323
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.
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
chore: update profiling API for hybrid SDKs #3255
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3255 +/- ##
=============================================
+ Coverage 89.150% 89.180% +0.030%
=============================================
Files 502 502
Lines 54113 54136 +23
Branches 19428 19424 -4
=============================================
+ Hits 48242 48279 +37
+ Misses 5011 5001 -10
+ Partials 860 856 -4
... and 11 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
17afc4b | 1228.94 ms | 1251.10 ms | 22.16 ms |
ab6fee4 | 1227.63 ms | 1239.80 ms | 12.17 ms |
98a8c16 | 1234.69 ms | 1265.02 ms | 30.33 ms |
9e6665f | 1233.33 ms | 1257.14 ms | 23.81 ms |
20163bb | 1248.92 ms | 1258.48 ms | 9.56 ms |
279841c | 1237.29 ms | 1254.12 ms | 16.83 ms |
075a044 | 1237.26 ms | 1255.36 ms | 18.10 ms |
be51b56 | 1220.84 ms | 1249.36 ms | 28.52 ms |
a6c634b | 1235.86 ms | 1250.53 ms | 14.67 ms |
d3edf46 | 1213.00 ms | 1227.46 ms | 14.46 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
17afc4b | 20.76 KiB | 436.25 KiB | 415.49 KiB |
ab6fee4 | 22.84 KiB | 401.67 KiB | 378.83 KiB |
98a8c16 | 20.76 KiB | 431.00 KiB | 410.24 KiB |
9e6665f | 22.84 KiB | 403.52 KiB | 380.67 KiB |
20163bb | 20.76 KiB | 426.82 KiB | 406.06 KiB |
279841c | 22.84 KiB | 403.19 KiB | 380.34 KiB |
075a044 | 20.76 KiB | 420.41 KiB | 399.65 KiB |
be51b56 | 20.76 KiB | 432.20 KiB | 411.44 KiB |
a6c634b | 20.76 KiB | 393.37 KiB | 372.61 KiB |
d3edf46 | 20.76 KiB | 436.65 KiB | 415.89 KiB |
d7f2a57
to
26003e6
Compare
+ (nullable NSDictionary<NSString *, id> *)collectProfileBetween:(uint64_t)startSystemTime | ||
and:(uint64_t)endSystemTime | ||
forTrace:(SentryId *)traceId; |
There was a problem hiding this comment.
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.
+ (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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good, just one point to review.
I would like for @armcknight to also review this since he has more context about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General direction of the PR looks fine, I am just curious about what we're doing with the active thread ID.
|
||
if (payload != nil) { | ||
payload[@"platform"] = SentryPlatformName; | ||
payload[@"transaction"] = @{ | ||
@"active_thread_id" : | ||
[NSNumber numberWithLongLong:sentry::profiling::ThreadHandle::current()->tid()] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🤷
I didn't realize this had automatic merge enabled. @vaind please respond to the feedback from myself and @brustolin |
📜 Description
Some changes coming from actual integration into the Flutter SDK.
💡 Motivation and Context
💚 How did you test it?
📝 Checklist
You have to check all boxes before merging:
sendDefaultPII
is enabled.🔮 Next steps
#skip-changelog