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

Feat: Add flag to disable debug file upload #4223

Merged
merged 6 commits into from
Nov 4, 2024
Merged

Conversation

lucas-zimerman
Copy link
Collaborator

@lucas-zimerman lucas-zimerman commented Oct 31, 2024

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

A new flag that can be used to disable the upload of native debug files.

💡 Motivation and Context

Users want to disable native debug file upload without altering the flag SENTRY_DISABLE_AUTO_UPLOAD, this introduces a new flag called SENTRY_DISABLE_XCODE_DEBUG_UPLOAD for disabling the native debug files upload on Xcode.

💚 How did you test it?

running the script locally with the flag

📝 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

Close #4056

Copy link
Contributor

github-actions bot commented Oct 31, 2024

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 30e9377

@lucas-zimerman lucas-zimerman marked this pull request as ready for review October 31, 2024 12:49
Copy link
Contributor

github-actions bot commented Oct 31, 2024

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 379.65 ms 440.11 ms 60.46 ms
Size 7.15 MiB 8.35 MiB 1.20 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
f06c879+dirty 361.27 ms 407.88 ms 46.61 ms
d43a46b+dirty 417.65 ms 472.98 ms 55.33 ms
5446992+dirty 371.61 ms 390.00 ms 18.39 ms
fe13591+dirty 539.51 ms 597.92 ms 58.40 ms
5a22220+dirty 384.61 ms 419.06 ms 34.45 ms
2534337+dirty 597.14 ms 665.04 ms 67.90 ms
e73d82f+dirty 377.67 ms 407.06 ms 29.39 ms
dadc233+dirty 363.19 ms 370.37 ms 7.18 ms
0677344+dirty 288.40 ms 391.44 ms 103.04 ms
70caa60+dirty 308.83 ms 393.06 ms 84.23 ms

App size

Revision Plain With Sentry Diff
f06c879+dirty 7.15 MiB 8.12 MiB 997.78 KiB
d43a46b+dirty 7.15 MiB 8.34 MiB 1.19 MiB
5446992+dirty 7.15 MiB 8.12 MiB 999.45 KiB
fe13591+dirty 7.15 MiB 8.35 MiB 1.20 MiB
5a22220+dirty 7.15 MiB 8.21 MiB 1.06 MiB
2534337+dirty 7.15 MiB 8.11 MiB 988.68 KiB
e73d82f+dirty 7.15 MiB 8.34 MiB 1.19 MiB
dadc233+dirty 7.15 MiB 8.04 MiB 910.84 KiB
0677344+dirty 7.15 MiB 8.07 MiB 949.80 KiB
70caa60+dirty 7.15 MiB 8.03 MiB 901.79 KiB

Copy link
Contributor

github-actions bot commented Oct 31, 2024

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1212.73 ms 1225.55 ms 12.82 ms
Size 2.36 MiB 3.08 MiB 736.79 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
c2a4e9b+dirty 1240.10 ms 1239.22 ms -0.88 ms
5571a20+dirty 1203.57 ms 1204.57 ms 1.00 ms
e2b64fe+dirty 1232.22 ms 1255.20 ms 22.98 ms
31fcca2+dirty 1209.17 ms 1216.21 ms 7.04 ms
c639edf+dirty 1236.18 ms 1235.04 ms -1.14 ms
0d3e677+dirty 1214.39 ms 1225.70 ms 11.31 ms
9433f35+dirty 1246.94 ms 1271.45 ms 24.52 ms
b95b8af+dirty 1221.39 ms 1228.52 ms 7.13 ms
2ec71da+dirty 1225.85 ms 1231.57 ms 5.72 ms
62a750b+dirty 1216.60 ms 1229.14 ms 12.54 ms

App size

Revision Plain With Sentry Diff
c2a4e9b+dirty 2.36 MiB 3.08 MiB 734.00 KiB
5571a20+dirty 2.36 MiB 2.92 MiB 569.93 KiB
e2b64fe+dirty 2.36 MiB 2.85 MiB 495.80 KiB
31fcca2+dirty 2.36 MiB 2.90 MiB 552.95 KiB
c639edf+dirty 2.36 MiB 3.08 MiB 736.63 KiB
0d3e677+dirty 2.36 MiB 3.10 MiB 753.12 KiB
9433f35+dirty 2.36 MiB 2.85 MiB 499.80 KiB
b95b8af+dirty 2.36 MiB 3.14 MiB 793.32 KiB
2ec71da+dirty 2.36 MiB 3.13 MiB 784.66 KiB
62a750b+dirty 2.36 MiB 2.92 MiB 570.00 KiB

Copy link
Contributor

github-actions bot commented Oct 31, 2024

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1239.75 ms 1241.00 ms 1.25 ms
Size 2.92 MiB 3.64 MiB 742.56 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
c2a4e9b+dirty 1247.39 ms 1243.04 ms -4.35 ms
5571a20+dirty 1228.09 ms 1233.45 ms 5.36 ms
e2b64fe+dirty 1285.78 ms 1297.56 ms 11.78 ms
31fcca2+dirty 1222.04 ms 1226.51 ms 4.47 ms
c639edf+dirty 1223.63 ms 1227.98 ms 4.35 ms
0d3e677+dirty 1239.02 ms 1241.22 ms 2.20 ms
9433f35+dirty 1232.24 ms 1232.74 ms 0.50 ms
b95b8af+dirty 1235.60 ms 1242.06 ms 6.46 ms
2ec71da+dirty 1230.29 ms 1239.50 ms 9.21 ms
62a750b+dirty 1228.12 ms 1230.53 ms 2.41 ms

App size

Revision Plain With Sentry Diff
c2a4e9b+dirty 2.92 MiB 3.64 MiB 739.91 KiB
5571a20+dirty 2.92 MiB 3.48 MiB 575.54 KiB
e2b64fe+dirty 2.92 MiB 3.41 MiB 499.97 KiB
31fcca2+dirty 2.92 MiB 3.46 MiB 557.31 KiB
c639edf+dirty 2.92 MiB 3.64 MiB 742.55 KiB
0d3e677+dirty 2.92 MiB 3.66 MiB 758.42 KiB
9433f35+dirty 2.92 MiB 3.41 MiB 503.55 KiB
b95b8af+dirty 2.92 MiB 3.69 MiB 794.16 KiB
2ec71da+dirty 2.92 MiB 3.69 MiB 791.06 KiB
62a750b+dirty 2.92 MiB 3.48 MiB 575.59 KiB

@krystofwoldrich
Copy link
Member

The flag looks good, I'm just curious, what is the use case? When a dev would like to upload source maps but no native files?

Would it make sense to have it also for Android, as the generic SENTRY_DISABLE_AUTO_UPLOAD works for both iOS and Android.

@lucas-zimerman
Copy link
Collaborator Author

The flag looks good, I'm just curious, what is the use case? When a dev would like to upload source maps but no native files?

I can see some specific use cases only:

  • Focusing only on JavaScript source maps.
  • Saving storage on self-hosted servers (and again focusing on JS source maps)
  • Speed up build process (despite being fast) for internal production ready builds.

The case of #4056 was very specific, and I don't other use cases other than the linked issue have.

@lucas-zimerman lucas-zimerman changed the title Feat: Add xcode flag to disable debug file upload Feat: Add flag to disable debug file upload Oct 31, 2024
Copy link
Collaborator

@antonis antonis left a comment

Choose a reason for hiding this comment

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

The implementation LGTM 🚀

@lucas-zimerman lucas-zimerman enabled auto-merge (squash) November 4, 2024 12:31
@lucas-zimerman lucas-zimerman merged commit 373cc8e into main Nov 4, 2024
38 of 39 checks passed
@lucas-zimerman lucas-zimerman deleted the feat/xcode-flag branch November 4, 2024 12:33
@@ -4,7 +4,8 @@ import java.util.regex.Matcher
import java.util.regex.Pattern

project.ext.shouldSentryAutoUpload = { ->
return System.getenv('SENTRY_DISABLE_AUTO_UPLOAD') != 'true'
return System.getenv('SENTRY_DISABLE_AUTO_UPLOAD') != 'true' ||
System.getenv('SENTRY_DISABLE_NATIVE_DEBUG_UPLOAD') != 'true'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey @lucas-zimerman 👋
While checking some a CI checks issue on a dependandabot PR I revisited the Android logic here. I think for the new flag to work as on iOS we should change || with &&. Wdyt?

project.ext.shouldSentryAutoUpload = { ->
  return System.getenv('SENTRY_DISABLE_AUTO_UPLOAD') != 'true' &&
         System.getenv('SENTRY_DISABLE_NATIVE_DEBUG_UPLOAD') != 'true'
}

or

project.ext.shouldSentryAutoUpload = { ->
  return !(System.getenv('SENTRY_DISABLE_AUTO_UPLOAD') == 'true' ||
         System.getenv('SENTRY_DISABLE_NATIVE_DEBUG_UPLOAD') == 'true')
}

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.

Request to have a specific flag to disable upload debug symbol build phase
3 participants