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

[SR] Add custom redaction options #3689

Merged
merged 13 commits into from
Sep 16, 2024
Merged

Conversation

romtsn
Copy link
Member

@romtsn romtsn commented Sep 13, 2024

📜 Description

  • Added a possibility to redact/ignore view types by adding the fq class name to the redact/ignore list in replay options
    • This will also consider subclasses of the added view type (e.g. EditText will be redacted because it inherits from TextView)
  • Added a possibility to redact/ignore certain views by setting a tag
    • Either a regular setTag("sentry-redact|sentry-ignore")
    • Or in case of multiple tags for a view setTag(R.id.sentry_privacy, "redact|ignore")
  • Added convenient view extensions that set tags under the hood
  • Removed the redactAllText and redactAllImages booleans in favour of just adding the android view classes to the redactClasses list
  • Some minor fixes along-the-way in Windows.kt where we were not properly handling double-init of the SDK

Also note that the order in which we define whether to redact a view or not matters, and marking individual views has precedence over a class. You can read up more on it here: https://www.notion.so/sentry/Redaction-Ignore-Mechanisms-for-Views-45e1302e8d844bd0b83a274a5d759e3f

💡 Motivation and Context

Part of getsentry/sentry#74441

💚 How did you test it?

Manually + automated

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • [] No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

*/
private boolean redactAllImages = true;
private Set<String> redactClasses = new CopyOnWriteArraySet<>();
Copy link
Member Author

Choose a reason for hiding this comment

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

@brustolin @bruno-garcia I guess we should align on naming here. Do you prefer this or redactViewTypes? I'm fine with either

Choose a reason for hiding this comment

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

I think we should align but just Classes is too generic, what about "redactViewClasses"?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, sounds good. I can also change it to redactViewTypes so you don't have to :)

Copy link
Contributor

github-actions bot commented Sep 13, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 399.69 ms 499.12 ms 99.43 ms
Size 1.70 MiB 2.35 MiB 661.11 KiB

Previous results on branch: rz/feat/session-replay-custom-redaction

Startup times

Revision Plain With Sentry Diff
2c39e81 540.60 ms 628.22 ms 87.62 ms
631f884 389.28 ms 439.36 ms 50.08 ms

App size

Revision Plain With Sentry Diff
2c39e81 1.70 MiB 2.35 MiB 661.12 KiB
631f884 1.70 MiB 2.35 MiB 661.11 KiB

Copy link

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

Looks good to me.

One question: We just need to redact TextView? All the input views are based on TextView?

Comment on lines +16 to +17
@Deprecated("Getter is unsupported.", level = DeprecationLevel.ERROR)
get() = error("Getter not supported")

Choose a reason for hiding this comment

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

l: Why is this deprecated?

Copy link
Member Author

Choose a reason for hiding this comment

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

I explained in a comment on top of the file, but basically we scream to the dev that this should not be used to get the value. This extension only exists to set the value conveniently in Kotlin (redactAllText = true instead of setRedactAllText(true)), so only the setter matters here

Choose a reason for hiding this comment

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

Thanks!! I didn't get the comment.

Can you just remove get() as in C# where you can have a property with just a setter?

Copy link
Member Author

Choose a reason for hiding this comment

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

nope, unfortunately not possible in Kotlin. It's possible to only have a getter without a setter, but not the other way around

@romtsn
Copy link
Member Author

romtsn commented Sep 16, 2024

Looks good to me.

One question: We just need to redact TextView? All the input views are based on TextView?

yep, EditText, RadioButton and others all inherit from TextView
image

Copy link
Member

@markushi markushi left a comment

Choose a reason for hiding this comment

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

LGTM!

@romtsn romtsn enabled auto-merge (squash) September 16, 2024 16:48
@romtsn romtsn merged commit 6368d4f into main Sep 16, 2024
25 checks passed
@romtsn romtsn deleted the rz/feat/session-replay-custom-redaction branch September 16, 2024 17:04
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