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 debounce to capturing screenshots #2368

Merged
merged 33 commits into from
Nov 26, 2024
Merged

Add debounce to capturing screenshots #2368

merged 33 commits into from
Nov 26, 2024

Conversation

denrase
Copy link
Collaborator

@denrase denrase commented Oct 22, 2024

📜 Description

Debounce screenshots made by sentry to avoid being removed by iOS watchdog.

Bildschirmfoto 2024-10-22 um 11 34 36

💡 Motivation and Context

Relates to #2360
Relates to #2368

💚 How did you test it?

Unit tests.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPii is enabled
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

Copy link
Contributor

github-actions bot commented Oct 22, 2024

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

Generated by 🚫 dangerJS against 4d5f535

Copy link

codecov bot commented Oct 22, 2024

Codecov Report

Attention: Patch coverage is 91.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 91.81%. Comparing base (7f97e6c) to head (4d5f535).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
flutter/lib/src/utils/timer_debouncer.dart 66.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2368      +/-   ##
==========================================
+ Coverage   86.92%   91.81%   +4.89%     
==========================================
  Files         259       84     -175     
  Lines        9245     2897    -6348     
==========================================
- Hits         8036     2660    -5376     
+ Misses       1209      237     -972     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

github-actions bot commented Oct 22, 2024

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1243.80 ms 1274.37 ms 30.57 ms
Size 8.38 MiB 9.78 MiB 1.40 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
6f3717a 1259.84 ms 1280.90 ms 21.06 ms
e6b16cd 1226.20 ms 1246.52 ms 20.32 ms
8fa3934 1252.79 ms 1272.41 ms 19.62 ms
cc80714 1205.53 ms 1223.90 ms 18.37 ms
0a82a1e 1233.56 ms 1244.56 ms 11.00 ms
3334ac1 1259.22 ms 1275.40 ms 16.17 ms
256df44 1252.49 ms 1274.27 ms 21.78 ms
cf91c9d 1217.08 ms 1233.00 ms 15.92 ms
b728df4 1287.43 ms 1293.94 ms 6.51 ms
61e71ec 1243.14 ms 1257.21 ms 14.07 ms

App size

Revision Plain With Sentry Diff
6f3717a 8.33 MiB 9.62 MiB 1.29 MiB
e6b16cd 8.33 MiB 9.54 MiB 1.22 MiB
8fa3934 8.09 MiB 9.07 MiB 1000.86 KiB
cc80714 8.33 MiB 9.40 MiB 1.07 MiB
0a82a1e 8.29 MiB 9.37 MiB 1.08 MiB
3334ac1 8.10 MiB 9.17 MiB 1.08 MiB
256df44 8.38 MiB 9.71 MiB 1.33 MiB
cf91c9d 8.33 MiB 9.40 MiB 1.07 MiB
b728df4 8.15 MiB 9.15 MiB 1020.72 KiB
61e71ec 8.33 MiB 9.40 MiB 1.07 MiB

Previous results on branch: feat/screenshot-debounce

Startup times

Revision Plain With Sentry Diff
b157a35 1256.12 ms 1273.90 ms 17.78 ms
e5f628a 1227.47 ms 1229.73 ms 2.26 ms
3d489d9 1256.27 ms 1279.69 ms 23.42 ms
9fce5c0 1226.77 ms 1248.82 ms 22.05 ms
e96bb8d 1248.14 ms 1270.88 ms 22.73 ms
5a9bcac 1258.37 ms 1282.79 ms 24.42 ms

App size

Revision Plain With Sentry Diff
b157a35 8.38 MiB 9.77 MiB 1.39 MiB
e5f628a 8.38 MiB 9.75 MiB 1.37 MiB
3d489d9 8.38 MiB 9.75 MiB 1.37 MiB
9fce5c0 8.38 MiB 9.77 MiB 1.39 MiB
e96bb8d 8.38 MiB 9.77 MiB 1.39 MiB
5a9bcac 8.38 MiB 9.77 MiB 1.39 MiB

Copy link
Contributor

github-actions bot commented Oct 22, 2024

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 469.60 ms 521.49 ms 51.89 ms
Size 6.49 MiB 7.56 MiB 1.07 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
1ac008b 370.04 ms 435.58 ms 65.54 ms
4ad2751 374.58 ms 462.00 ms 87.42 ms
43760f9 321.78 ms 366.77 ms 44.99 ms
fec92cc 399.96 ms 479.26 ms 79.30 ms
c9d3212 301.34 ms 361.58 ms 60.24 ms
5603ab2 309.84 ms 345.20 ms 35.36 ms
3e9fb0e 329.14 ms 359.16 ms 30.02 ms
6d317ea 303.46 ms 356.06 ms 52.60 ms
1a93825 347.31 ms 424.54 ms 77.23 ms
bffc2c5 348.00 ms 399.89 ms 51.89 ms

App size

Revision Plain With Sentry Diff
1ac008b 6.33 MiB 7.27 MiB 954.13 KiB
4ad2751 6.16 MiB 7.14 MiB 1010.86 KiB
43760f9 6.15 MiB 7.13 MiB 1000.49 KiB
fec92cc 6.35 MiB 7.33 MiB 1005.56 KiB
c9d3212 6.16 MiB 7.14 MiB 1010.90 KiB
5603ab2 5.94 MiB 6.95 MiB 1.01 MiB
3e9fb0e 5.94 MiB 6.95 MiB 1.01 MiB
6d317ea 5.94 MiB 6.92 MiB 1001.74 KiB
1a93825 6.27 MiB 7.20 MiB 956.36 KiB
bffc2c5 6.34 MiB 7.28 MiB 966.31 KiB

Previous results on branch: feat/screenshot-debounce

Startup times

Revision Plain With Sentry Diff
e96bb8d 451.02 ms 488.94 ms 37.92 ms
9fce5c0 458.88 ms 484.86 ms 25.98 ms
5a9bcac 441.24 ms 476.58 ms 35.34 ms
b157a35 668.50 ms 743.42 ms 74.92 ms
e5f628a 451.42 ms 495.36 ms 43.94 ms
3d489d9 452.25 ms 498.58 ms 46.33 ms

App size

Revision Plain With Sentry Diff
e96bb8d 6.49 MiB 7.56 MiB 1.07 MiB
9fce5c0 6.49 MiB 7.56 MiB 1.07 MiB
5a9bcac 6.49 MiB 7.56 MiB 1.07 MiB
b157a35 6.49 MiB 7.56 MiB 1.07 MiB
e5f628a 6.49 MiB 7.57 MiB 1.08 MiB
3d489d9 6.49 MiB 7.57 MiB 1.08 MiB

@denrase denrase marked this pull request as ready for review October 22, 2024 11:49
@denrase
Copy link
Collaborator Author

denrase commented Oct 23, 2024

Closing due to discussion here: #2371 (comment)

@denrase denrase closed this Oct 23, 2024
@denrase denrase reopened this Nov 12, 2024
@denrase denrase self-assigned this Nov 12, 2024
@denrase denrase marked this pull request as draft November 12, 2024 08:58
@denrase denrase removed their assignment Nov 12, 2024
@denrase
Copy link
Collaborator Author

denrase commented Nov 13, 2024

@buenaflor Looks like we have some provisioning profile issue.

@buenaflor
Copy link
Contributor

buenaflor commented Nov 14, 2024

@denrase on it

edit: ci is fixed

@denrase denrase requested a review from buenaflor November 18, 2024 09:25
@denrase denrase requested a review from buenaflor November 18, 2024 17:00
@buenaflor
Copy link
Contributor

before capture callback looks like is added to the redact screenshot pr: https://github.com/getsentry/sentry-dart/pull/2361/files, _options.screenshot.beforeCapture although I'm not sure whether we should keep our approach with beforeCaptureScreenshot, might be easier to manage?

@denrase
Copy link
Collaborator Author

denrase commented Nov 19, 2024

@buenaflor Lets wait then when the other PR is merged. It still uses the BeforeScreenshotCallback, which we should change to BeforeCaptureCallback then. Do other SDKs also nest options?

@buenaflor
Copy link
Contributor

buenaflor commented Nov 19, 2024

I'm not sure if we should nest the before screenshot options tbh, probably would not be consistent with other sdks and the before capture view hierarchy

but I don't know if we're going to nest for every sdk

@buenaflor
Copy link
Contributor

buenaflor commented Nov 25, 2024

screenshot redaction is merged, we can continue here, they changed it to root options

@denrase denrase requested a review from buenaflor November 25, 2024 15:51
@denrase
Copy link
Collaborator Author

denrase commented Nov 25, 2024

@buenaflor rdy for one more review round

CHANGELOG.md Outdated Show resolved Hide resolved
@denrase denrase changed the title Add debounce to ScreenshotWidget Add debounce to capturing screenshots Nov 25, 2024
@denrase denrase requested a review from buenaflor November 25, 2024 16:34
Copy link
Contributor

@buenaflor buenaflor left a comment

Choose a reason for hiding this comment

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

🚀

@denrase denrase merged commit be08651 into main Nov 26, 2024
48 checks passed
@denrase denrase deleted the feat/screenshot-debounce branch November 26, 2024 10:03
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