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

fix: Do not delete the app state #2382

Merged
merged 5 commits into from
Nov 14, 2022

Conversation

kevinrenskers
Copy link
Contributor

@kevinrenskers kevinrenskers commented Nov 14, 2022

📜 Description

The app state is needed on the next app start, to be copied to previous app state. This is needed to determine the app start type. We should never delete the app state.

💡 Motivation and Context

Closes #2376

💚 How did you test it?

Unit tests

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • Review from the native team if needed
  • No breaking changes

🔮 Next steps

The app state is needed on the next app start, to be copied to previous app state. This is needed to determine the app start type.

Closes 2376
@kevinrenskers kevinrenskers marked this pull request as ready for review November 14, 2022 12:45
@github-actions
Copy link

github-actions bot commented Nov 14, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1240.10 ms 1273.43 ms 33.33 ms
Size 20.75 KiB 374.15 KiB 353.40 KiB

Baseline results on branch: master

Startup times

Revision Plain With Sentry Diff
5025d2e 1248.52 ms 1251.72 ms 3.20 ms
0fdf0b2 1245.88 ms 1247.69 ms 1.82 ms
b15627c 1256.48 ms 1264.68 ms 8.20 ms
604586a 1216.06 ms 1249.10 ms 33.04 ms
db5f62a 1234.47 ms 1257.80 ms 23.33 ms
4e037c4 1205.00 ms 1227.58 ms 22.58 ms
8607e67 1255.80 ms 1259.50 ms 3.70 ms
46deabf 1217.73 ms 1247.30 ms 29.57 ms
0032a5d 1267.35 ms 1282.34 ms 14.99 ms
725723e 1228.47 ms 1255.09 ms 26.62 ms

App size

Revision Plain With Sentry Diff
5025d2e 20.51 KiB 331.79 KiB 311.28 KiB
0fdf0b2 20.51 KiB 332.90 KiB 312.39 KiB
b15627c 20.50 KiB 337.76 KiB 317.25 KiB
604586a 20.51 KiB 333.15 KiB 312.65 KiB
db5f62a 20.51 KiB 333.16 KiB 312.65 KiB
4e037c4 20.50 KiB 361.80 KiB 341.29 KiB
8607e67 20.50 KiB 338.99 KiB 318.49 KiB
46deabf 20.75 KiB 374.16 KiB 353.41 KiB
0032a5d 20.75 KiB 369.27 KiB 348.52 KiB
725723e 20.75 KiB 367.21 KiB 346.46 KiB

Previous results on branch: fix/2376-do-not-delete-app-state

Startup times

Revision Plain With Sentry Diff
07f37e5 1228.33 ms 1259.07 ms 30.74 ms
bba6cd3 1243.64 ms 1258.44 ms 14.80 ms
6f9637d 1265.12 ms 1281.86 ms 16.73 ms
b6d2fb0 1201.00 ms 1236.82 ms 35.82 ms

App size

Revision Plain With Sentry Diff
07f37e5 20.75 KiB 374.15 KiB 353.39 KiB
bba6cd3 20.75 KiB 374.19 KiB 353.44 KiB
6f9637d 20.75 KiB 373.64 KiB 352.89 KiB
b6d2fb0 20.75 KiB 373.64 KiB 352.89 KiB

@kevinrenskers
Copy link
Contributor Author

Please also see my question here about removing the call to [self stop] within - [SentryAppStartTracker buildAppStartMeasurement].

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.

Please add an integration test to reproduce the issue in #2376 and prove that is fixed now.

CHANGELOG.md Outdated
@@ -10,6 +10,7 @@
### Fixes

- Too long flush duration (#2370)
- Do not delete the app state when OOM tracking is disabled (#2382)
Copy link
Member

Choose a reason for hiding this comment

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

m: Please change this, so we describe what we are fixing instead of how. I think we fixed a bug that we always report cold starts when OOM is enabled.

@@ -271,15 +286,15 @@ class SentryAppStartTrackerTests: NotificationCenterTestCase {
assertValidHybridStart(type: .warm)
}

private func givenPreviousAppState(appState: SentryAppState) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As this is not the previous but the current app state, this name really didn't make sense.

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

@kevinrenskers kevinrenskers merged commit 2a547de into master Nov 14, 2022
@kevinrenskers kevinrenskers deleted the fix/2376-do-not-delete-app-state branch November 14, 2022 14:03
@github-actions
Copy link

Fails
🚫 Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Do not delete the app state ([#2382](https://github.com/getsentry/sentry-cocoa/pull/2382))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 1b40492

@kevinrenskers
Copy link
Contributor Author

Why is it so often complaining about a missing changelog when it's definitely there.

kevinrenskers added a commit that referenced this pull request Nov 15, 2022
…ents

* master:
  build(deps): bump github/codeql-action from 2.1.31 to 2.1.32 (#2386)
  build(deps): bump fastlane from 2.210.1 to 2.211.0 (#2385)
  ref: json serialization error reporting (#2355)
  release: 7.31.0
  ref: Fix outdated comment in SessionTracker (#2381)
  fix: Do not delete the app state (#2382)
  build: Split Swift and Clang format for pre-commit (#2380)
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.

Always cold start if OOM tracking is disabled
2 participants