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

[Statsig] Don't log extra background events #5134

Merged
merged 1 commit into from
Sep 4, 2024
Merged

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Sep 4, 2024

Fixes excessive logging of state:background:sampled with secondsActive = 0 on iOS.

The problem is that on iOS, app state goes active -> inactive -> background. As a result, the else branch of this code gets triggered both on inactive and background. It "correctly" counts the time only once (i.e. when going active -> inactive) and reports the other change as secondsActive = 0 (because we weren't active in the first place). But it would be better not to send the event at all. With this change, only actual changes to and from active state emit the event.

Test Plan

--- a/src/lib/statsig/statsig.tsx
+++ b/src/lib/statsig/statsig.tsx
@@ -222,11 +222,13 @@ AppState.addEventListener('change', (state: AppStateStatus) => {
   if (state === 'active') {
     lastActive = performance.now()
     logEvent('state:foreground:sampled', {})
+    console.log('log foreground')
   } else {
     let secondsActive = 0
     if (lastActive != null) {
       secondsActive = Math.round((performance.now() - lastActive) / 1e3)
       lastActive = null
+      console.log('log background', secondsActive)
       logEvent('state:background:sampled', {
         secondsActive,
       })

On iOS, verify logs show up when you switch away from the app and back to it. Verify there is no extra "log background 0" log which would've happened before this change. Verify pulling the notification center and back also gets counted as backgrounding and foregrounding.

On Android, verify logs show up when you switch away from the app and back to it.

On web, it doesn't feel like this setup works reliably at all — not sure if it broke at some point. Concretely, the listener only seems to fire as the tab is about to be closed, but it doesn't seem triggered on tab switch. That's unrelated to this PR though.

Copy link

render bot commented Sep 4, 2024

Copy link

github-actions bot commented Sep 4, 2024

Old size New size Diff
7.09 MB 7.09 MB 0 B (0.00%)

Copy link
Member

@estrattonbailey estrattonbailey left a comment

Choose a reason for hiding this comment

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

Makes sense to me, code looks good, didn't test but can if you'd like a 2nd look

@gaearon gaearon merged commit 8860890 into main Sep 4, 2024
6 checks passed
@gaearon gaearon deleted the dont-log-empty-events branch September 4, 2024 13:41
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.

2 participants