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 invalid session creation when app is launched in background #2543

Merged
merged 5 commits into from
Feb 15, 2023

Conversation

markushi
Copy link
Member

@markushi markushi commented Feb 14, 2023

📜 Description

Skip session creation if the app is not started in foreground.

💡 Motivation and Context

This is a follow-up based on some customer feedback, specifically seeing session creations spikes for scheduled background jobs (e.g. a nightly sync).

Likely fixes #2512

💚 How did you test it?

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

@github-actions
Copy link
Contributor

github-actions bot commented Feb 14, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 352.32 ms 364.70 ms 12.38 ms
Size 1.73 MiB 2.34 MiB 624.03 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
4b32504 315.69 ms 373.96 ms 58.27 ms
754021c 358.70 ms 361.98 ms 3.28 ms
c1399c1 345.06 ms 385.49 ms 40.43 ms
5fa24ec 326.29 ms 384.53 ms 58.24 ms
fe30606 310.82 ms 335.36 ms 24.55 ms
f6a135d 263.96 ms 383.59 ms 119.63 ms
14c083a 350.82 ms 388.86 ms 38.04 ms
4b32504 357.14 ms 404.04 ms 46.90 ms
fe30606 327.46 ms 351.74 ms 24.28 ms

App size

Revision Plain With Sentry Diff
4b32504 1.73 MiB 2.34 MiB 623.74 KiB
754021c 1.73 MiB 2.33 MiB 623.06 KiB
c1399c1 1.73 MiB 2.33 MiB 620.61 KiB
5fa24ec 1.73 MiB 2.33 MiB 620.61 KiB
fe30606 1.73 MiB 2.34 MiB 623.74 KiB
f6a135d 1.73 MiB 2.33 MiB 623.10 KiB
14c083a 1.73 MiB 2.33 MiB 620.61 KiB
4b32504 1.73 MiB 2.34 MiB 623.74 KiB
fe30606 1.73 MiB 2.34 MiB 623.74 KiB

Previous results on branch: fix/skewed-session-metrics

Startup times

Revision Plain With Sentry Diff
c40dc3b 284.90 ms 343.31 ms 58.41 ms
20762f9 313.35 ms 367.08 ms 53.74 ms

App size

Revision Plain With Sentry Diff
c40dc3b 1.73 MiB 2.34 MiB 624.03 KiB
20762f9 1.73 MiB 2.34 MiB 623.73 KiB

@codecov
Copy link

codecov bot commented Feb 14, 2023

Codecov Report

Base: 80.20% // Head: 80.20% // No change to project coverage 👍

Coverage data is based on head (f322413) compared to base (1045a47).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #2543   +/-   ##
=========================================
  Coverage     80.20%   80.20%           
  Complexity     3960     3960           
=========================================
  Files           324      324           
  Lines         14937    14937           
  Branches       1968     1968           
=========================================
  Hits          11980    11980           
  Misses         2183     2183           
  Partials        774      774           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@markushi
Copy link
Member Author

@romtsn as an alternative we could keep the old logic (always create a session) and attach a foreground flag once the first Activity is launched ~ session.setIsForeground(true|false). When SentryClient.captureSession() is called, we could then drop non-foreground sessions. This way we don't have to check for foreground importance which might causes issues in third-party app stores (see #2537)

@romtsn
Copy link
Member

romtsn commented Feb 15, 2023

@romtsn as an alternative we could keep the old logic (always create a session) and attach a foreground flag once the first Activity is launched ~ session.setIsForeground(true|false). When SentryClient.captureSession() is called, we could then drop non-foreground sessions. This way we don't have to check for foreground importance which might causes issues in third-party app stores (see #2537)

I think that makes sense, would you mind changing it then?

@@ -479,6 +479,13 @@ public void captureSession(final @NotNull Session session, final @Nullable Hint
return;
}

if (session.isAppInForeground() != null && !session.isAppInForeground()) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe replace this with
if (!Boolean.TRUE.equals(session.isAppInForeground()))?

@romtsn
Copy link
Member

romtsn commented Feb 15, 2023

:shipit:

@markushi markushi merged commit 3524661 into main Feb 15, 2023
@markushi markushi deleted the fix/skewed-session-metrics branch February 15, 2023 13:00
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.

3 participants