-
-
Notifications
You must be signed in to change notification settings - Fork 366
feat: Add SentryDistribution #6149
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
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6149 +/- ##
=============================================
+ Coverage 86.807% 86.812% +0.005%
=============================================
Files 438 438
Lines 37323 37323
Branches 17434 17436 +2
=============================================
+ Hits 32399 32401 +2
+ Misses 4879 4876 -3
- Partials 45 46 +1 see 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
e3ebff3 | 1223.47 ms | 1249.27 ms | 25.80 ms |
5218a05 | 1236.35 ms | 1245.41 ms | 9.06 ms |
7bb24a2 | 1229.16 ms | 1256.66 ms | 27.50 ms |
8bb3e37 | 1235.18 ms | 1252.63 ms | 17.45 ms |
9ec4c56 | 1222.69 ms | 1248.77 ms | 26.08 ms |
fdea6f5 | 1216.08 ms | 1241.82 ms | 25.73 ms |
0b5fd21 | 1237.52 ms | 1251.36 ms | 13.84 ms |
55f739c | 1226.06 ms | 1248.78 ms | 22.71 ms |
073562b | 1232.63 ms | 1259.88 ms | 27.24 ms |
4a7a005 | 1229.15 ms | 1243.35 ms | 14.20 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
e3ebff3 | 23.75 KiB | 878.48 KiB | 854.73 KiB |
5218a05 | 23.75 KiB | 984.71 KiB | 960.96 KiB |
7bb24a2 | 23.75 KiB | 973.68 KiB | 949.93 KiB |
8bb3e37 | 23.75 KiB | 968.25 KiB | 944.50 KiB |
9ec4c56 | 23.75 KiB | 921.20 KiB | 897.45 KiB |
fdea6f5 | 23.75 KiB | 867.15 KiB | 843.40 KiB |
0b5fd21 | 23.75 KiB | 912.78 KiB | 889.03 KiB |
55f739c | 23.75 KiB | 858.73 KiB | 834.98 KiB |
073562b | 23.75 KiB | 927.06 KiB | 903.31 KiB |
4a7a005 | 23.75 KiB | 979.96 KiB | 956.22 KiB |
Previous results on branch: addDistribution
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9478181 | 1222.62 ms | 1247.98 ms | 25.35 ms |
e4e9fa7 | 1234.24 ms | 1261.62 ms | 27.38 ms |
b03d8aa | 1233.00 ms | 1255.20 ms | 22.20 ms |
b606360 | 1235.00 ms | 1255.91 ms | 20.91 ms |
7a3001a | 1229.40 ms | 1262.56 ms | 33.16 ms |
a40faec | 1206.49 ms | 1231.98 ms | 25.49 ms |
4efe26d | 1208.45 ms | 1238.27 ms | 29.82 ms |
2c7d1f4 | 1225.24 ms | 1249.27 ms | 24.02 ms |
5b58113 | 1221.59 ms | 1248.87 ms | 27.27 ms |
cde82dd | 1218.43 ms | 1241.29 ms | 22.86 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9478181 | 23.75 KiB | 974.94 KiB | 951.20 KiB |
e4e9fa7 | 23.75 KiB | 974.94 KiB | 951.20 KiB |
b03d8aa | 23.75 KiB | 974.89 KiB | 951.14 KiB |
b606360 | 23.75 KiB | 980.80 KiB | 957.05 KiB |
7a3001a | 23.75 KiB | 979.09 KiB | 955.35 KiB |
a40faec | 23.75 KiB | 974.89 KiB | 951.14 KiB |
4efe26d | 23.75 KiB | 977.31 KiB | 953.57 KiB |
2c7d1f4 | 23.75 KiB | 979.96 KiB | 956.21 KiB |
5b58113 | 23.75 KiB | 974.89 KiB | 951.14 KiB |
cde82dd | 23.75 KiB | 979.96 KiB | 956.21 KiB |
b403705
to
ea23f05
Compare
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.
Left some comments. As we start a new SDK here, we can make sure to build a best-practice solution from the beginning.
Why do you want to skip the changelog when introducing such a big new feature? |
I don't think we would actually skip the changelog, I just want to align on the API before writing a changelog, and don't want the bot to post that big comment on the PR |
08e210c
to
67dd23f
Compare
67dd23f
to
813776f
Compare
813776f
to
36f7403
Compare
36f7403
to
da7ce75
Compare
da7ce75
to
344978f
Compare
609afa7
to
919ce11
Compare
Thanks @philprime for the review, just updated it. PTAL whenever you get a chance |
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.
Good progress. I left some further comments to consider.
Regarding this comment:
I don't think we would actually skip the changelog, I just want to align on the API before writing a changelog, and don't want the bot to post that big comment on the PR
In that case it would still be better to have a draft changelog message, because right now it would just skip the CI job due to the skip-changelog
, also not blocking any auto-merge attempt.
c13bea0
to
fcf0050
Compare
273faf4
to
d3e6583
Compare
Thanks @philprime it's ready for another pass whenever you get a chance! |
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.
Cursor left another comment you might want to consider, other than that LGTM. Thanks for applying all my feedback.
I enhanced the
3 tasks completed
Learn more about Cursor Agents |
@noahsmartin I did not expect the bot to change the PR, you might want to revert the last commit |
afe992e
to
ea50306
Compare
ea50306
to
6e87db3
Compare
Adds the ETDistribution model to the package.swift
Intentionally not supporting cocoapods or usage from ObjC for now, that can come in a future version.
There are a few ways we could expose this API:
SentrySDK
naming so you would do something likeSentrySDK.checkForUpdates()
SentrySDK
. This is not preferred because it means users would have to depend on both the Sentry target, and the distribution target.Sentry.SentrySDK
andSentryDistribution.SentrySDK
but most of the time you wouldn't need the module prefixes if they are being used in different files.Distribution
. This is what this PR does. The API is:Closes #6173
Note
Adds new
SentryDistribution
SwiftPM target with update-checking API, includes sample app and CI build/test coverage.SentryDistribution
target andSentryDistributionTests
inPackage.swift
.Updater
, models, networking, Mach-O UUID parsing, utils) underSources/SentryDistribution
.Samples/DistributionSample
app (XcodeGen spec, SwiftUI app, update flow) and include it in workspace and Makefile..github/actions/prepare-package.swift
withremove-duplicate
andchange-path
inputs and conditional steps.build-distribution
job to buildSentryDistribution
framework.distribution-tests
job to run SPM package tests (with duplicate target removal), and wire into required checks.DistributionSample
scheme.SentryDistribution
as a SwiftPM target.Written by Cursor Bugbot for commit 6e87db3. This will update automatically on new commits. Configure here.