search

getsentry / sentry-dart

Sentry SDK for Dart and Flutter
https://sentry.io/for/flutter/
MIT License
766 stars 239 forks source link

redact screenshots via view hierarchy #2361

Closed martinhaintz closed 1 week ago

martinhaintz commented 1 month ago

:scroll: Description

Switching to View Hierarchy for screenshot generation.

:bulb: Motivation and Context

close #1956

:green_heart: How did you test it?

Example App and Unittests

:pencil: Checklist

:crystal_ball: Next steps

github-actions[bot] commented 1 month ago
Messages
:book: Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by :no_entry_sign: dangerJS against 3fbe92450c544b887f8f52c2c7a254b8b3cbf8bc

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 89.47368% with 8 lines in your changes missing coverage. Please review.

Project coverage is 92.09%. Comparing base (25fc225) to head (3fbe924). Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
flutter/lib/src/screenshot/recorder.dart 64.70% 6 Missing :warning:
...rc/event_processor/screenshot_event_processor.dart 92.85% 1 Missing :warning:
flutter/lib/src/sentry_privacy_options.dart 96.87% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2361 +/- ## ========================================== + Coverage 86.94% 92.09% +5.14% ========================================== Files 257 83 -174 Lines 9238 2871 -6367 ========================================== - Hits 8032 2644 -5388 + Misses 1206 227 -979 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

🚨 Try these New Features:

github-actions[bot] commented 1 month ago

Android Performance metrics :rocket:

  Plain With Sentry Diff
Startup time 445.36 ms 487.04 ms 41.68 ms
Size 6.49 MiB 7.56 MiB 1.07 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
d4120ac3aa1b3297e224a93b2f9d790adb0e683b 373.14 ms 447.35 ms 74.21 ms
103eb14bb95684cd90f822c3ea85589c8650e8ef 405.50 ms 444.30 ms 38.80 ms
ad69abc68bd1bd5bc392227b3b553b2a2e38ac4f 297.35 ms 385.89 ms 88.54 ms
72dfc836ec6ac6176efb374c4c1c3e3871f4117e 298.62 ms 340.14 ms 41.52 ms
061fed2925fdc038a8b4ed8f7905e646d7c0a887 434.11 ms 506.49 ms 72.38 ms
256df44bb670822fc3087877f775a3d82bb476b0 447.58 ms 485.84 ms 38.25 ms
30c119305351b7acc7ef45c7bffd4f1d48cefc64 349.00 ms 438.20 ms 89.20 ms
408bdab123f225adb67d6213518f882ad8c3815f 360.18 ms 434.92 ms 74.74 ms
cecd4ed8dfd86177cc44e93236d7cbea68fcfeeb 438.09 ms 490.12 ms 52.04 ms
09eab47b4dc3260f0d675166619e5b2ef1e7c0a3 455.30 ms 478.46 ms 23.16 ms

App size

Revision Plain With Sentry Diff
d4120ac3aa1b3297e224a93b2f9d790adb0e683b 6.27 MiB 7.20 MiB 960.42 KiB
103eb14bb95684cd90f822c3ea85589c8650e8ef 6.52 MiB 7.59 MiB 1.06 MiB
ad69abc68bd1bd5bc392227b3b553b2a2e38ac4f 6.06 MiB 7.09 MiB 1.03 MiB
72dfc836ec6ac6176efb374c4c1c3e3871f4117e 5.94 MiB 6.92 MiB 1001.71 KiB
061fed2925fdc038a8b4ed8f7905e646d7c0a887 6.52 MiB 7.59 MiB 1.06 MiB
256df44bb670822fc3087877f775a3d82bb476b0 6.52 MiB 7.59 MiB 1.06 MiB
30c119305351b7acc7ef45c7bffd4f1d48cefc64 6.27 MiB 7.20 MiB 958.74 KiB
408bdab123f225adb67d6213518f882ad8c3815f 6.27 MiB 7.20 MiB 959.07 KiB
cecd4ed8dfd86177cc44e93236d7cbea68fcfeeb 6.49 MiB 7.57 MiB 1.08 MiB
09eab47b4dc3260f0d675166619e5b2ef1e7c0a3 6.49 MiB 7.56 MiB 1.07 MiB

Previous results on branch: feat/redact-screenshots-via-view-hierarchy

Startup times

Revision Plain With Sentry Diff
198d1a378a66b185f2f8364ef70b472825cd4170 478.64 ms 529.65 ms 51.01 ms
cc1d8361170bbc98f531106a46e9645856220245 506.06 ms 517.21 ms 11.15 ms
42be7f3c66b3db16730e7daf953592efb76d072c 351.20 ms 372.54 ms 21.34 ms
606454f73f1277f550614462279d783afbcd4a9b 473.23 ms 532.88 ms 59.65 ms
22f22b04b0a307204d8471c7140dd9d6ace78fd6 468.33 ms 511.96 ms 43.63 ms
57a0823d3c7d252a1085cece3c53afa03d981c34 485.63 ms 526.69 ms 41.06 ms
823598ac7b14830bace5f6700006663d8c497035 468.91 ms 496.46 ms 27.55 ms
820200463b52cb5a04982f136dc3f57c0ab69883 464.45 ms 512.12 ms 47.67 ms
3606202d1de69ea75ce13f3143918ef57928dd80 672.45 ms 690.24 ms 17.80 ms
73a422463642235991b4737f207b602180a83df3 510.66 ms 563.88 ms 53.22 ms

App size

Revision Plain With Sentry Diff
198d1a378a66b185f2f8364ef70b472825cd4170 6.49 MiB 7.56 MiB 1.07 MiB
cc1d8361170bbc98f531106a46e9645856220245 6.49 MiB 7.56 MiB 1.07 MiB
42be7f3c66b3db16730e7daf953592efb76d072c 6.49 MiB 7.57 MiB 1.08 MiB
606454f73f1277f550614462279d783afbcd4a9b 6.49 MiB 7.56 MiB 1.07 MiB
22f22b04b0a307204d8471c7140dd9d6ace78fd6 6.49 MiB 7.57 MiB 1.08 MiB
57a0823d3c7d252a1085cece3c53afa03d981c34 6.49 MiB 7.57 MiB 1.08 MiB
823598ac7b14830bace5f6700006663d8c497035 6.49 MiB 7.56 MiB 1.07 MiB
820200463b52cb5a04982f136dc3f57c0ab69883 6.49 MiB 7.56 MiB 1.07 MiB
3606202d1de69ea75ce13f3143918ef57928dd80 6.49 MiB 7.57 MiB 1.08 MiB
73a422463642235991b4737f207b602180a83df3 6.49 MiB 7.57 MiB 1.08 MiB
github-actions[bot] commented 1 month ago

iOS Performance metrics :rocket:

  Plain With Sentry Diff
Startup time 1241.61 ms 1252.35 ms 10.74 ms
Size 8.38 MiB 9.78 MiB 1.40 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
a49594a2366317ecfbf3078f2573c410cc51ad0b 1284.83 ms 1313.29 ms 28.45 ms
f2db4ece59961760d0017bb7b0592fc561ab79f0 1244.14 ms 1259.79 ms 15.65 ms
117d988b347861588d574fcc789ecbb4f7c8bdb5 1200.83 ms 1223.24 ms 22.41 ms
613760b51612c5874f586dd29349e387b289c8cd 1263.10 ms 1277.27 ms 14.16 ms
48adddf2bb6654c3490f6b14b745c01e3f6c95e2 1255.76 ms 1280.31 ms 24.55 ms
09c1f550c470c27444a89744002ef7ba6f65dbeb 1258.11 ms 1280.45 ms 22.34 ms
3a69405611a7c7368a27aeef1017e8cf1fa77448 1292.84 ms 1303.96 ms 11.12 ms
0a23f9840336da9128fd8f15f1a373563820df64 1252.98 ms 1276.76 ms 23.78 ms
3e3389168b45dba147dea4d5fdb53697679d3758 6245.86 ms 6260.90 ms 15.04 ms
e8603bbc68f19dd756ed1a48a23a88322a594c55 1240.85 ms 1254.79 ms 13.94 ms

App size

Revision Plain With Sentry Diff
a49594a2366317ecfbf3078f2573c410cc51ad0b 8.16 MiB 9.16 MiB 1.00 MiB
f2db4ece59961760d0017bb7b0592fc561ab79f0 8.10 MiB 9.16 MiB 1.07 MiB
117d988b347861588d574fcc789ecbb4f7c8bdb5 8.32 MiB 9.38 MiB 1.05 MiB
613760b51612c5874f586dd29349e387b289c8cd 8.15 MiB 9.13 MiB 1000.46 KiB
48adddf2bb6654c3490f6b14b745c01e3f6c95e2 8.28 MiB 9.34 MiB 1.06 MiB
09c1f550c470c27444a89744002ef7ba6f65dbeb 8.38 MiB 9.74 MiB 1.36 MiB
3a69405611a7c7368a27aeef1017e8cf1fa77448 8.15 MiB 9.15 MiB 1018.56 KiB
0a23f9840336da9128fd8f15f1a373563820df64 8.10 MiB 9.18 MiB 1.08 MiB
3e3389168b45dba147dea4d5fdb53697679d3758 8.29 MiB 9.38 MiB 1.09 MiB
e8603bbc68f19dd756ed1a48a23a88322a594c55 8.33 MiB 9.40 MiB 1.07 MiB

Previous results on branch: feat/redact-screenshots-via-view-hierarchy

Startup times

Revision Plain With Sentry Diff
981039c3839b662cc22403bca88c8258f75a7669 1231.98 ms 1243.80 ms 11.82 ms
22f22b04b0a307204d8471c7140dd9d6ace78fd6 1248.92 ms 1274.13 ms 25.21 ms
35ebb0bd63ef512afe408baf38f766a0cc800967 1230.51 ms 1247.45 ms 16.94 ms
820200463b52cb5a04982f136dc3f57c0ab69883 1244.20 ms 1249.82 ms 5.61 ms
d6212ac92fa231ac1c71a14fcfaedd10cb9b757a 1248.63 ms 1267.72 ms 19.08 ms
3606202d1de69ea75ce13f3143918ef57928dd80 1257.33 ms 1284.26 ms 26.93 ms
823598ac7b14830bace5f6700006663d8c497035 1251.18 ms 1275.98 ms 24.80 ms
198d1a378a66b185f2f8364ef70b472825cd4170 1244.67 ms 1259.96 ms 15.28 ms
57a0823d3c7d252a1085cece3c53afa03d981c34 1239.41 ms 1267.28 ms 27.87 ms
73a422463642235991b4737f207b602180a83df3 1240.18 ms 1266.15 ms 25.96 ms

App size

Revision Plain With Sentry Diff
981039c3839b662cc22403bca88c8258f75a7669 8.38 MiB 9.75 MiB 1.37 MiB
22f22b04b0a307204d8471c7140dd9d6ace78fd6 8.38 MiB 9.75 MiB 1.37 MiB
35ebb0bd63ef512afe408baf38f766a0cc800967 8.38 MiB 9.75 MiB 1.37 MiB
820200463b52cb5a04982f136dc3f57c0ab69883 8.38 MiB 9.77 MiB 1.39 MiB
d6212ac92fa231ac1c71a14fcfaedd10cb9b757a 8.38 MiB 9.77 MiB 1.39 MiB
3606202d1de69ea75ce13f3143918ef57928dd80 8.38 MiB 9.75 MiB 1.37 MiB
823598ac7b14830bace5f6700006663d8c497035 8.38 MiB 9.77 MiB 1.39 MiB
198d1a378a66b185f2f8364ef70b472825cd4170 8.38 MiB 9.77 MiB 1.39 MiB
57a0823d3c7d252a1085cece3c53afa03d981c34 8.38 MiB 9.75 MiB 1.37 MiB
73a422463642235991b4737f207b602180a83df3 8.38 MiB 9.75 MiB 1.37 MiB
martinhaintz commented 1 month ago

@vaind @buenaflor I moved the redacting features from the SR(Screen Replay) config to the SS(ScreenShot) config and derived from it. I also copied the options from SR for the SS functionality.

vaind commented 1 month ago

Why is the SR feature experimental? Should the redacting settings for SS also stay experimental?

There still may be changes to replay as well as the redaction logic, therefore I'd say it should be marked experimental for screenshot redaction too. The main purpose is that it lets users know they should use this feature with care as it may have some issues. Also it may change without a major release.

martinhaintz commented 2 weeks ago

@vaind could you please have a look at the tests clears replay ID from context and captures images? thanks.

martinhaintz commented 2 weeks ago

@vaind how, is the change of the device resolution handled in Session Replay? Lets say we have a mac desktop application and it is started in portrait mode (w:600, h: 1200), then the recorder config at startup is set to this target resolution. If the user resizes the windows to landscape (w:1200, h: 600), then recorder config for the target size is still in portrait mode, although everything in the window is rendered for landscape.

vaind commented 2 weeks ago

@vaind how, is the change of the device resolution handled in Session Replay?

Currently, it's an open issue. I've checked how this behaves and what needs to be done and I'll make a PR to address this

Lets say we have a mac desktop application

Just FYI that doesn't support session replay at the moment, but the platform doesn't matter for the argument you're making. Just wanted to clarify for anyone else reading this.

and it is started in portrait mode (w:600, h: 1200), then the recorder config at startup is set to this target resolution. If the user resizes the windows to landscape (w:1200, h: 600), then recorder config for the target size is still in portrait mode, although everything in the window is rendered for landscape.

For screenshots themselves (i.e. this PR) that shouldn't matter because we're setting a "constraint", not really dimensions (SentryScreenshotQuality.targetResolution() returns a single value and we use that as the target either for width or for height).

For replays, this needs to change and we'll need to respect changes in resolution in the PR-to-be I've mentioned above. Basically, we'll need to stop, reconfigure & restart replay when a resolution changes.

martinhaintz commented 2 weeks ago

For replays, this needs to change and we'll need to respect changes in resolution in the PR-to-be I've mentioned above. Basically, we'll need to stop, reconfigure & restart replay when a resolution changes.

I understand this behavior, but as a user it would be nice, to have a video and see, that the resizing of the video probably resulted in a crash.

vaind commented 2 weeks ago

I understand this behavior, but as a user it would be nice, to have a video and see, that the resizing of the video probably resulted in a crash.

I believe it's still a single replay, just a different segment

martinhaintz commented 2 weeks ago

@buenaflor @vaind In screenshot_event_processor.dart the beforeScreenshot callback is executed first and afterwards it will check if the screenshot functionality is available for this renderer. Is this a bug or is this the expected behaviour?

IMHO I would assume, that the beforeScreenshotis not called, if the screenshot feature itself is not available for the platform. Therefore I would move the platform check before the callback handling.

    final beforeScreenshot = _options.screenshot.beforeCapture;
    if (beforeScreenshot != null) {
      try {
        final result = beforeScreenshot(event, hint: hint);
        bool takeScreenshot;
        if (result is Future<bool>) {
          takeScreenshot = await result;
        } else {
          takeScreenshot = result;
        }
        if (!takeScreenshot) {
          return event;
        }
      } catch (exception, stackTrace) {
        _options.logger(
          SentryLevel.error,
          'The beforeScreenshot callback threw an exception',
          exception: exception,
          stackTrace: stackTrace,
        );
        if (_options.automatedTestMode) {
          rethrow;
        }
      }
    }

    final renderer = _options.rendererWrapper.getRenderer();

    if (_options.platformChecker.isWeb &&
        renderer != FlutterRenderer.canvasKit) {
      _options.logger(
        SentryLevel.debug,
        'Cannot take screenshot with ${renderer?.name} renderer.',
      );
      return event;
    }
github-actions[bot] commented 2 weeks ago

🚨 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:

martinhaintz commented 2 weeks ago

@vaind I reverted the changes regarding the html renderer, went through all files changed in this PR and tested it on the android emulator on Pixel 7 Pro API 35 for quality low, high and full, with and without redaction and the screenshot size and the redaction looks good.

Let me know, if I can support you in testing.

martinhaintz commented 1 week ago

I will now start to update the docs, based on the changes of this PR.

vaind commented 1 week ago

I will now start to update the docs, based on the changes of this PR.

We should merge prior to docs changes as these should normally come live only when a release is being made