refactor: make screenshot recorder capture() synchronous so it's harder to break accidentally with future changes #2493

vaind · 2024-12-13T13:14:44Z

📜 Description

Capturing screenshots has two critical parts:

capture the image
determine where to put mask rectangles over that image

These two must take place without any asynchronous gap.

Therefore, I'm removing async from ScreenshotRecorder.capture() so that someone doesn't accidentally shift around some code in the future and introduce an await between these two steps.

Also, I'm changing the WidgetFilter usage so that it's not reused between runs as that could cause the .items list to get overwritten if another screenshot starts capturing before a previous one has finished processing.

Additionally, this PR moves the actual handling of the replay screenshot and converting it to the image to SchedulerBinding.scheduleTask() so that it runs between frames.

For event screenshot attachments, I'm keeping leaving it as is, otherwise captureEvent() would wait indefinitely if the app never becomes idle; this may happen in tests where you have to explicitly call TestWidgetsFlutterBinding.idle() so it could actually break sentry-sdk user tests if they awaited for captureEvent() with real http backend. For us, it happens in integration tests.

Also, it adds Timeline events that help when profiling (won't do anything in release builds)

#skip-changelog

💡 Motivation and Context

💚 How did you test it?

📝 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

🔮 Next steps

…er to break accidentally with future changes

github-actions · 2024-12-13T13:15:10Z

🚨 Detected changes in high risk code 🚨

High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:

flutter/lib/src/screenshot/recorder.dart

codecov · 2024-12-13T13:15:38Z

Codecov Report

Attention: Patch coverage is 89.53488% with 9 lines in your changes missing coverage. Please review.

Project coverage is 91.59%. Comparing base (4d046e0) to head (804834f).
Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
flutter/lib/src/screenshot/recorder.dart	88.23%	8 Missing ⚠️
flutter/lib/src/replay/replay_recorder.dart	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2493      +/-   ##
==========================================
+ Coverage   86.97%   91.59%   +4.62%     
==========================================
  Files         264       88     -176     
  Lines        9388     3081    -6307     
==========================================
- Hits         8165     2822    -5343     
+ Misses       1223      259     -964

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

github-actions · 2024-12-13T13:28:40Z

Android Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	435.07 ms	524.53 ms	89.47 ms
Size	6.46 MiB	7.49 MiB	1.02 MiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`24b6e60`	440.64 ms	557.96 ms	117.32 ms
`04bd9e6`	402.06 ms	483.56 ms	81.50 ms
`0e0581f`	474.61 ms	522.96 ms	48.35 ms
`333903e`	332.76 ms	406.86 ms	74.10 ms
`3f3ef0b`	382.24 ms	459.26 ms	77.02 ms
`62de927`	313.81 ms	358.15 ms	44.34 ms
`e3ef570`	389.71 ms	459.16 ms	69.45 ms
`5d2b46d`	307.74 ms	349.85 ms	42.11 ms
`870f5eb`	329.45 ms	369.29 ms	39.84 ms
`4bcf446`	497.87 ms	533.48 ms	35.61 ms

App size

Revision	Plain	With Sentry	Diff
`24b6e60`	6.33 MiB	7.26 MiB	950.14 KiB
`04bd9e6`	6.35 MiB	7.34 MiB	1008.33 KiB
`0e0581f`	6.49 MiB	7.57 MiB	1.08 MiB
`333903e`	6.06 MiB	7.10 MiB	1.04 MiB
`3f3ef0b`	6.33 MiB	7.26 MiB	943.11 KiB
`62de927`	6.15 MiB	7.11 MiB	981.78 KiB
`e3ef570`	6.33 MiB	7.26 MiB	950.38 KiB
`5d2b46d`	6.16 MiB	7.13 MiB	1000.82 KiB
`870f5eb`	5.94 MiB	6.92 MiB	1005.77 KiB
`4bcf446`	6.49 MiB	7.56 MiB	1.07 MiB

Previous results on branch: refactor/replay-recorder-sync

Startup times

Revision	Plain	With Sentry	Diff
`2691fdb`	495.60 ms	610.88 ms	115.28 ms
`95b0818`	482.96 ms	551.55 ms	68.59 ms
`5dcf1f1`	408.10 ms	449.11 ms	41.01 ms
`7bdc48d`	454.11 ms	520.56 ms	66.45 ms
`031548e`	457.62 ms	516.70 ms	59.09 ms
`3d795ed`	480.41 ms	533.22 ms	52.81 ms
`0f86ce6`	433.36 ms	494.65 ms	61.30 ms
`147b9eb`	444.11 ms	525.33 ms	81.22 ms
`9d86790`	491.24 ms	562.70 ms	71.46 ms

App size

Revision	Plain	With Sentry	Diff
`2691fdb`	6.46 MiB	7.49 MiB	1.03 MiB
`95b0818`	6.46 MiB	7.49 MiB	1.03 MiB
`5dcf1f1`	6.46 MiB	7.49 MiB	1.03 MiB
`7bdc48d`	6.46 MiB	7.49 MiB	1.03 MiB
`031548e`	6.46 MiB	7.49 MiB	1.03 MiB
`3d795ed`	6.46 MiB	7.49 MiB	1.03 MiB
`0f86ce6`	6.46 MiB	7.49 MiB	1.03 MiB
`147b9eb`	6.46 MiB	7.49 MiB	1.02 MiB
`9d86790`	6.46 MiB	7.49 MiB	1.03 MiB

github-actions · 2024-12-13T13:35:24Z

iOS Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1254.67 ms	1267.98 ms	13.31 ms
Size	8.42 MiB	9.84 MiB	1.41 MiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`4c78360`	1230.35 ms	1252.37 ms	22.01 ms
`c57d3b7`	1249.92 ms	1280.31 ms	30.39 ms
`7359546`	1252.02 ms	1271.55 ms	19.53 ms
`3de8b9b`	1234.22 ms	1251.94 ms	17.72 ms
`1c6eb5b`	1277.85 ms	1285.71 ms	7.86 ms
`a49594a`	1284.83 ms	1313.29 ms	28.45 ms
`905bf99`	1240.84 ms	1271.47 ms	30.63 ms
`2d4fd8b`	1207.98 ms	1232.94 ms	24.96 ms
`6078ddc`	1207.84 ms	1224.10 ms	16.27 ms
`955541a`	1230.22 ms	1252.96 ms	22.73 ms

App size

Revision	Plain	With Sentry	Diff
`4c78360`	8.32 MiB	9.38 MiB	1.06 MiB
`c57d3b7`	8.32 MiB	9.52 MiB	1.20 MiB
`7359546`	8.42 MiB	9.83 MiB	1.40 MiB
`3de8b9b`	8.28 MiB	9.34 MiB	1.06 MiB
`1c6eb5b`	8.15 MiB	9.12 MiB	986.27 KiB
`a49594a`	8.16 MiB	9.16 MiB	1.00 MiB
`905bf99`	8.38 MiB	9.74 MiB	1.36 MiB
`2d4fd8b`	8.28 MiB	9.34 MiB	1.06 MiB
`6078ddc`	8.33 MiB	9.40 MiB	1.07 MiB
`955541a`	8.28 MiB	9.34 MiB	1.06 MiB

Previous results on branch: refactor/replay-recorder-sync

Startup times

Revision	Plain	With Sentry	Diff
`5dcf1f1`	1251.71 ms	1271.88 ms	20.17 ms
`7bdc48d`	1238.72 ms	1258.42 ms	19.70 ms
`031548e`	1239.59 ms	1257.32 ms	17.73 ms
`147b9eb`	1264.24 ms	1274.47 ms	10.22 ms
`0f86ce6`	1256.49 ms	1271.85 ms	15.36 ms
`95b0818`	1253.08 ms	1269.90 ms	16.82 ms
`9d86790`	1249.47 ms	1262.78 ms	13.31 ms
`2691fdb`	1251.20 ms	1268.65 ms	17.45 ms
`3d795ed`	1256.82 ms	1275.70 ms	18.89 ms

App size

Revision	Plain	With Sentry	Diff
`5dcf1f1`	8.42 MiB	9.83 MiB	1.41 MiB
`7bdc48d`	8.42 MiB	9.83 MiB	1.41 MiB
`031548e`	8.42 MiB	9.83 MiB	1.41 MiB
`147b9eb`	8.42 MiB	9.84 MiB	1.41 MiB
`0f86ce6`	8.42 MiB	9.83 MiB	1.41 MiB
`95b0818`	8.42 MiB	9.83 MiB	1.41 MiB
`9d86790`	8.42 MiB	9.83 MiB	1.41 MiB
`2691fdb`	8.42 MiB	9.83 MiB	1.41 MiB
`3d795ed`	8.42 MiB	9.83 MiB	1.41 MiB

…ryover

buenaflor

looks good, only got one question

flutter/lib/src/screenshot/recorder.dart

buenaflor

lgtm 👍

refactor: make screenshot recorder capture() synchronous so it's hard…

3d795ed

…er to break accidentally with future changes

vaind and others added 14 commits December 13, 2024 20:12

formatting

7bdc48d

enh: add timeline callbacks

9d86790

fix min_version_test

825fc89

Merge branch 'main' into refactor/replay-recorder-sync

6e9b5ec

try to fix ci

441eccc

try to fix ci

b7bc970

try to fix ci

2691fdb

Merge branch 'main' into refactor/replay-recorder-sync

031548e

fix recorder scheduling for screenshots & fix widget filter items car…

5dcf1f1

…ryover

fix late final usage

05506ed

cleanup

33987d5

fix ci

95b0818

cleanup, ci fix

0f86ce6

extract replayrecorder

147b9eb

vaind marked this pull request as ready for review December 17, 2024 07:38

vaind requested review from krystofwoldrich, stefanosiano, buenaflor and martinhaintz as code owners December 17, 2024 07:38

buenaflor reviewed Dec 17, 2024

View reviewed changes

flutter/lib/src/screenshot/recorder.dart Show resolved Hide resolved

test throwing in recorder task

804834f

buenaflor approved these changes Dec 17, 2024

View reviewed changes

vaind merged commit 0b204c0 into main Dec 18, 2024
51 checks passed

vaind deleted the refactor/replay-recorder-sync branch December 18, 2024 06:16

vaind added the Session Replay label Dec 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: make screenshot recorder capture() synchronous so it's harder to break accidentally with future changes #2493

refactor: make screenshot recorder capture() synchronous so it's harder to break accidentally with future changes #2493

vaind commented Dec 13, 2024 •

edited

Loading

github-actions bot commented Dec 13, 2024

codecov bot commented Dec 13, 2024 •

edited

Loading

github-actions bot commented Dec 13, 2024 •

edited

Loading

Baseline results on branch: main

Startup times

App size

Previous results on branch: refactor/replay-recorder-sync

Startup times

App size

github-actions bot commented Dec 13, 2024 •

edited

Loading

Baseline results on branch: main

Startup times

App size

Previous results on branch: refactor/replay-recorder-sync

Startup times

App size

buenaflor left a comment

buenaflor left a comment

refactor: make screenshot recorder capture() synchronous so it's harder to break accidentally with future changes #2493

refactor: make screenshot recorder capture() synchronous so it's harder to break accidentally with future changes #2493

Conversation

vaind commented Dec 13, 2024 • edited Loading

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Dec 13, 2024

🚨 Detected changes in high risk code 🚨

codecov bot commented Dec 13, 2024 • edited Loading

Codecov Report

github-actions bot commented Dec 13, 2024 • edited Loading

Android Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: refactor/replay-recorder-sync

Startup times

App size

github-actions bot commented Dec 13, 2024 • edited Loading

iOS Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: refactor/replay-recorder-sync

Startup times

App size

buenaflor left a comment

Choose a reason for hiding this comment

buenaflor left a comment

Choose a reason for hiding this comment

vaind commented Dec 13, 2024 •

edited

Loading

codecov bot commented Dec 13, 2024 •

edited

Loading

github-actions bot commented Dec 13, 2024 •

edited

Loading

github-actions bot commented Dec 13, 2024 •

edited

Loading