- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 371
 
          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.  | 
    
          ❌ 2 Tests Failed:
 View the top 3 failed test(s) by shortest run time
 
 
 To view more test analytics, go to the Test Analytics Dashboard  | 
    
captureLog to Hub and Client
      
          Performance metrics 🚀
 
 | 
    
| Revision | Plain | With Sentry | Diff | 
|---|---|---|---|
| 9264ee8 | 1233.15 ms | 1250.02 ms | 16.87 ms | 
| d72784d | 1214.31 ms | 1241.35 ms | 27.04 ms | 
| 8da82b4 | 1220.08 ms | 1248.24 ms | 28.16 ms | 
| 43597ba | 1214.88 ms | 1243.52 ms | 28.65 ms | 
| 3cdbc22 | 1231.63 ms | 1251.06 ms | 19.43 ms | 
| bbe6658 | 1221.00 ms | 1248.51 ms | 27.51 ms | 
| 00d9740 | 1223.53 ms | 1249.75 ms | 26.22 ms | 
| 3067c23 | 1230.48 ms | 1257.90 ms | 27.42 ms | 
| cc7f629 | 1226.00 ms | 1245.51 ms | 19.51 ms | 
| 748df9d | 1231.63 ms | 1259.47 ms | 27.84 ms | 
App size
| Revision | Plain | With Sentry | Diff | 
|---|---|---|---|
| 9264ee8 | 23.75 KiB | 913.09 KiB | 889.34 KiB | 
| d72784d | 23.75 KiB | 988.01 KiB | 964.27 KiB | 
| 8da82b4 | 23.75 KiB | 913.63 KiB | 889.88 KiB | 
| 43597ba | 23.75 KiB | 880.32 KiB | 856.58 KiB | 
| 3cdbc22 | 23.75 KiB | 928.14 KiB | 904.40 KiB | 
| bbe6658 | 23.75 KiB | 908.02 KiB | 884.27 KiB | 
| 00d9740 | 23.75 KiB | 938.32 KiB | 914.57 KiB | 
| 3067c23 | 23.75 KiB | 928.15 KiB | 904.40 KiB | 
| cc7f629 | 23.75 KiB | 878.48 KiB | 854.73 KiB | 
| 748df9d | 23.75 KiB | 902.48 KiB | 878.74 KiB | 
Previous results on branch: denrase/logs-in-hub-and-client
Startup times
| Revision | Plain | With Sentry | Diff | 
|---|---|---|---|
| db59d5d | 1217.41 ms | 1240.59 ms | 23.18 ms | 
| 391bd71 | 1231.72 ms | 1262.83 ms | 31.11 ms | 
| 31d864b | 1212.77 ms | 1246.85 ms | 34.08 ms | 
| 225f528 | 1211.81 ms | 1252.58 ms | 40.77 ms | 
| 9df70d3 | 1228.40 ms | 1262.55 ms | 34.16 ms | 
| 08821c7 | 1206.15 ms | 1239.35 ms | 33.20 ms | 
| a429649 | 1216.39 ms | 1217.77 ms | 1.39 ms | 
App size
| Revision | Plain | With Sentry | Diff | 
|---|---|---|---|
| db59d5d | 23.75 KiB | 1.01 MiB | 1006.79 KiB | 
| 391bd71 | 23.75 KiB | 1.00 MiB | 1004.71 KiB | 
| 31d864b | 23.75 KiB | 1.00 MiB | 1003.12 KiB | 
| 225f528 | 23.75 KiB | 1.01 MiB | 1006.79 KiB | 
| 9df70d3 | 23.75 KiB | 1.01 MiB | 1006.51 KiB | 
| 08821c7 | 23.75 KiB | 1.02 MiB | 1017.66 KiB | 
| a429649 | 23.75 KiB | 1.00 MiB | 1002.92 KiB | 
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.
Thanks for tackling this @denrase. I found a few high-level comments to address first, before I give this another pass.
        
          
                Sources/Sentry/SentryClient.m
              
                Outdated
          
        
      | // This is a workaround for experimental logs, until we'll write batched logs to disk, | ||
| // to avoid data loss due to crashes. This is a trade-off until then. | 
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.
l: Logs aren't experimental anymore with V9. We're close to merging the develop docs PR for logs for crashes getsentry/sentry-docs#15274. Maybe we could link this PR already or create an issue for logs for crashes and link it here.
| ) { | ||
| 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.
m: I just noticed that this doesn't seem right. When checking the develop docs: https://develop.sentry.dev/sdk/telemetry/logs/#log-envelope-item-payload
The trace id should be grabbed from the current propagation context in the SDK.
Now we always set this to an empty one and then we overwrite it here
sentry-cocoa/Sources/Swift/Tools/SentryLogBatcher.swift
Lines 73 to 74 in 4446d9f
| let propagationContextTraceIdString = scope.propagationContextTraceIdString | |
| log.traceId = SentryId(uuidString: propagationContextTraceIdString) | 
So no matter what we set here, we're going to overwrite it. Can you please double check how other SDKs do 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.
SentryLog passes it down to the hub, client and batcher, it's set there. Since it;s a required value, it needs to be non-null, so we set it to an intermediate value.
Users can modify it in the beforeSendLog callback, in line with every other property. That's why it is not exposed through the constructors. What I do not know is if this a valid use case.
In Flutter we have documented this behaviour.
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.
# Conflicts: # Sources/Swift/Tools/SentryLogger.swift # Tests/SentryTests/SentryLoggerTests.swift # Tests/SentryTests/SentrySDKInternalTests.swift
# Conflicts: # Sentry.xcodeproj/project.pbxproj
# 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
      | } | ||
| } | ||
| log.attributes = mutableAttributes; | ||
| #endif | 
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.
Bug: Swift-Objective-C mismatch crashes from log attributes
The captureLog method creates SentryStructuredLogAttribute objects and assigns them to log.attributes, which is a Swift property expecting SentryLog.Attribute values. This type mismatch across the Swift-Objective-C boundary can cause runtime errors or crashes when log attributes are accessed.
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.
| - (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]; | |
| } | 
| 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.
| // 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.
| ) { | ||
| 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?
| self.logBatcher = [[SentryLogBatcher alloc] | ||
| initWithOptions:options | ||
| dispatchQueue:SentryDependencyContainer.sharedInstance.dispatchQueueWrapper]; | ||
| self.logBatcher.delegate = self; | 
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 think we could already pass the SentryLogBatcherDelegate in via the init method and make the property private to the SentryLogBatcher.
| // Send the payload. | ||
| 
               | 
          ||
| client.captureLogsData(payloadData, with: NSNumber(value: encodedLogs.count)) | ||
| delegate?.capture(logsData: payloadData as NSData, count: NSNumber(value: encodedLogs.count)) | 
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 think we should add a debug log when the delegate is nil, otherwise it would just silently fail here, and no logs get sent. It's unlikely that this happens right now, but as the code changes this could become useful.
📜 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.