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.

Sign up for GitHub

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

Jump to bottom

Fix: Aligns Android autoupload logic with iOS #4262

Merged
merged 2 commits into from
Nov 12, 2024
Merged

Fix: Aligns Android autoupload logic with iOS #4262

merged 2 commits into from
Nov 12, 2024

Conversation

antonis
Copy link
Collaborator

@antonis antonis commented Nov 12, 2024 •
edited
Loading

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Aligns Android SENTRY_DISABLE_NATIVE_DEBUG_UPLOAD logic with the SENTRY_DISABLE_XCODE_DEBUG_UPLOAD on iOS, so that the upload is performed only when both the new flag is false and the SENTRY_DISABLE_AUTO_UPLOAD is false

SENTRY_DISABLE_ AUTO_UPLOAD SENTRY_DISABLE_ NATIVE_DEBUG_UPLOAD XCODE_DEBUG_UPLOAD Upload iOS Upload Android (before) Upload Android (after)
true true false false false
true false false true false
false true false true false
false false true true true

#4263 fixes case when the native upload is disabled but still JS source maps should be uploaded.

💡 Motivation and Context

See #4258 (comment)

💚 How did you test it?

CI

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled
  • All tests passing
  • No breaking changes

🔮 Next steps

#skip-changelog

@antonis antonis marked this pull request as ready for review November 12, 2024 09:01
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 12, 2024 •
edited
Loading

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 468.48 ms 450.55 ms -17.93 ms
Size 17.74 MiB 20.08 MiB 2.34 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
e73f4ed+dirty 332.96 ms 354.33 ms 21.37 ms
70e6261 482.65 ms 495.70 ms 13.05 ms
1d86dd6 405.14 ms 411.06 ms 5.92 ms
3853f43 329.68 ms 346.32 ms 16.64 ms
e5bc97b 438.96 ms 437.39 ms -1.57 ms
4cc5c27 460.04 ms 496.32 ms 36.28 ms
12427f4 393.69 ms 414.84 ms 21.14 ms
5446992 403.40 ms 426.70 ms 23.30 ms
cdf2f33 469.46 ms 462.17 ms -7.29 ms
0d3e677 422.82 ms 411.90 ms -10.92 ms

App size

Revision Plain With Sentry Diff
e73f4ed+dirty 17.73 MiB 20.04 MiB 2.31 MiB
70e6261 17.73 MiB 19.94 MiB 2.21 MiB
1d86dd6 17.73 MiB 19.86 MiB 2.12 MiB
3853f43 17.73 MiB 19.81 MiB 2.08 MiB
e5bc97b 17.74 MiB 20.08 MiB 2.34 MiB
4cc5c27 17.73 MiB 19.95 MiB 2.21 MiB
12427f4 17.73 MiB 19.85 MiB 2.12 MiB
5446992 17.73 MiB 19.85 MiB 2.12 MiB
cdf2f33 17.74 MiB 20.08 MiB 2.34 MiB
0d3e677 17.74 MiB 20.07 MiB 2.34 MiB

Previous results on branch: antonis/android-skip-upload

Startup times

Revision Plain With Sentry Diff
c484309 433.41 ms 432.16 ms -1.25 ms

App size

Revision Plain With Sentry Diff
c484309 17.74 MiB 20.08 MiB 2.34 MiB

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 12, 2024 •
edited
Loading

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 490.57 ms 570.20 ms 79.62 ms
Size 7.15 MiB 8.35 MiB 1.20 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
e73f4ed+dirty 262.98 ms 311.02 ms 48.04 ms
52c0562+dirty 401.23 ms 435.65 ms 34.42 ms
1d86dd6+dirty 335.76 ms 371.22 ms 35.46 ms
70e6261+dirty 395.08 ms 408.12 ms 13.04 ms
575f9da+dirty 337.15 ms 370.47 ms 33.32 ms
d2c32bb+dirty 445.45 ms 497.85 ms 52.41 ms
0677344+dirty 288.40 ms 391.44 ms 103.04 ms
6e8584e+dirty 383.37 ms 400.84 ms 17.47 ms
e5bc97b+dirty 409.10 ms 471.61 ms 62.51 ms
8ae23a7+dirty 398.10 ms 411.48 ms 13.38 ms

App size

Revision Plain With Sentry Diff
e73f4ed+dirty 7.15 MiB 8.09 MiB 965.94 KiB
52c0562+dirty 7.15 MiB 8.39 MiB 1.24 MiB
1d86dd6+dirty 7.15 MiB 8.13 MiB 1002.18 KiB
70e6261+dirty 7.15 MiB 8.21 MiB 1.07 MiB
575f9da+dirty 7.15 MiB 8.10 MiB 979.68 KiB
d2c32bb+dirty 7.15 MiB 8.35 MiB 1.20 MiB
0677344+dirty 7.15 MiB 8.07 MiB 949.80 KiB
6e8584e+dirty 7.15 MiB 8.13 MiB 1002.18 KiB
e5bc97b+dirty 7.15 MiB 8.35 MiB 1.20 MiB
8ae23a7+dirty 7.15 MiB 8.35 MiB 1.20 MiB

Previous results on branch: antonis/android-skip-upload

Startup times

Revision Plain With Sentry Diff
c484309+dirty 373.04 ms 401.94 ms 28.90 ms

App size

Revision Plain With Sentry Diff
c484309+dirty 7.15 MiB 8.35 MiB 1.20 MiB

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 12, 2024 •
edited
Loading

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1242.15 ms 1227.43 ms -14.71 ms
Size 2.92 MiB 3.66 MiB 757.22 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
76d1baf+dirty 1245.00 ms 1257.76 ms 12.76 ms
575f9da+dirty 1272.00 ms 1284.38 ms 12.38 ms
15c80ab+dirty 1248.41 ms 1251.24 ms 2.83 ms
4a6664f+dirty 1218.77 ms 1221.07 ms 2.30 ms
0db0c72+dirty 1258.88 ms 1262.52 ms 3.64 ms
b1e8712+dirty 1284.11 ms 1297.82 ms 13.71 ms
d197b5c+dirty 1234.80 ms 1249.20 ms 14.40 ms
c2a4e9b+dirty 1247.39 ms 1243.04 ms -4.35 ms
86d6d2c+dirty 1291.62 ms 1296.80 ms 5.18 ms
ac41368+dirty 1226.69 ms 1229.96 ms 3.27 ms

App size

Revision Plain With Sentry Diff
76d1baf+dirty 2.92 MiB 3.38 MiB 475.74 KiB
575f9da+dirty 2.92 MiB 3.43 MiB 524.26 KiB
15c80ab+dirty 2.92 MiB 3.39 MiB 481.56 KiB
4a6664f+dirty 2.92 MiB 3.60 MiB 702.09 KiB
0db0c72+dirty 2.92 MiB 3.40 MiB 492.71 KiB
b1e8712+dirty 2.92 MiB 3.40 MiB 494.15 KiB
d197b5c+dirty 2.92 MiB 3.37 MiB 464.41 KiB
c2a4e9b+dirty 2.92 MiB 3.64 MiB 739.91 KiB
86d6d2c+dirty 2.92 MiB 3.37 MiB 464.31 KiB
ac41368+dirty 2.92 MiB 3.69 MiB 794.29 KiB

Previous results on branch: antonis/android-skip-upload

Startup times

Revision Plain With Sentry Diff
c484309+dirty 1230.29 ms 1225.92 ms -4.37 ms

App size

Revision Plain With Sentry Diff
c484309+dirty 2.92 MiB 3.66 MiB 757.18 KiB

@krystofwoldrich
Copy link
Member

krystofwoldrich commented Nov 12, 2024 •
edited
Loading

Thank you @antonis, this make sense, the flag should behave exactly the same if possible.

I've noticed one more case where there was a difference between iOS and Android.

If you agree I would supercede this PR with #4263

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 12, 2024 •
edited
Loading

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1231.00 ms 1236.89 ms 5.89 ms
Size 2.36 MiB 3.10 MiB 752.58 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
76d1baf+dirty 1244.10 ms 1268.52 ms 24.42 ms
575f9da+dirty 1266.22 ms 1274.84 ms 8.62 ms
15c80ab+dirty 1223.74 ms 1228.96 ms 5.22 ms
4a6664f+dirty 1209.49 ms 1208.63 ms -0.86 ms
0db0c72+dirty 1275.02 ms 1285.84 ms 10.82 ms
b1e8712+dirty 1256.02 ms 1265.14 ms 9.12 ms
d197b5c+dirty 1217.61 ms 1242.66 ms 25.05 ms
c2a4e9b+dirty 1240.10 ms 1239.22 ms -0.88 ms
86d6d2c+dirty 1267.55 ms 1286.21 ms 18.66 ms
ac41368+dirty 1226.65 ms 1237.90 ms 11.24 ms

App size

Revision Plain With Sentry Diff
76d1baf+dirty 2.36 MiB 2.82 MiB 469.45 KiB
575f9da+dirty 2.36 MiB 2.87 MiB 520.20 KiB
15c80ab+dirty 2.36 MiB 2.83 MiB 474.49 KiB
4a6664f+dirty 2.36 MiB 3.04 MiB 696.39 KiB
0db0c72+dirty 2.36 MiB 2.84 MiB 487.01 KiB
b1e8712+dirty 2.36 MiB 2.84 MiB 488.84 KiB
d197b5c+dirty 2.36 MiB 2.82 MiB 462.86 KiB
c2a4e9b+dirty 2.36 MiB 3.08 MiB 734.00 KiB
86d6d2c+dirty 2.36 MiB 2.82 MiB 462.82 KiB
ac41368+dirty 2.36 MiB 3.14 MiB 793.46 KiB

Previous results on branch: antonis/android-skip-upload

Startup times

Revision Plain With Sentry Diff
c484309+dirty 1207.47 ms 1209.54 ms 2.07 ms

App size

Revision Plain With Sentry Diff
c484309+dirty 2.36 MiB 3.10 MiB 752.62 KiB

@antonis
Copy link
Collaborator Author

antonis commented Nov 12, 2024 •
edited
Loading

If you agree I would supercede this PR with #4263

Sounds good @krystofwoldrich 👍 I've merged your changes so that they target main and updated the PR description.

krystofwoldrich
krystofwoldrich approved these changes Nov 12, 2024
View reviewed changes
Copy link
Member

@krystofwoldrich krystofwoldrich left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 Good catch. Thank you.

lucas-zimerman
lucas-zimerman approved these changes Nov 12, 2024
View reviewed changes
Copy link
Collaborator

@lucas-zimerman lucas-zimerman left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@antonis antonis merged commit 63ed251 into main Nov 12, 2024
63 checks passed
@antonis antonis deleted the antonis/android-skip-upload branch November 12, 2024 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krystofwoldrich krystofwoldrich krystofwoldrich approved these changes

@lucas-zimerman lucas-zimerman lucas-zimerman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@antonis @krystofwoldrich @lucas-zimerman