-
-
Notifications
You must be signed in to change notification settings - Fork 372
Structured Logs: Add log APIs to Hub and Client
#6518
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
base: main
Are you sure you want to change the base?
Conversation
|
@philipphofmann Hacked this together. Works locally. If there are no other blockers, everything should work out without breaking existing public API. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6518 +/- ##
=============================================
- Coverage 85.648% 85.119% -0.529%
=============================================
Files 452 452
Lines 27655 27688 +33
Branches 12118 12124 +6
=============================================
- Hits 23686 23568 -118
- Misses 3694 3833 +139
- Partials 275 287 +12
... and 15 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
captureLog to Hub and Client
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 3279d4e | 1215.76 ms | 1256.45 ms | 40.69 ms |
| 37183fe | 1212.33 ms | 1238.92 ms | 26.59 ms |
| 80a5166 | 1224.49 ms | 1251.29 ms | 26.80 ms |
| 0b5fd21 | 1237.52 ms | 1251.36 ms | 13.84 ms |
| 8745cc0 | 1228.13 ms | 1250.48 ms | 22.35 ms |
| ebc72be | 1221.24 ms | 1249.66 ms | 28.42 ms |
| 0b6776b | 1230.18 ms | 1262.06 ms | 31.88 ms |
| 6e99155 | 1223.96 ms | 1249.25 ms | 25.29 ms |
| e8f9a1d | 1229.02 ms | 1264.17 ms | 35.15 ms |
| 535ebd9 | 1194.59 ms | 1219.84 ms | 25.26 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 3279d4e | 23.75 KiB | 938.32 KiB | 914.57 KiB |
| 37183fe | 23.75 KiB | 913.63 KiB | 889.87 KiB |
| 80a5166 | 23.75 KiB | 904.53 KiB | 880.78 KiB |
| 0b5fd21 | 23.75 KiB | 912.78 KiB | 889.03 KiB |
| 8745cc0 | 23.74 KiB | 971.81 KiB | 948.07 KiB |
| ebc72be | 23.75 KiB | 908.22 KiB | 884.47 KiB |
| 0b6776b | 23.75 KiB | 968.23 KiB | 944.49 KiB |
| 6e99155 | 23.75 KiB | 963.18 KiB | 939.43 KiB |
| e8f9a1d | 23.75 KiB | 969.78 KiB | 946.04 KiB |
| 535ebd9 | 23.75 KiB | 1008.67 KiB | 984.92 KiB |
Previous results on branch: denrase/logs-in-hub-and-client
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 391bd71 | 1231.72 ms | 1262.83 ms | 31.11 ms |
| db59d5d | 1217.41 ms | 1240.59 ms | 23.18 ms |
| cf93b3b | 1224.50 ms | 1246.35 ms | 21.85 ms |
| 31d864b | 1212.77 ms | 1246.85 ms | 34.08 ms |
| 08821c7 | 1206.15 ms | 1239.35 ms | 33.20 ms |
| 9df70d3 | 1228.40 ms | 1262.55 ms | 34.16 ms |
| a429649 | 1216.39 ms | 1217.77 ms | 1.39 ms |
| 225f528 | 1211.81 ms | 1252.58 ms | 40.77 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 391bd71 | 23.75 KiB | 1.00 MiB | 1004.71 KiB |
| db59d5d | 23.75 KiB | 1.01 MiB | 1006.79 KiB |
| cf93b3b | 23.75 KiB | 1.00 MiB | 1003.12 KiB |
| 31d864b | 23.75 KiB | 1.00 MiB | 1003.12 KiB |
| 08821c7 | 23.75 KiB | 1.02 MiB | 1017.66 KiB |
| 9df70d3 | 23.75 KiB | 1.01 MiB | 1006.51 KiB |
| a429649 | 23.75 KiB | 1.00 MiB | 1002.92 KiB |
| 225f528 | 23.75 KiB | 1.01 MiB | 1006.79 KiB |
# Conflicts: # Sources/Sentry/SentryClient.m # Sources/Sentry/SentryHub.m # Sources/Sentry/include/SentryClient+Logs.h # Sources/Swift/Helper/SentrySDK.swift # Sources/Swift/Tools/SentryLogBatcher.swift # Sources/Swift/Tools/SentryLogger.swift # sdk_api.json
captureLog to Hub and ClientHub and Client
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.
We slowly getting to LGTM. Thanks @denrase for including the feedback.
| // Do not use this directly, instead use the non-underscored `captureLog` method that is | ||
| // defined through a SentryClient.swift file. | ||
| - (void)_swiftCaptureLog:(NSObject *)log withScope:(SentryScope *)scope; |
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: Can't we move that to SentryClient+Private.h? If not, please double-check with Noah if he has an idea how to avoid this. I would really like to avoid this. The same applies to the _swiftLogger of the SentryHub.
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 issue is the circular dependency with Swift/ObjC. Checked and this seems to be how it's done elsewhere in the SDK. One solution would be to move SentryLog to ObjC implementations, then it's not needed.
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.
Checked, using SentryLog (Swift) in ObjC SentryClientInternal.h and then trying to call it from Swift again does only work for non-SPM build. @noahsmartin Do you have any suggestion on how to better handle this?
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.
@noahsmartin Pretty much the same approach as we have here #5329, with the difference being that we are here now on an internal class, which does make it more risky, but we'll get a compile issue when we change the method signature, since we are no using dynamism here.
| - (void)_swiftCaptureLog:(NSObject *)log withScope:(SentryScope *)scope | ||
| { | ||
| if ([log isKindOfClass:[SentryLog class]]) { | ||
| [self.logBatcher addLog:(SentryLog *)log scope:scope]; | ||
| } | ||
| } |
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.
m: We use SentryLog below. What drove you to use NSObject here. Can't we use SentryLog?
| - (void)_swiftCaptureLog:(NSObject *)log withScope:(SentryScope *)scope | |
| { | |
| if ([log isKindOfClass:[SentryLog class]]) { | |
| [self.logBatcher addLog:(SentryLog *)log scope:scope]; | |
| } | |
| } | |
| - (void)_swiftCaptureLog:(SentryLog *)log withScope:(SentryScope *)scope | |
| { | |
| [self.logBatcher addLog:log scope:scope]; | |
| } |
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.
See above, cause this is a Swift Type used in ObjC, once we call the internal hub from Swift (when using SPM), it's not visible.
Sources/Sentry/SentryHub.m
Outdated
| if (client != nil) { | ||
| #if SENTRY_TARGET_REPLAY_SUPPORTED | ||
| NSMutableDictionary<NSString *, SentryStructuredLogAttribute *> *mutableAttributes = | ||
| [log.attributes mutableCopy]; |
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.
m: We can avoid allocating this mutable copy. We only need to add the sentry.replay_id attribute when SR is active. It would be great to modify the code to do so.
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 swift dict is visibla als non-mutable in ObjC. I have added a helper method to set individual attributes.
| // swiftlint:disable force_cast | ||
| return SentrySDKInternal.currentHub()._swiftLogger as! SentryLogger | ||
| // swiftlint:enable force_cast |
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.
m: I would prefer getting rid of this force cast by trying to change the type of the _swiftLogger property to SentryLogger. SentryLogger is public so it should work.
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.
Same issue as above, when using SPM, logger would not be visible here due tu circular dependency between Swift/Objc parts of the SDK.
| ) { | ||
| self.init( | ||
| timestamp: Date(), | ||
| traceId: SentryId.empty, |
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.
OK, then let's keep it. Please add a comment explaining this.
| // that limitation by providing Swift-native methods and properties that use dynamic | ||
| // dispatch internally. | ||
|
|
||
| #if SWIFT_PACKAGE |
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.
@noahsmartin, can you please double-check if this approach works?
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 existing code, I just moved it to its own file so it stays on our radar. Circular dependency issue when installing through SPM, before that, the SPM target on CI would not compile.
…ds/.property to +Private header
…/sentry-cocoa into denrase/logs-in-hub-and-client
📜 Description
capture(log:scope:)methods toClientloggerAPI toHubSentryLog, so users can actually create them for hub/client method usage.💡 Motivation and Context
Closes #6503
💚 How did you test it?
Unit tests
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.