-
-
Notifications
You must be signed in to change notification settings - Fork 368
Structured Logs: Add SentrySwiftLog Integration
#6286
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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6286 +/- ##
=============================================
+ Coverage 86.818% 86.837% +0.018%
=============================================
Files 438 439 +1
Lines 37311 37356 +45
Branches 17428 17429 +1
=============================================
+ Hits 32393 32439 +46
+ Misses 4874 4643 -231
- Partials 44 274 +230
... and 38 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 2ec2700 | 1238.28 ms | 1258.82 ms | 20.53 ms |
| d0f70ce | 1226.54 ms | 1247.04 ms | 20.50 ms |
| 5bf2b17 | 1213.53 ms | 1238.54 ms | 25.01 ms |
| efab7d3 | 1219.98 ms | 1252.12 ms | 32.14 ms |
| e3ebff3 | 1223.47 ms | 1249.27 ms | 25.80 ms |
| 7b3399c | 1193.35 ms | 1227.38 ms | 34.03 ms |
| 3133d0e | 1237.86 ms | 1262.87 ms | 25.01 ms |
| 22b6996 | 1234.00 ms | 1263.24 ms | 29.24 ms |
| 43597ba | 1214.88 ms | 1243.52 ms | 28.65 ms |
| fc6557e | 1226.40 ms | 1249.88 ms | 23.48 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 2ec2700 | 23.75 KiB | 980.80 KiB | 957.05 KiB |
| d0f70ce | 23.75 KiB | 913.09 KiB | 889.34 KiB |
| 5bf2b17 | 23.75 KiB | 913.27 KiB | 889.52 KiB |
| efab7d3 | 23.75 KiB | 912.78 KiB | 889.03 KiB |
| e3ebff3 | 23.75 KiB | 878.48 KiB | 854.73 KiB |
| 7b3399c | 23.75 KiB | 946.68 KiB | 922.94 KiB |
| 3133d0e | 23.74 KiB | 976.79 KiB | 953.04 KiB |
| 22b6996 | 23.75 KiB | 908.02 KiB | 884.27 KiB |
| 43597ba | 23.75 KiB | 880.32 KiB | 856.58 KiB |
| fc6557e | 23.75 KiB | 866.68 KiB | 842.93 KiB |
# Conflicts: # Package.swift # Sentry.xcodeproj/project.pbxproj
swift-log IntegrationSentrySwiftLog Integration
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.
Looks good, just a small comment
| # Clean up orphaned commas and fix syntax | ||
| sed -i '' '/^[[:space:]]*,$/d' $PACKAGE_FILE | ||
| sed -i '' 's/name: "Sentry\(-.*\)\?"$/name: "Sentry\1",/g' $PACKAGE_FILE | ||
| sed -i '' 's/platforms: \[\.iOS(\.v11), \.macOS(\.v10_13), \.tvOS(\.v11), \.watchOS(\.v4)\]$/platforms: [.iOS(.v11), .macOS(.v10_13), .tvOS(.v11), .watchOS(.v4)],/g' $PACKAGE_FILE |
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.
What is this trying to do?
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.
One of the CI jobs is replacing dependencies with local build artefacts. This broke, as swift-log was added to our Package.swift. This code fixes those issues.
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.
Does this only happen because of swift-log or should we do the fix in a separate PR?
| .library(name: "SentrySwiftUI", targets: ["Sentry", "SentrySwiftUI", "SentryCppHelper"]), | ||
| .library(name: "SentryDistribution", targets: ["SentryDistribution"]) | ||
| .library(name: "SentryDistribution", targets: ["SentryDistribution"]), | ||
| .library(name: "SentrySwiftLog", targets: ["Sentry", "SentrySwiftLog"]) |
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 don't fully understand why this is another target. Is this only here because swift-log is a new dependency? Shouldn't we add it as a dependency to Sentry instead?
| platforms: [.iOS(.v15), .macOS(.v12), .tvOS(.v15), .watchOS(.v8)], | ||
| products: products, | ||
| dependencies: [ | ||
| .package(url: "https://github.com/apple/swift-log", from: "1.6.0") |
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: So I think this is going to be a problem as soon as new swift-log version becomes available. AFAIK the SPM doesn't support peer-dependencies, so if someone uses swift-log with a version not being 1.6.0, while we depend on it, Xcode will not build because it doesn't support the same dependency with different versions like NPM.
So basically this will lock our SDK users into exactly the swift-log version we declare, requiring us to release a new SDK version with every update of swift-log, furthermore requiring users to update the Sentry SDK even though they just want a new version of swift-log. At this point I do not have a proposal how to fix this other than asking the user to "inject" their swift-log version by making it conform to a Sentry-included protocol
|
|
||
| Removes deprecated user feedback API, this is replaced with the new feedback API (#5591) | ||
| Removes `enablePerformanceV2` option and makes this the default. The app start duration will now finish when the first frame is drawn instead of when the OS posts the UIWindowDidBecomeVisibleNotification. (#6008) | ||
| Structured Logs: Move options out of experimental (#6359) |
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: Why is one list with - and one without?
📜 Description
The Sentry
swift-logintegration.Setup
SentrySwiftLogas a target toSentrytargetSentrySwiftLogtoPackage.swiftiOS15-SwiftUiapp so you can play around with itImplementation
SentryLogHandlersentry-logmetadata is mapped to attributes and prefixed withswift-logDiscussion
Here vs Own Repo
Added the integration to this repo, because it is easier for us to maintain.
On the
swift-logrepo integrations section is linking to the swift package index with search termswift-log. So for visibility, we'd need to have a separateswift-log-sentryrepo.https://github.com/apple/swift-log?tab=readme-ov-file#available-log-handler-backends
https://swiftpackageindex.com/search?query=swift-log
Distribution
Only distributed through SPM, as
swift-logdropped support for CocoaPods. Also, we'll not be able to distribute this as a binary, asswift-logis not supporting that.💡 Motivation and Context
Closes #5372
💚 How did you test it?
Unit Tests
Sample App
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.