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

Clear window reference only on activity stop in profileMeasurements collector #2407

Merged
merged 7 commits into from Dec 5, 2022

Conversation

stefanosiano
Copy link
Member

📜 Description

the current window reference in SentryFrameMetricsCollector is cleared only after the activity stops, and no more when the profiler stops

💡 Motivation and Context

We need the reference to the current window to attach or detach frameMetrics listeners. Currently, when the profiler stops we clear this reference. This means that if the profiler is restarted in the same activity, we cannot attach the frameMetrics listener because we don't have the reference to the window.
So we are having profiles without measurements, especially when auto instrumentation starts (and stops) profiler when an activity is started

💚 How did you test it?

Added a unit test

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

🔮 Next steps

…d only after the activity stops, and no more when the profiler stops
@github-actions
Copy link
Contributor

github-actions bot commented Dec 1, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 302.46 ms 331.20 ms 28.74 ms
Size 1.73 MiB 2.32 MiB 612.36 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
31f3e4c 325.22 ms 342.77 ms 17.55 ms
81a1a6c 328.73 ms 421.28 ms 92.55 ms
7597ded 289.60 ms 339.69 ms 50.09 ms
507f924 342.51 ms 402.65 ms 60.14 ms
38e4f11 358.20 ms 433.73 ms 75.53 ms
3695453 301.78 ms 371.14 ms 69.36 ms
81a1a6c 294.04 ms 341.19 ms 47.15 ms
4a9c176 319.77 ms 363.20 ms 43.43 ms
16371c5 314.02 ms 394.54 ms 80.52 ms
f809aac 301.51 ms 346.60 ms 45.09 ms

App size

Revision Plain With Sentry Diff
31f3e4c 1.73 MiB 2.32 MiB 612.47 KiB
81a1a6c 1.73 MiB 2.32 MiB 612.47 KiB
7597ded 1.73 MiB 2.32 MiB 609.88 KiB
507f924 1.73 MiB 2.32 MiB 609.95 KiB
38e4f11 1.73 MiB 2.32 MiB 609.82 KiB
3695453 1.73 MiB 2.32 MiB 611.62 KiB
81a1a6c 1.73 MiB 2.32 MiB 612.47 KiB
4a9c176 1.73 MiB 2.33 MiB 612.69 KiB
16371c5 1.73 MiB 2.32 MiB 611.62 KiB
f809aac 1.73 MiB 2.32 MiB 608.63 KiB

Previous results on branch: fix/profiling-metrics-missing

Startup times

Revision Plain With Sentry Diff
c7f9e2c 307.46 ms 321.12 ms 13.66 ms
2998e46 284.45 ms 318.54 ms 34.09 ms
ea5fc7c 281.34 ms 343.08 ms 61.74 ms

App size

Revision Plain With Sentry Diff
c7f9e2c 1.73 MiB 2.32 MiB 612.36 KiB
2998e46 1.73 MiB 2.32 MiB 612.45 KiB
ea5fc7c 1.73 MiB 2.32 MiB 612.36 KiB

@codecov-commenter
Copy link

codecov-commenter commented Dec 1, 2022

Codecov Report

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

Coverage data is based on head (65555a2) compared to base (f122116).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #2407   +/-   ##
=========================================
  Coverage     80.03%   80.03%           
  Complexity     3765     3765           
=========================================
  Files           301      301           
  Lines         14207    14207           
  Branches       1884     1884           
=========================================
  Hits          11371    11371           
  Misses         2092     2092           
  Partials        744      744           

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.

@stefanosiano stefanosiano marked this pull request as ready for review December 2, 2022 01:13
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.

LGTM, please have a look at my comment

@stefanosiano stefanosiano merged commit 1e1ab7f into main Dec 5, 2022
@stefanosiano stefanosiano deleted the fix/profiling-metrics-missing branch December 5, 2022 17:14
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.

None yet

3 participants