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

Always cold start if OOM tracking is disabled #2376

Closed
WZBbiao opened this issue Nov 11, 2022 · 15 comments · Fixed by #2382
Closed

Always cold start if OOM tracking is disabled #2376

WZBbiao opened this issue Nov 11, 2022 · 15 comments · Fixed by #2382

Comments

@WZBbiao
Copy link

WZBbiao commented Nov 11, 2022

Platform

iOS

Installed

CocoaPods

Version

7.29.0

Steps to Reproduce

  1. disable OOM Tracking
    SentrySDK.start { options in
    options.dsn = "https://xxx.sentry.io/140"
    options.enableOutOfMemoryTracking = false
    }

  2. test app start

Expected Result

In the case of non-first-time installation, several consecutive app start should be warm start.

Actual Result

Always cold start.

@philipphofmann
Copy link
Member

From our docs

Cold start: App launched for the first time, after a reboot or update. The app is not in memory and no process exists.
Warm start: App launched at least once, is partially in memory, and no process exists.

several consecutive boots should be cold start.

What do you mean by consecutive boots? It's only a cold start if App launched for the first time, after a reboot or update. The app is not in memory, and no process exists. If you mean reboot, then several consecutive reboots are always cold start as if you reboot your device, it's a cold start.

Sorry, I don't understand your problem, @WZBbiao.

@WZBbiao
Copy link
Author

WZBbiao commented Nov 11, 2022

Sorry, I mean not reboot, but repeatedly killing/opening the app.

@brustolin
Copy link
Contributor

Sorry, I mean not reboot, but repeatedly killing/opening the app.

If by killing you mean closing the process in the App Switcher, then yes, every start will be a cold start.

@WZBbiao
Copy link
Author

WZBbiao commented Nov 14, 2022

If by killing you mean closing the process in the App Switcher, then yes, every start will be a cold start.

Cold start: App launched for the first time, after a reboot or update. The app is not in memory and no process exists.
Warm start: App launched at least once, is partially in memory, and no process exists.

However, by definition this is more like a cold start.
Are there any other discussions on this issue?

@brustolin
Copy link
Contributor

However, by definition this is more like a cold start.

Sorry @WZBbiao I dont get it? Can you describe the steps you are doing that you believe should not be a cold start?

@WZBbiao
Copy link
Author

WZBbiao commented Nov 14, 2022

  1. Download the app
  2. Start the app (this startup is marked as a cold startup, no problem)
  3. Kill the app from the App Switcher
  4. Launch the app again, This startup is still marked as a cold boot, which does not meet the cold start definition

Cold start: App launched for the first time, after a reboot or update. The app is not in memory and no process exists.

@brustolin
Copy link
Contributor

Once you remove the app from the App Switcher, you kill the process, and the app is no longer in memory.
As mentioned in the last part of the description, this is a cold start.

@WZBbiao
Copy link
Author

WZBbiao commented Nov 14, 2022

Ok, but according to my test, when other conditions remain unchanged, repeat launch demo app, when enableOutOfMemoryTracking = true, the start type is warm start, and when enableOutOfMemoryTracking = false, the start type always is cold start.
I put demo project in attachment.
image
image
SentryColdStart.zip

@brustolin
Copy link
Contributor

@WZBbiao, thank you for bringing this up, and thank you for the sample project.
We may have a problem with wrongfully reporting warm start, and not cold.
@kevinrenskers will look into it.

@kevinrenskers
Copy link
Contributor

@philipphofmann @brustolin both of you seem the think that killing and restarting an app should be a cold start, but according to the code, it isn't: https://github.com/getsentry/sentry-cocoa/blob/master/Sources/Sentry/SentryAppStartTracker.m#L230-L261. It only uses the system's boot time, which doesn't change if you kill and restart the app. You can kill and restart the app all you like but the system boot time stays the same, and as such it's always reported as a warm start.

Then there is the separate issue that this behaves differently when OOM tracking is disabled. This is a problem that should have been solved with #2276, I am now looking into this.

@WZBbiao WZBbiao closed this as completed Nov 14, 2022
@WZBbiao WZBbiao reopened this Nov 14, 2022
@kevinrenskers
Copy link
Contributor

kevinrenskers commented Nov 14, 2022

OK, found the cause, but not sure why it behaves the way it does.

  • - [SentryAppStartTracker didBecomeVisible] is called
  • Which calls - [SentryAppStartTracker buildAppStartMeasurement]
  • Which calls - [SentryAppStartTracker stop]
  • Which calls [self.appStateManager stop]

Now, because there is nothing else using the app state manager (OOM integration is not installed), this causes [self.fileManager deleteAppState] to be called, which removes the current app state from disk. So the next time you start the app, there is no app state to copy to previous app state, and as such it is always seen as a cold start (because self.previousAppState == nil).

When you do have OOM installed, that stop method doesn't end up deleting the app state and everything works as expected.

I don't think deleteAppState should ever be called. Why was that code there, what is the reason to delete app state?

@kevinrenskers
Copy link
Contributor

PR to fix the app state getting deleted: #2382.

But the logic regarding warm/cold start is unchanged, it's all based app system boot time.

@kevinrenskers
Copy link
Contributor

Question for @philipphofmann: why does - [SentryAppStartTracker buildAppStartMeasurement] even call - [SentryAppStartTracker stop]? Doesn't really make sense to me? This means that as soon as didBecomeVisible is called, we unsubscribe from UIApplicationDidFinishLaunchingNotification, UIWindowDidBecomeVisibleNotification and UIApplicationDidEnterBackgroundNotification.

That seems very weird to me. I think we need to remove the call to [self stop] within - [SentryAppStartTracker buildAppStartMeasurement].

@philipphofmann
Copy link
Member

why does - [SentryAppStartTracker buildAppStartMeasurement] even call - [SentryAppStartTracker stop]?

Because we only want to build an appStartMeasurement once during the lifetime of the SDK.

I think we need to remove the call to [self stop] within - [SentryAppStartTracker buildAppStartMeasurement]

But that's how it is currently.

@philipphofmann
Copy link
Member

Sorry about the confusion. We want to stop the AppStartTracker but not the AppStateManager.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants