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 app start span end-time is wrong if SDK init is deferred #2519

Merged
merged 10 commits into from
Feb 8, 2023

Conversation

markushi
Copy link
Member

@markushi markushi commented Feb 6, 2023

This way the appstart start/end times are properly tracked, even if the
SDK is not initialized yet. In case of a deferred SDK init, the app
start span will be attached to the first tracked Activity after SDK
init.

💡 Motivation and Context

Fixes #2518

💚 How did you test it?

Unit tests

📝 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

This way the appstart start/end times are properly tracked, even if the
SDK is not initialized yet. In case of a deferred SDK init, the app
start span will be attached to the first tracked Activity after SDK
init.
@markushi markushi changed the title Move AppStartState write operations to SentryPerformanceProvider Fix app start span end is wrong if SDK init is deferred Feb 6, 2023
@markushi markushi changed the title Fix app start span end is wrong if SDK init is deferred Fix app start span end-time is wrong if SDK init is deferred Feb 6, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 352.10 ms 385.02 ms 32.92 ms
Size 1.73 MiB 2.33 MiB 623.52 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: fix/app-start-time-deferred-sdk-init

Startup times

Revision Plain With Sentry Diff
8a9e6d9 320.86 ms 362.48 ms 41.62 ms
15e2361 321.98 ms 390.90 ms 68.92 ms
301557d 310.90 ms 362.35 ms 51.46 ms
0a8bf63 313.18 ms 355.65 ms 42.47 ms
099f7ad 361.06 ms 364.21 ms 3.15 ms

App size

Revision Plain With Sentry Diff
8a9e6d9 1.73 MiB 2.33 MiB 623.40 KiB
15e2361 1.73 MiB 2.33 MiB 623.40 KiB
301557d 1.73 MiB 2.33 MiB 623.40 KiB
0a8bf63 1.73 MiB 2.33 MiB 623.39 KiB
099f7ad 1.73 MiB 2.33 MiB 623.40 KiB

@codecov
Copy link

codecov bot commented Feb 6, 2023

Codecov Report

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

Coverage data is based on head (5b10d84) compared to base (754021c).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #2519   +/-   ##
=========================================
  Coverage     80.18%   80.18%           
  Complexity     3943     3943           
=========================================
  Files           323      323           
  Lines         14896    14896           
  Branches       1965     1965           
=========================================
  Hits          11944    11944           
  Misses         2178     2178           
  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.

Copy link
Member

@romtsn romtsn left a comment

Choose a reason for hiding this comment

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

lgtm 👍 a couple of minor comments

@romtsn
Copy link
Member

romtsn commented Feb 6, 2023

@marandaneto would you like to take a look at this? Was there a reason why we have not capture the app start span end time in the same provider where we capture the start time?

@marandaneto
Copy link
Contributor

@marandaneto would you like to take a look at this? Was there a reason why we have not capture the app start span end time in the same provider where we capture the start time?

Most likely because we offer the possibility of no content providers (docs tell how to remove them).
Also because Hybrid SDKs finish the app start by themselves (checking their own screen visibility).
The current approach might be a problem for the later, it has to be checked.

@markushi markushi merged commit b8fb344 into main Feb 8, 2023
@markushi markushi deleted the fix/app-start-time-deferred-sdk-init branch February 8, 2023 07:11
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.

App Start spans are not properly generated if SDK init is deferred
3 participants