-
-
Notifications
You must be signed in to change notification settings - Fork 371
feat: enablePreWarmedAppStartTracing by default #6508
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: enablePreWarmedAppStartTracing by default #6508
Conversation
Set the default value of enablePreWarmedAppStartTracing to true. Fixes GH-3535
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6508 +/- ##
=============================================
- Coverage 86.996% 86.963% -0.034%
=============================================
Files 452 452
Lines 37691 37678 -13
Branches 17488 17478 -10
=============================================
- Hits 32790 32766 -24
- Misses 4854 4865 +11
Partials 47 47
... and 8 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
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.
Bug: Telemetry Tracking Issue for Default Features
The conditional logic for preWarmedAppStartTracing was removed from both the SDK's features and integrations lists. This prevents the feature from being reported in telemetry, even though it's now enabled by default and can still be explicitly disabled. This creates an inconsistency with how other default-enabled features are tracked.
Sources/Swift/Helper/SentryEnabledFeaturesBuilder.swift#L12-L28
sentry-cocoa/Sources/Swift/Helper/SentryEnabledFeaturesBuilder.swift
Lines 12 to 28 in 4ac4f35
| if options.enableCaptureFailedRequests { | |
| features.append("captureFailedRequests") | |
| } | |
| if options.enableTimeToFullDisplayTracing { | |
| features.append("timeToFullDisplayTracing") | |
| } | |
| if options.swiftAsyncStacktraces { | |
| features.append("swiftAsyncStacktraces") | |
| } | |
| if options.enablePersistingTracesWhenCrashing { | |
| features.append("persistingTracesWhenCrashing") | |
| } | |
Sources/Swift/Helper/SentrySdkInfo.swift#L56-L57
sentry-cocoa/Sources/Swift/Helper/SentrySdkInfo.swift
Lines 56 to 57 in 4ac4f35
| let features = SentryEnabledFeaturesBuilder.getEnabledFeatures(options: options) | |
| let integrations = SentrySDKInternal.currentHub().trimmedInstalledIntegrationNames() |
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 5c5648e | 1234.44 ms | 1253.79 ms | 19.35 ms |
| fb48c9a | 1232.49 ms | 1266.27 ms | 33.78 ms |
| d72784d | 1214.31 ms | 1241.35 ms | 27.04 ms |
| 7af87c1 | 1229.31 ms | 1253.84 ms | 24.53 ms |
| 029a804 | 1219.65 ms | 1241.96 ms | 22.30 ms |
| 139db8b | 1231.50 ms | 1258.19 ms | 26.69 ms |
| 7f4bf81 | 1241.73 ms | 1270.66 ms | 28.93 ms |
| 83bb978 | 1238.33 ms | 1260.04 ms | 21.71 ms |
| 42cfd79 | 1222.13 ms | 1244.23 ms | 22.10 ms |
| aac24ac | 1225.94 ms | 1256.38 ms | 30.45 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 5c5648e | 23.75 KiB | 879.60 KiB | 855.86 KiB |
| fb48c9a | 23.75 KiB | 1006.34 KiB | 982.59 KiB |
| d72784d | 23.75 KiB | 988.01 KiB | 964.27 KiB |
| 7af87c1 | 23.75 KiB | 933.34 KiB | 909.59 KiB |
| 029a804 | 23.75 KiB | 928.11 KiB | 904.36 KiB |
| 139db8b | 23.75 KiB | 920.64 KiB | 896.89 KiB |
| 7f4bf81 | 23.75 KiB | 919.70 KiB | 895.95 KiB |
| 83bb978 | 23.75 KiB | 920.64 KiB | 896.89 KiB |
| 42cfd79 | 23.75 KiB | 880.20 KiB | 856.45 KiB |
| aac24ac | 23.75 KiB | 1019.17 KiB | 995.42 KiB |
philprime
left a comment
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.
LGTM
📜 Description
Set the default value of enablePreWarmedAppStartTracing to true.
Docs PR getsentry/sentry-docs#15295.
💡 Motivation and Context
Fixes GH-3535
💚 How did you test it?
Unit tests.
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.