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

Do not send session updates for terminated sessions #2849

Merged
merged 9 commits into from
Jul 20, 2023

Conversation

romtsn
Copy link
Member

@romtsn romtsn commented Jul 19, 2023

📜 Description

If the session is in terminal state (!= Status.Ok), we should not send any new session updates, because there's no protection for that server-side, so potentially we were reporting incorrect crash-free rate. Follow-up of #2845

💡 Motivation and Context

Internal customer report

💚 How did you test it?

Manually and automated

📝 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 Jul 19, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 326.00 ms 370.40 ms 44.40 ms
Size 1.72 MiB 2.29 MiB 575.37 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
adf8fe3 300.49 ms 357.36 ms 56.87 ms
f60279b 324.60 ms 345.33 ms 20.73 ms
dc67004 273.86 ms 346.37 ms 72.51 ms
d0c08e7 310.28 ms 350.68 ms 40.40 ms
c03a05e 390.40 ms 419.35 ms 28.94 ms
f274c79 313.96 ms 355.96 ms 42.00 ms
f274c79 334.86 ms 348.54 ms 13.68 ms
caf50ec 302.36 ms 325.25 ms 22.89 ms
9246ed4 281.79 ms 352.08 ms 70.29 ms
0310da5 381.20 ms 404.50 ms 23.30 ms

App size

Revision Plain With Sentry Diff
adf8fe3 1.72 MiB 2.29 MiB 575.24 KiB
f60279b 1.72 MiB 2.29 MiB 575.23 KiB
dc67004 1.72 MiB 2.28 MiB 573.45 KiB
d0c08e7 1.72 MiB 2.29 MiB 574.68 KiB
c03a05e 1.72 MiB 2.29 MiB 574.43 KiB
f274c79 1.72 MiB 2.29 MiB 575.01 KiB
f274c79 1.72 MiB 2.29 MiB 575.01 KiB
caf50ec 1.72 MiB 2.29 MiB 575.24 KiB
9246ed4 1.72 MiB 2.28 MiB 572.22 KiB
0310da5 1.72 MiB 2.28 MiB 573.45 KiB

Previous results on branch: rz/fix/session-terminal-status

Startup times

Revision Plain With Sentry Diff
1407f1a 322.63 ms 336.54 ms 13.91 ms
4d57fed 257.48 ms 316.08 ms 58.60 ms
69b3074 279.96 ms 347.34 ms 67.38 ms

App size

Revision Plain With Sentry Diff
1407f1a 1.72 MiB 2.29 MiB 574.76 KiB
4d57fed 1.72 MiB 2.29 MiB 575.37 KiB
69b3074 1.72 MiB 2.29 MiB 574.76 KiB

@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (0bff5c1) 81.37% compared to head (7579062) 81.37%.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #2849   +/-   ##
=========================================
  Coverage     81.37%   81.37%           
- Complexity     4635     4639    +4     
=========================================
  Files           354      354           
  Lines         17064    17066    +2     
  Branches       2307     2308    +1     
=========================================
+ Hits          13885    13888    +3     
+ Misses         2229     2228    -1     
  Partials        950      950           
Impacted Files Coverage Δ
sentry/src/main/java/io/sentry/SentryClient.java 88.24% <100.00%> (+0.25%) ⬆️
sentry/src/main/java/io/sentry/Session.java 94.41% <100.00%> (+0.02%) ⬆️

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

@romtsn romtsn merged commit 101f707 into main Jul 20, 2023
@romtsn romtsn deleted the rz/fix/session-terminal-status branch July 20, 2023 15:43
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.

4 participants