redact screenshots via view hierarchy #2361

martinhaintz · 2024-10-15T14:46:23Z

📜 Description

Switching to View Hierarchy for screenshot generation.

💡 Motivation and Context

close #1956

💚 How did you test it?

Example App and Unittests

📝 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

Update the Docs

…nd ScreenshotRecorderConfig.

github-actions · 2024-10-15T14:46:53Z

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

Generated by 🚫 dangerJS against 57a0823

codecov · 2024-10-15T14:47:43Z

Codecov Report

Attention: Patch coverage is 82.41758% with 16 lines in your changes missing coverage. Please review.

Project coverage is 84.93%. Comparing base (0479083) to head (57a0823).

Files with missing lines	Patch %	Lines
flutter/lib/src/sentry_flutter_options.dart	33.33%	10 Missing ⚠️
flutter/lib/src/sentry_flutter.dart	55.55%	4 Missing ⚠️
...rc/event_processor/screenshot_event_processor.dart	92.85%	1 Missing ⚠️
flutter/lib/src/sentry_privacy_options.dart	96.87%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2361      +/-   ##
==========================================
+ Coverage   84.78%   84.93%   +0.15%     
==========================================
  Files         253       81     -172     
  Lines        9070     2808    -6262     
==========================================
- Hits         7690     2385    -5305     
+ Misses       1380      423     -957

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

flutter/lib/src/sentry_replay_options.dart

github-actions · 2024-10-16T14:21:31Z

Android Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	485.63 ms	526.69 ms	41.06 ms
Size	6.49 MiB	7.57 MiB	1.08 MiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`e2d89fc`	323.84 ms	376.23 ms	52.39 ms
`be173fa`	375.94 ms	458.36 ms	82.42 ms
`7b2e0ad`	415.59 ms	457.26 ms	41.67 ms
`322aa66`	284.98 ms	341.76 ms	56.78 ms
`eecbbca`	324.37 ms	352.49 ms	28.12 ms
`f3a18f2`	456.29 ms	504.41 ms	48.12 ms
`e0ba81f`	390.38 ms	465.40 ms	75.02 ms
`33527b4`	403.58 ms	507.44 ms	103.86 ms
`3334ac1`	303.98 ms	366.65 ms	62.67 ms
`5e7abc5`	360.82 ms	401.18 ms	40.37 ms

App size

Revision	Plain	With Sentry	Diff
`e2d89fc`	6.06 MiB	7.03 MiB	989.37 KiB
`be173fa`	6.35 MiB	7.33 MiB	1005.54 KiB
`7b2e0ad`	6.52 MiB	7.61 MiB	1.08 MiB
`322aa66`	5.94 MiB	6.92 MiB	1005.75 KiB
`eecbbca`	5.94 MiB	6.89 MiB	975.78 KiB
`f3a18f2`	6.49 MiB	7.57 MiB	1.08 MiB
`e0ba81f`	6.52 MiB	7.59 MiB	1.06 MiB
`33527b4`	6.35 MiB	7.42 MiB	1.07 MiB
`3334ac1`	6.06 MiB	7.03 MiB	993.54 KiB
`5e7abc5`	6.34 MiB	7.28 MiB	966.66 KiB

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

Startup times

Revision	Plain	With Sentry	Diff
`981039c`	490.58 ms	539.26 ms	48.67 ms
`22f22b0`	468.33 ms	511.96 ms	43.63 ms
`73a4224`	510.66 ms	563.88 ms	53.22 ms
`ba079eb`	589.27 ms	642.40 ms	53.13 ms
`3606202`	672.45 ms	690.24 ms	17.80 ms
`35ebb0b`	446.44 ms	489.70 ms	43.26 ms
`42be7f3`	351.20 ms	372.54 ms	21.34 ms

App size

Revision	Plain	With Sentry	Diff
`981039c`	6.49 MiB	7.57 MiB	1.08 MiB
`22f22b0`	6.49 MiB	7.57 MiB	1.08 MiB
`73a4224`	6.49 MiB	7.57 MiB	1.08 MiB
`ba079eb`	6.49 MiB	7.57 MiB	1.08 MiB
`3606202`	6.49 MiB	7.57 MiB	1.08 MiB
`35ebb0b`	6.49 MiB	7.57 MiB	1.08 MiB
`42be7f3`	6.49 MiB	7.57 MiB	1.08 MiB

github-actions · 2024-10-16T14:31:48Z

iOS Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1239.41 ms	1267.28 ms	27.87 ms
Size	8.38 MiB	9.75 MiB	1.37 MiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`8cb6557`	1265.14 ms	1266.08 ms	0.94 ms
`bc29768`	1247.25 ms	1268.63 ms	21.38 ms
`3e5078c`	1239.73 ms	1248.69 ms	8.96 ms
`6957bfd`	1283.80 ms	1289.00 ms	5.20 ms
`0ceb89c`	1252.02 ms	1271.78 ms	19.75 ms
`d883d62`	1221.39 ms	1230.18 ms	8.80 ms
`1d47eb7`	1212.57 ms	1222.00 ms	9.43 ms
`21d4150`	1252.86 ms	1280.55 ms	27.69 ms
`6a5a65d`	1237.22 ms	1250.29 ms	13.07 ms
`b0811cc`	1238.23 ms	1255.82 ms	17.59 ms

App size

Revision	Plain	With Sentry	Diff
`8cb6557`	8.10 MiB	9.18 MiB	1.08 MiB
`bc29768`	8.32 MiB	9.38 MiB	1.05 MiB
`3e5078c`	8.28 MiB	9.34 MiB	1.06 MiB
`6957bfd`	8.15 MiB	9.15 MiB	1020.71 KiB
`0ceb89c`	8.15 MiB	9.12 MiB	989.78 KiB
`d883d62`	8.29 MiB	9.36 MiB	1.07 MiB
`1d47eb7`	8.29 MiB	9.39 MiB	1.09 MiB
`21d4150`	8.16 MiB	9.17 MiB	1.01 MiB
`6a5a65d`	8.34 MiB	9.65 MiB	1.31 MiB
`b0811cc`	8.32 MiB	9.38 MiB	1.06 MiB

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

Startup times

Revision	Plain	With Sentry	Diff
`73a4224`	1240.18 ms	1266.15 ms	25.96 ms
`42be7f3`	1249.76 ms	1269.80 ms	20.04 ms
`981039c`	1231.98 ms	1243.80 ms	11.82 ms
`35ebb0b`	1230.51 ms	1247.45 ms	16.94 ms
`ba079eb`	1247.86 ms	1255.13 ms	7.27 ms
`3606202`	1257.33 ms	1284.26 ms	26.93 ms
`22f22b0`	1248.92 ms	1274.13 ms	25.21 ms

App size

Revision	Plain	With Sentry	Diff
`73a4224`	8.38 MiB	9.75 MiB	1.37 MiB
`42be7f3`	8.38 MiB	9.75 MiB	1.37 MiB
`981039c`	8.38 MiB	9.75 MiB	1.37 MiB
`35ebb0b`	8.38 MiB	9.75 MiB	1.37 MiB
`ba079eb`	8.38 MiB	9.75 MiB	1.37 MiB
`3606202`	8.38 MiB	9.75 MiB	1.37 MiB
`22f22b0`	8.38 MiB	9.75 MiB	1.37 MiB

martinhaintz · 2024-10-16T14:57:21Z

@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.

Should we keep the settings separate, so the user can enable redacting for SR but disable it for SS?
Why is the SR feature experimental? Should the redacting settings for SS also stay experimental?

vaind · 2024-10-17T22:01:43Z

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.

vaind

I realize this is still a draft but wanted to have a first look early. Looks like you're on a good path to integrating these two features, I've just had some comments what could possibly be improved.

flutter/lib/src/event_processor/screenshot_event_processor.dart

vaind · 2024-10-17T22:07:18Z

flutter/lib/src/event_processor/screenshot_event_processor.dart

+      return bytes;
+    } else {
+      _options.logger(
+          SentryLevel.debug, 'Screenshot is 0 bytes, not attaching the image.');


Ever happened to you while testing? Looks like a bug.

No, not that I am aware of. This code was already here before.
Maybe @denrase knows more?

@vaind I looked up the toByteData method, and it will return null if an error occurs and also throws an exception. I would rewrite this, with a try/catch block, but we can also remove this code, if you think, that we don`t need it. WDYT?

flutter/lib/src/screenshot/recorder.dart

flutter/lib/src/screenshot/sentry_screenshot_quality.dart

flutter/lib/src/screenshot/recorder.dart

flutter/lib/src/sentry_replay_options.dart

flutter/lib/src/sentry_redaction_options.dart

flutter/lib/src/sentry_flutter.dart

vaind

I've started looking into this and it all looks pretty good, but I'll have a closer look later today/early tomorrow, especially around options.experimental.privacy and how a user would set it (in the sample app). I think it should be possible for the user to not have to create the instance itself (i.e. it can have a non-nullable public getter)

buenaflor

left some small feedback regarding changing redact -> mask in comments etc... and some changelog changes

generally looks good to me but I'll have ivan give the final ✅

buenaflor · 2024-10-29T12:44:25Z

flutter/lib/src/sentry_flutter.dart

+  /// As we have only one central place to configure the redaction,
+  /// we need to set the redaction options to full fill the above default settings.
+  /// The plan is to unify this behaviour with the next major release.
+  static void _setRedactionOptions(SentryFlutterOptions options) {


Suggested change

static void _setRedactionOptions(SentryFlutterOptions options) {

static void _setMaskingOptions(SentryFlutterOptions options) {

minor: since we changed the wording from redact -> mask, it might make sense to change it

buenaflor · 2024-10-29T12:49:08Z

flutter/lib/src/sentry_flutter.dart

+  /// Screen redaction was previously introduced with the SessionReplay feature.
+  /// Screen redaction is enabled by default for SessionReplay.
+  /// As we also to use this feature for Screenshot, which previously was not
+  /// capable of redacting the screenshot, we need to disable redaction for Screenshot by default
+  /// so we don`t break the existing behavior.
+  /// As we have only one central place to configure the redaction,
+  /// we need to set the redaction options to full fill the above default settings.
+  /// The plan is to unify this behaviour with the next major release.


Suggested change

/// Screen redaction was previously introduced with the SessionReplay feature.

/// Screen redaction is enabled by default for SessionReplay.

/// As we also to use this feature for Screenshot, which previously was not

/// capable of redacting the screenshot, we need to disable redaction for Screenshot by default

/// so we don`t break the existing behavior.

/// As we have only one central place to configure the redaction,

/// we need to set the redaction options to full fill the above default settings.

/// The plan is to unify this behaviour with the next major release.

/// Masking behaviour

/// - If only Screenshot is enabled: masking is disabled by default.

/// - If both Screenshot and Session Replay are enabled: masking is enabled for both by default.

/// - If the user explicitly sets masking to false: masking is disabled for both features.

/// We don't want to break the existing screenshot integration which is not masked by default.

/// The plan is to unify screenshot and replay masking with the next major release.

suggestion to keep it a bit shorter

buenaflor · 2024-10-29T12:49:44Z

flutter/lib/src/sentry_privacy_options.dart

+/// Configuration of the experimental privacy feature.
+class SentryPrivacyOptions {
+  /// Mask all text content. Draws a rectangle of text bounds with text color
+  /// on top. Currently, only [Text] and [EditableText] Widgets are redacted.


Suggested change

/// on top. Currently, only [Text] and [EditableText] Widgets are redacted.

/// on top. Currently, only [Text] and [EditableText] Widgets are masked.

let's keep the naming consistent

buenaflor · 2024-10-29T12:56:03Z

CHANGELOG.md

@@ -5,6 +5,7 @@
 ### Enhancements

 - Cache parsed DSN ([#2365](https://github.com/getsentry/sentry-dart/pull/2365))
+- Switching from traditional screenshot to view hierarchy for screenshots which allows redacting ([#2361](https://github.com/getsentry/sentry-dart/pull/2361))


the wording is a bit confusing from a user perspective we can improve it by sayingAllow Screenshots to be masked for privacy reasons or something like that

Also show an exampl how a user can enable it and please add the screenshot & replay masking behaviour (the one that I commented) as sub bullet points for more context so it's clear how it behaves

buenaflor · 2024-10-29T12:56:35Z

CHANGELOG.md

 ### Enhancements

 - Cache parsed DSN ([#2365](https://github.com/getsentry/sentry-dart/pull/2365))
+- Switching from traditional screenshot to view hierarchy for screenshots which allows redacting ([#2361](https://github.com/getsentry/sentry-dart/pull/2361))


The changelog entry should be under ## Features, if there is none please create it

vaind

generally looks good, please have a look at my PR mentioned below.

However, there's some bug with the sizing because the image is captured too small (with transparent offset on the right and bottom that is actually part of the image):
https://sentry-sdks.sentry.io/issues/4722238716/events/f071c8f05dac430ba64411635fcb49da/

When previously it would show up correctly (size-wise, ignore the content of this one):
https://sentry-sdks.sentry.io/issues/4722238716/events/7479041de7b14cfcaf870d55ac1023ad/

vaind · 2024-10-30T18:52:05Z

flutter/lib/src/sentry_screenshot_options.dart

+  /// Requires adding the [SentryScreenshotWidget] to the widget tree.
+  /// Example:
+  /// runApp(SentryScreenshotWidget(child: App()));
+  /// The [SentryScreenshotWidget] has to be the root widget of the app.


We should point users to use the single root widget:

Suggested change

/// The [SentryScreenshotWidget] has to be the root widget of the app.

/// The [SentryWidget] has to be the root widget of the app.

vaind · 2024-10-30T18:55:39Z

flutter/lib/src/sentry_flutter_options.dart

-  Hint? hint,
-});
+  /// Privacy configuration for masking sensitive data in the Screenshot and Session Replay.
+  SentryPrivacyOptions? privacy;


I've made suggestion how we can make this more user-friendly not having to initialize the field) in another PR on top of this one: #2382

martinhaintz added 2 commits October 15, 2024 10:19

use view hierachy for screenshots

42be7f3

fix tests and distinguish between ScheduledScreenshotRecorderConfig a…

49a413b

…nd ScreenshotRecorderConfig.

Merge branch 'main' into feat/redact-screenshots-via-view-hierarchy

d283df9

buenaflor reviewed Oct 15, 2024

View reviewed changes

flutter/lib/src/sentry_replay_options.dart Show resolved Hide resolved

martinhaintz added 3 commits October 16, 2024 16:06

removed unused imports

fdc7d6d

Merge branch 'main' into feat/redact-screenshots-via-view-hierarchy

ea94d47

remove unused test

981039c

add changelog

4671c53

martinhaintz requested a review from vaind October 16, 2024 14:52

vaind reviewed Oct 17, 2024

View reviewed changes

martinhaintz added 5 commits October 22, 2024 14:55

rename variable

f722191

add internal

22f22b0

split into screenshot, screenreplay and redaction options

9bb14ad

fix comments

0969b7b

Merge branch 'main' into feat/redact-screenshots-via-view-hierarchy

73a4224

vaind reviewed Oct 24, 2024

View reviewed changes

flutter/lib/src/sentry_redaction_options.dart Outdated Show resolved Hide resolved

martinhaintz added 2 commits October 25, 2024 11:25

export redaction options and remove unused dependencies

c2daf00

renaming to SentryPrivacyOptions

3606202

martinhaintz marked this pull request as ready for review October 25, 2024 09:54

martinhaintz requested review from krystofwoldrich and stefanosiano as code owners October 25, 2024 09:54

martinhaintz requested review from buenaflor and vaind October 25, 2024 09:54

buenaflor reviewed Oct 28, 2024

View reviewed changes

flutter/lib/src/sentry_flutter.dart Show resolved Hide resolved

martinhaintz added 3 commits October 29, 2024 09:57

add explanation for setRedactionOptions

35ebb0b

Merge branch 'main' into feat/redact-screenshots-via-view-hierarchy

ba079eb

fix deprecation warnings

57a0823

martinhaintz requested a review from buenaflor October 29, 2024 10:07

vaind reviewed Oct 29, 2024

View reviewed changes

buenaflor requested changes Oct 29, 2024

View reviewed changes

vaind mentioned this pull request Oct 30, 2024

feat: non-nullable privacy setting #2382

Open

vaind requested changes Oct 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

redact screenshots via view hierarchy #2361

redact screenshots via view hierarchy #2361

martinhaintz commented Oct 15, 2024 •

edited

Loading

github-actions bot commented Oct 15, 2024 •

edited

Loading

codecov bot commented Oct 15, 2024 •

edited

Loading

github-actions bot commented Oct 16, 2024 •

edited

Loading

Baseline results on branch: main

Startup times

App size

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

Startup times

App size

github-actions bot commented Oct 16, 2024 •

edited

Loading

Baseline results on branch: main

Startup times

App size

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

Startup times

App size

martinhaintz commented Oct 16, 2024 •

edited

Loading

vaind commented Oct 17, 2024

vaind left a comment

vaind Oct 17, 2024

martinhaintz Oct 22, 2024

martinhaintz Oct 25, 2024

vaind left a comment

buenaflor left a comment

buenaflor Oct 29, 2024

buenaflor Oct 29, 2024

buenaflor Oct 29, 2024

buenaflor Oct 29, 2024

buenaflor Oct 29, 2024

vaind left a comment •

edited

Loading

vaind Oct 30, 2024

vaind Oct 30, 2024

	static void _setRedactionOptions(SentryFlutterOptions options) {
	static void _setMaskingOptions(SentryFlutterOptions options) {

	/// on top. Currently, only [Text] and [EditableText] Widgets are redacted.
	/// on top. Currently, only [Text] and [EditableText] Widgets are masked.

	/// The [SentryScreenshotWidget] has to be the root widget of the app.
	/// The [SentryWidget] has to be the root widget of the app.

redact screenshots via view hierarchy #2361

Are you sure you want to change the base?

redact screenshots via view hierarchy #2361

Conversation

martinhaintz commented Oct 15, 2024 • edited Loading

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Oct 15, 2024 • edited Loading

codecov bot commented Oct 15, 2024 • edited Loading

Codecov Report

github-actions bot commented Oct 16, 2024 • edited Loading

Android Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

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

Startup times

App size

github-actions bot commented Oct 16, 2024 • edited Loading

iOS Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

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

Startup times

App size

martinhaintz commented Oct 16, 2024 • edited Loading

vaind commented Oct 17, 2024

vaind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaind left a comment

Choose a reason for hiding this comment

buenaflor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaind left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinhaintz commented Oct 15, 2024 •

edited

Loading

github-actions bot commented Oct 15, 2024 •

edited

Loading

codecov bot commented Oct 15, 2024 •

edited

Loading

github-actions bot commented Oct 16, 2024 •

edited

Loading

github-actions bot commented Oct 16, 2024 •

edited

Loading

martinhaintz commented Oct 16, 2024 •

edited

Loading

vaind left a comment •

edited

Loading