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

added FrameMetrics to Android profiling data #2342

Merged
merged 27 commits into from
Nov 18, 2022

Conversation

stefanosiano
Copy link
Member

📜 Description

added ProfileMeasurements to AndroidTransactionProfiler and ProfilingTraceData
added frameMetrics (slow and frozen frames) and screen refresh rate to profiling measurements
added a SentryFrameMetricsCollector to collect frameMetrics
changed a benchmark device on Saucelabs as it's not available anymore

💡 Motivation and Context

We want to add data to profiles. So we're starting reporting all slow and frozen frames
Oneplus 7 pro on Saucelabs is not available anymore, and i replaced it with Oneplus 7T

💚 How did you test it?

Added Unit and Ui tests

📝 Checklist

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

🔮 Next steps

…TraceData

added frameMetrics (slow and frozen frames) and screen refresh rate
added a SentryFrameMetricsCollector to collect frameMetrics
changed a benchmark device on Saucelabs as it's not available anymore
@github-actions
Copy link
Contributor

github-actions bot commented Nov 7, 2022

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

Generated by 🚫 dangerJS against bf05cd0

…TraceData

added frameMetrics (slow and frozen frames) and screen refresh rate
added a SentryFrameMetricsCollector to collect frameMetrics
changed a benchmark device on Saucelabs as it's not available anymore
@github-actions
Copy link
Contributor

github-actions bot commented Nov 7, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 342.91 ms 377.74 ms 34.83 ms
Size 1.73 MiB 2.33 MiB 612.76 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
f809aac 301.51 ms 346.60 ms 45.09 ms
a04f788 321.78 ms 354.12 ms 32.35 ms
38e4f11 358.20 ms 433.73 ms 75.53 ms
507f924 342.51 ms 402.65 ms 60.14 ms
90e9745 314.68 ms 357.28 ms 42.60 ms
7597ded 289.60 ms 339.69 ms 50.09 ms

App size

Revision Plain With Sentry Diff
f809aac 1.73 MiB 2.32 MiB 608.63 KiB
a04f788 1.73 MiB 2.32 MiB 609.88 KiB
38e4f11 1.73 MiB 2.32 MiB 609.82 KiB
507f924 1.73 MiB 2.32 MiB 609.95 KiB
90e9745 1.73 MiB 2.32 MiB 608.63 KiB
7597ded 1.73 MiB 2.32 MiB 609.88 KiB

Previous results on branch: feat/profiling-frame-metrics

Startup times

Revision Plain With Sentry Diff
43860fd 297.76 ms 330.25 ms 32.49 ms
9385630 313.85 ms 319.06 ms 5.21 ms
4d7bda6 338.96 ms 461.37 ms 122.41 ms
a6b6b98 370.26 ms 459.67 ms 89.41 ms
b1b2eb0 315.10 ms 349.81 ms 34.71 ms
5711c14 298.11 ms 323.40 ms 25.30 ms
e7e2fb1 317.90 ms 353.63 ms 35.73 ms
5b4c2f3 319.28 ms 356.79 ms 37.52 ms
922e325 360.50 ms 405.19 ms 44.69 ms

App size

Revision Plain With Sentry Diff
43860fd 1.73 MiB 2.33 MiB 612.62 KiB
9385630 1.73 MiB 2.32 MiB 612.50 KiB
4d7bda6 1.73 MiB 2.33 MiB 612.74 KiB
a6b6b98 1.73 MiB 2.32 MiB 612.52 KiB
b1b2eb0 1.73 MiB 2.32 MiB 612.50 KiB
5711c14 1.73 MiB 2.32 MiB 612.50 KiB
e7e2fb1 1.73 MiB 2.32 MiB 612.50 KiB
5b4c2f3 1.73 MiB 2.32 MiB 612.52 KiB
922e325 1.73 MiB 2.32 MiB 612.55 KiB

@codecov-commenter
Copy link

codecov-commenter commented Nov 7, 2022

Codecov Report

Base: 80.33% // Head: 80.28% // Decreases project coverage by -0.04% ⚠️

Coverage data is based on head (bf05cd0) compared to base (84f0ef1).
Patch coverage: 60.50% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2342      +/-   ##
============================================
- Coverage     80.33%   80.28%   -0.05%     
- Complexity     3673     3690      +17     
============================================
  Files           290      292       +2     
  Lines         13729    13845     +116     
  Branches       1810     1833      +23     
============================================
+ Hits          11029    11116      +87     
- Misses         2003     2014      +11     
- Partials        697      715      +18     
Impacted Files Coverage Δ
...sentry/profilemeasurements/ProfileMeasurement.java 52.72% <52.72%> (ø)
...y/profilemeasurements/ProfileMeasurementValue.java 58.33% <58.33%> (ø)
...ry/src/main/java/io/sentry/ProfilingTraceData.java 78.36% <92.30%> (+3.04%) ⬆️
sentry/src/main/java/io/sentry/JsonSerializer.java 97.97% <100.00%> (+0.06%) ⬆️
...ntry/src/main/java/io/sentry/JsonObjectReader.java 82.50% <0.00%> (+2.50%) ⬆️
.../main/java/io/sentry/ProfilingTransactionData.java 52.52% <0.00%> (+10.10%) ⬆️

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.

…TraceData

added frameMetrics (slow and frozen frames) and screen refresh rate
added a SentryFrameMetricsCollector to collect frameMetrics
changed a benchmark device on Saucelabs as it's not available anymore
…TraceData

added frameMetrics (slow and frozen frames) and screen refresh rate
added a SentryFrameMetricsCollector to collect frameMetrics
changed a benchmark device on Saucelabs as it's not available anymore
@stefanosiano stefanosiano marked this pull request as ready for review November 10, 2022 08:42
added automatic activity transaction in ui test
// Most frames take just a few nanoseconds longer than the optimal calculated
// duration.
// Therefore we subtract one, because otherwise almost all frames would be slow.
boolean isSlow = durationNanos > nanosInSecond / (refreshRate - 1);
Copy link
Member

Choose a reason for hiding this comment

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

I like this approach, we should probably also switch over in the ActivityFramesTracker, because for now we're just using hardcoded 16ms for detecting slow frames

Copy link
Member

Choose a reason for hiding this comment

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

Not in this PR of course :)

* removed useless file exist check in AndroidTransactionProfiler
* wrapped AndroidTransactionProfiler methods in executor service
* fixed crash in sample app's ProfilingActivity
* updated ui tests
…tyStarted(), not onActivityCreated() anymore
frameMetricsListener removal wrapped in a try catch block
@stefanosiano stefanosiano merged commit ed00ecb into main Nov 18, 2022
@stefanosiano stefanosiano deleted the feat/profiling-frame-metrics branch November 18, 2022 09:58
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