Errors shortly after SentrySDK.init don't affect the session

[philipphofmann]

Description

Capturing an error direclty after calling SentrySDK.init doesn't affect the session error count, as we start the session when didBecomeActive is called.

https://github.com/getsentry/sentry-cocoa/blob/f273eda1e1d73faa43e66fa4330e336347ab46ec/Sources/Sentry/SentrySessionTracker.m#L131-L163

We could start the session during the SDK init / when initializing the SessionIntegration.

Related to https://github.com/getsentry/sentry-java/issues/2241.

[github-actions[bot]]

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

---

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

[kevinrenskers]

We could start the session during the SDK init / when initializing the SessionIntegration.

The comment in SentrySessionTracker makes it sound like we can NOT start the session during SDK init, because we don't know if a background task or hybrid SDK initialized the SDK:

 * We can't start the session in this method because we don't know if a background task or a hybrid
 * SDK initialized the SDK. Hybrid SDKs must only post this notification if they are running in the
 * foreground because the auto session tracking logic doesn't support background tasks. Posting the
 * notification from the background would mess up the session stats.

So, if we would start the session during SDK init / when initializing the SessionIntegration, wouldn't we mess up session stats, as the comment says?

By the way, the weird thing about this comment is that it says "we can't start the session in this method".. yet it does start the session in that method. Uhm, what? Can someone explain this comment?

Also, what's this SentryHybridSdkDidBecomeActiveNotificationName all about? Don't we get the regular didBecomeActiveNotificationName notification in case of React Native or Flutter?

[philipphofmann]

So, if we would start the session during SDK init / when initializing the SessionIntegration, wouldn't we mess up session stats, as the comment says?

Yes, exactly. Sorry, I forgot about that. That's why I added extensive comments in the past.

The comment was out of sync with the code. I fixed it https://github.com/getsentry/sentry-cocoa/pull/2381.

Also, what's this SentryHybridSdkDidBecomeActiveNotificationName all about? Don't we get the regular didBecomeActiveNotificationName notification in case of React Native or Flutter?

This should answer your question. If not, I need to update the code docs. https://github.com/getsentry/sentry-cocoa/blob/9754750ca630b058a4762dfa4517c49e42290934/Sources/Sentry/include/SentryInternalNotificationNames.h#L1-L8

Approach B As the code comments indicate, we can't start the session earlier. I also wouldn't touch the session logic too much, as it's running smoothly for around two years now, and we could easily break it by starting the session earlier, even though we have good test coverage. As we are trying to fix an edge case here, I suggest doing the following. We could keep track of session error counts in the hub if there is no session yet here https://github.com/getsentry/sentry-cocoa/blob/9754750ca630b058a4762dfa4517c49e42290934/Sources/Sentry/SentryHub.m#L198-L210

If the SDK starts a session shortly after setting the session error count in the hub above, we could increment the error count for this specific session. I think it should be fine to send the session update after sending the event, but I need to double check that first.

[philipphofmann]

It's OK to send a session update after sending the event, but we shouldn't do that too often to minimize request overload.

[kevinrenskers]

Ok, that makes sense to me.

Probably best to tackle this after #2374 is merged, since it'll be touching the same code for a big part. Could I get some reviews on that PR?