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

Add main flag to threads and in_foreground flag for app contexts #2516

Merged
merged 9 commits into from
Feb 8, 2023

Conversation

marandaneto
Copy link
Contributor

@marandaneto marandaneto commented Feb 3, 2023

📜 Description

Add main flag to threads and in_foreground flag for app contexts

💡 Motivation and Context

Relates to getsentry/team-mobile#36

💚 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 3, 2023

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 47d0c2c

@marandaneto
Copy link
Contributor Author

@markushi @romtsn can I get early feedback here before going ahead?

@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 352.72 ms 356.48 ms 3.76 ms
Size 1.73 MiB 2.34 MiB 623.74 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
5fa24ec 326.29 ms 384.53 ms 58.24 ms
c1399c1 345.06 ms 385.49 ms 40.43 ms
754021c 358.70 ms 361.98 ms 3.28 ms
14c083a 350.82 ms 388.86 ms 38.04 ms
f6a135d 263.96 ms 383.59 ms 119.63 ms

App size

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

Previous results on branch: feat/main-thread-foreground-app

Startup times

Revision Plain With Sentry Diff
640c424 359.73 ms 388.98 ms 29.24 ms
029bd19 327.79 ms 378.85 ms 51.06 ms
a4ccef1 300.85 ms 321.66 ms 20.81 ms
4edebf2 312.88 ms 372.92 ms 60.04 ms
7133791 328.92 ms 360.87 ms 31.95 ms
885ac80 307.27 ms 352.98 ms 45.71 ms

App size

Revision Plain With Sentry Diff
640c424 1.73 MiB 2.33 MiB 623.16 KiB
029bd19 1.73 MiB 2.33 MiB 623.17 KiB
a4ccef1 1.73 MiB 2.33 MiB 623.16 KiB
4edebf2 1.73 MiB 2.33 MiB 623.17 KiB
7133791 1.73 MiB 2.33 MiB 623.16 KiB
885ac80 1.73 MiB 2.33 MiB 623.17 KiB

@codecov
Copy link

codecov bot commented Feb 3, 2023

Codecov Report

Base: 80.18% // Head: 80.19% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (c3e3112) compared to base (b8fb344).
Patch coverage: 93.33% of modified lines in pull request are covered.

❗ Current head c3e3112 differs from pull request most recent head 47d0c2c. Consider uploading reports for the commit 47d0c2c to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2516      +/-   ##
============================================
+ Coverage     80.18%   80.19%   +0.01%     
- Complexity     3943     3948       +5     
============================================
  Files           323      323              
  Lines         14896    14911      +15     
  Branches       1965     1967       +2     
============================================
+ Hits          11944    11958      +14     
- Misses         2178     2179       +1     
  Partials        774      774              
Impacted Files Coverage Δ
...src/main/java/io/sentry/protocol/SentryThread.java 92.39% <85.71%> (-0.55%) ⬇️
sentry/src/main/java/io/sentry/protocol/App.java 95.23% <100.00%> (+0.39%) ⬆️

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

markushi commented Feb 7, 2023

Looking good 👍 , if you need any help on the TODOs around the thread main or current flags please let me know.

@romtsn
Copy link
Member

romtsn commented Feb 7, 2023

lgtm as well

@marandaneto marandaneto changed the title Add flag for is main thread and is app in foreground Add main flag to threads and in_foreground flag for app contexts Feb 7, 2023
@marandaneto marandaneto marked this pull request as ready for review February 7, 2023 15:46
Comment on lines +259 to +260
// This should not be set by Hybrid SDKs since they have their own app's lifecycle
if (!HintUtils.isFromHybridSdk(hint) && app.getInForeground() == null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Running into #2525

Comment on lines +229 to +230
// This should not be set by Hybrid SDKs since they have their own threading model
if (!isHybridSDK && thread.isMain() == null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marandaneto marandaneto requested a review from markushi February 7, 2023 18:31
@marandaneto
Copy link
Contributor Author

@romtsn @markushi this is ready for final review.
I found that problem #2525 but it's not a blocker since this check already exists in other places.
This likely has to be addressed as well but can be a separate PR.

Copy link
Member

@markushi markushi left a comment

Choose a reason for hiding this comment

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

Nice!

@marandaneto marandaneto merged commit fe30606 into main Feb 8, 2023
@marandaneto marandaneto deleted the feat/main-thread-foreground-app branch February 8, 2023 12:01
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