Skip to content
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

ref: Remove SentryPrivate #3623

Merged
merged 78 commits into from
Mar 4, 2024
Merged

Conversation

brustolin
Copy link
Contributor

@brustolin brustolin commented Feb 8, 2024

📜 Description

We have SentryPrivate because SPM dont support two languages in the same project.
This two frameworks are causing some problems for projects that download Sentry code and build it themselves.

💡 Motivation and Context

close #3622

💚 How did you test it?

Ci

📝 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

Copy link

github-actions bot commented Feb 8, 2024

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

  • Sources/Sentry/SentryNetworkTracker.m

Copy link

github-actions bot commented Feb 8, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1231.84 ms 1246.60 ms 14.77 ms
Size 21.58 KiB 539.86 KiB 518.28 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
3297d6e 1219.02 ms 1243.25 ms 24.23 ms
c5ff7b8 1226.68 ms 1235.04 ms 8.36 ms
48e8c2e 1222.98 ms 1237.78 ms 14.80 ms
d3630c3 1229.78 ms 1241.94 ms 12.16 ms
8838e54 1252.06 ms 1258.20 ms 6.14 ms
adcc7d8 1225.90 ms 1245.08 ms 19.18 ms
fb53d97 1226.08 ms 1245.12 ms 19.04 ms
005bb4c 1237.38 ms 1255.54 ms 18.16 ms
c76f9b0 1249.78 ms 1265.14 ms 15.36 ms
7fb7afb 1230.12 ms 1251.04 ms 20.92 ms

App size

Revision Plain With Sentry Diff
3297d6e 21.58 KiB 418.45 KiB 396.86 KiB
c5ff7b8 22.85 KiB 414.80 KiB 391.95 KiB
48e8c2e 21.58 KiB 418.44 KiB 396.86 KiB
d3630c3 22.84 KiB 403.19 KiB 380.34 KiB
8838e54 22.85 KiB 413.45 KiB 390.60 KiB
adcc7d8 20.76 KiB 426.15 KiB 405.39 KiB
fb53d97 20.76 KiB 425.81 KiB 405.04 KiB
005bb4c 20.76 KiB 419.70 KiB 398.94 KiB
c76f9b0 22.85 KiB 406.69 KiB 383.84 KiB
7fb7afb 20.76 KiB 419.70 KiB 398.94 KiB

Previous results on branch: fix/no-sentryprivate-for-self-build

Startup times

Revision Plain With Sentry Diff
e63fdc9 1240.30 ms 1258.02 ms 17.72 ms
f55f6c8 1234.67 ms 1247.78 ms 13.10 ms
e7e4378 1235.87 ms 1254.27 ms 18.39 ms
0a55ccb 1203.04 ms 1215.84 ms 12.80 ms
a56a8da 1233.69 ms 1246.35 ms 12.65 ms
1078ecb 1234.45 ms 1249.02 ms 14.57 ms
76f64c5 1205.51 ms 1213.22 ms 7.71 ms
8ac547e 1214.57 ms 1222.44 ms 7.87 ms
55cb5f2 1237.29 ms 1255.88 ms 18.59 ms
b399df4 1201.65 ms 1212.37 ms 10.71 ms

App size

Revision Plain With Sentry Diff
e63fdc9 21.58 KiB 419.68 KiB 398.10 KiB
f55f6c8 21.58 KiB 419.81 KiB 398.23 KiB
e7e4378 21.58 KiB 539.75 KiB 518.17 KiB
0a55ccb 21.58 KiB 539.75 KiB 518.17 KiB
a56a8da 21.58 KiB 419.81 KiB 398.23 KiB
1078ecb 21.58 KiB 539.75 KiB 518.17 KiB
76f64c5 21.58 KiB 539.75 KiB 518.16 KiB
8ac547e 21.58 KiB 419.81 KiB 398.23 KiB
55cb5f2 21.58 KiB 419.81 KiB 398.23 KiB
b399df4 21.58 KiB 419.81 KiB 398.23 KiB

Copy link

github-actions bot commented Feb 8, 2024

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

  • Sources/Sentry/SentryNetworkTracker.m

Copy link

github-actions bot commented Feb 8, 2024

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

  • Sources/Sentry/SentryNetworkTracker.m

Copy link

github-actions bot commented Feb 9, 2024

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

  • Sources/Sentry/SentryNetworkTracker.m

Copy link

codecov bot commented Feb 9, 2024

Codecov Report

Attention: Patch coverage is 84.61538% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 89.297%. Comparing base (6569103) to head (5f46b57).

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3623       +/-   ##
=============================================
+ Coverage   89.246%   89.297%   +0.051%     
=============================================
  Files          534       534               
  Lines        58940     58969       +29     
  Branches     21158     21168       +10     
=============================================
+ Hits         52602     52658       +56     
+ Misses        5302      5277       -25     
+ Partials      1036      1034        -2     
Files Coverage Δ
Sources/Sentry/SentryCoreDataTracker.m 94.482% <ø> (ø)
Sources/Sentry/SentryNetworkTracker.m 92.650% <ø> (+0.516%) ⬆️
Sources/Sentry/SentryTracer.m 96.309% <ø> (ø)
Sources/Sentry/SentryUIApplication.m 46.198% <ø> (ø)
Sources/Sentry/SentryViewHierarchy.m 98.979% <ø> (ø)
Sources/Swift/MetricKit/SentryMXManager.swift 100.000% <100.000%> (ø)
Sources/Swift/SwiftDescriptor.swift 100.000% <100.000%> (ø)
Sources/Swift/Tools/HTTPHeaderSanitizer.swift 100.000% <100.000%> (ø)
Sources/Swift/Tools/UrlSanitized.swift 95.238% <100.000%> (ø)
...rations/MetricKit/SentryMXCallStackTreeTests.swift 98.630% <ø> (ø)
... and 7 more

... and 14 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6569103...5f46b57. Read the comment docs.

Copy link

github-actions bot commented Feb 9, 2024

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

  • Sources/Sentry/SentryNetworkTracker.m

Copy link

github-actions bot commented Feb 9, 2024

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

  • Sources/Sentry/SentryNetworkTracker.m

@brustolin brustolin changed the title fix: Remove SentryPrivate from self build fix: Remove SentryPrivate from self build and Cocoapod Feb 9, 2024
CHANGELOG.md Outdated Show resolved Hide resolved
@brustolin brustolin marked this pull request as ready for review February 9, 2024 10:59
Copy link

github-actions bot commented Feb 9, 2024

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

  • Sources/Sentry/SentryNetworkTracker.m

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @brustolin 👍. LGTM

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Philipp Hofmann <[email protected]>
Copy link

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

  • Sources/Sentry/SentryNetworkTracker.m

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the most exiting PR in 2024 so far 🥇😃.

I think we should add an entry to the internal decision log to explain why we chose this approach and that the biggest benefit is that we can mix Swift with ObjC code. Furthermore, I think we should somewhere in the code, maybe in build.yml, explain how this process exactly works, because it's not obvious.

.github/workflows/benchmarking.yml Show resolved Hide resolved
.github/last-release-runid Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
Package.swift Show resolved Hide resolved
Tests/Perf/metrics-test.yml Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
Sources/Configuration/SentrySwiftUI.xcconfig Show resolved Hide resolved
scripts/update-package-sha.sh Show resolved Hide resolved
scripts/build-xcframework.sh Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Mar 1, 2024

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

  • Sources/Sentry/SentryNetworkTracker.m

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another quick pass.

.github/workflows/release.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Mar 1, 2024

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

  • Sources/Sentry/SentryNetworkTracker.m

@brustolin brustolin changed the title fix: Remove SentryPrivate from self build and Cocoapod ref: Remove SentryPrivate Mar 1, 2024
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Mar 1, 2024

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

  • Sources/Sentry/SentryNetworkTracker.m

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but please wait with merging until we release 8.21.0.

Copy link

github-actions bot commented Mar 4, 2024

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

  • Sources/Sentry/SentryNetworkTracker.m

Copy link

github-actions bot commented Mar 4, 2024

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

  • Sources/Sentry/SentryNetworkTracker.m

@brustolin brustolin merged commit e3adcd5 into main Mar 4, 2024
69 of 70 checks passed
@brustolin brustolin deleted the fix/no-sentryprivate-for-self-build branch March 4, 2024 13:37
philipphofmann added a commit that referenced this pull request Mar 18, 2024
We removed SentryPrivate with GH-3623, so we also have to remove it
from the version bump.
philipphofmann added a commit that referenced this pull request Mar 18, 2024
We removed SentryPrivate with GH-3623, so we also have to remove it
from the version bump.
threema-matteo pushed a commit to threema-ch/sentry-cocoa that referenced this pull request May 21, 2024
We have SentryPrivate because SPM dont support two languages in the same project.
This two frameworks are causing some problems for projects that download Sentry code and build it themselves.

Co-authored-by: Philipp Hofmann <[email protected]>
threema-matteo pushed a commit to threema-ch/sentry-cocoa that referenced this pull request May 21, 2024
We removed SentryPrivate with getsentryGH-3623, so we also have to remove it
from the version bump.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SentryPrivate framework shared library gets removed during build
3 participants