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

feat(replay): Add SentryMask and SentryUnmask native components #4224

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

krystofwoldrich
Copy link
Member

@krystofwoldrich krystofwoldrich commented Oct 31, 2024

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

This PR adds custom redact/mask components for iOS.

The implementation includes noop fallback for Android and other unsupported platforms and fallback Web implementation. These will be added in a follow up PRs.

import * as Sentry from '@sentry/react-native'

const Example = () => (
  <View>
    <Sentry.Unmask>
      <Text>This is unmasked because it's direct child of Sentry.Unmask (can be masked if Sentry.Masked is used higher in the hierarchy)</Text>
      <Sentry.Mask>
        <Text>This is masked always because it's a child of a Sentry.Mask</Text>
        <Sentry.Unmask>
          {/* Sentry.Unmask does not override the Sentry.Mask from above in the hierarchy */}
          <Text>This is masked always because it's a child of Sentry.Mask</Text>
        </Sentry.Unmask>
      </Sentry.Mask>
    </Sentry.Unmask>
  </View>
);
Device Replay
Screenshot 2024-11-11 at 16 24 03 Screenshot 2024-11-11 at 16 24 41

Todo

💡 Motivation and Context

This is a part of RN Replay GA tasks

💚 How did you test it?

ci, local,

📝 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
  • All tests passing
  • No breaking changes

🔮 Next steps

Copy link
Contributor

github-actions bot commented Oct 31, 2024

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

Generated by 🚫 dangerJS against e0ccb78

Copy link
Contributor

github-actions bot commented Oct 31, 2024

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 450.69 ms 441.20 ms -9.48 ms
Size 17.74 MiB 20.07 MiB 2.34 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
70e6261 482.65 ms 495.70 ms 13.05 ms
76d1baf+dirty 335.72 ms 355.52 ms 19.80 ms
22e31b6 396.48 ms 419.64 ms 23.16 ms
fe13591 478.92 ms 480.84 ms 1.92 ms
d197b5c+dirty 338.94 ms 354.87 ms 15.93 ms
ad6c299 375.94 ms 382.02 ms 6.08 ms
abb7058 370.27 ms 389.58 ms 19.31 ms
2ec71da 438.14 ms 460.46 ms 22.32 ms
31fcca2 391.22 ms 414.78 ms 23.56 ms
62a750b 395.96 ms 423.36 ms 27.41 ms

App size

Revision Plain With Sentry Diff
70e6261 17.73 MiB 19.94 MiB 2.21 MiB
76d1baf+dirty 17.73 MiB 20.04 MiB 2.31 MiB
22e31b6 17.73 MiB 19.84 MiB 2.10 MiB
fe13591 17.74 MiB 20.07 MiB 2.34 MiB
d197b5c+dirty 17.73 MiB 20.04 MiB 2.31 MiB
ad6c299 17.73 MiB 19.75 MiB 2.02 MiB
abb7058 17.73 MiB 19.83 MiB 2.10 MiB
2ec71da 17.73 MiB 20.10 MiB 2.37 MiB
31fcca2 17.73 MiB 19.90 MiB 2.17 MiB
62a750b 17.73 MiB 19.93 MiB 2.20 MiB

Copy link
Contributor

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1227.00 ms 1238.17 ms 11.17 ms
Size 2.92 MiB 3.64 MiB 743.77 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
12427f4+dirty 1224.90 ms 1231.40 ms 6.50 ms
5446992+dirty 1249.94 ms 1254.80 ms 4.86 ms
e73d82f+dirty 1231.20 ms 1228.81 ms -2.40 ms
148f924+dirty 1220.72 ms 1221.30 ms 0.58 ms
fe13591+dirty 1250.69 ms 1246.27 ms -4.43 ms
1faf8e3+dirty 1223.38 ms 1220.56 ms -2.82 ms
484813b+dirty 1225.07 ms 1221.00 ms -4.07 ms
4a6664f+dirty 1218.77 ms 1221.07 ms 2.30 ms
e5bc97b+dirty 1229.17 ms 1227.64 ms -1.54 ms
15c80ab+dirty 1248.41 ms 1251.24 ms 2.83 ms

App size

Revision Plain With Sentry Diff
12427f4+dirty 2.92 MiB 3.44 MiB 533.29 KiB
5446992+dirty 2.92 MiB 3.44 MiB 535.26 KiB
e73d82f+dirty 2.92 MiB 3.64 MiB 740.56 KiB
148f924+dirty 2.92 MiB 3.60 MiB 701.88 KiB
fe13591+dirty 2.92 MiB 3.66 MiB 757.71 KiB
1faf8e3+dirty 2.92 MiB 3.64 MiB 742.61 KiB
484813b+dirty 2.92 MiB 3.64 MiB 740.56 KiB
4a6664f+dirty 2.92 MiB 3.60 MiB 702.09 KiB
e5bc97b+dirty 2.92 MiB 3.66 MiB 758.40 KiB
15c80ab+dirty 2.92 MiB 3.39 MiB 481.56 KiB

Copy link
Contributor

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1225.12 ms 1220.30 ms -4.82 ms
Size 2.36 MiB 3.08 MiB 737.88 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
12427f4+dirty 1267.15 ms 1271.30 ms 4.15 ms
5446992+dirty 1273.28 ms 1276.68 ms 3.40 ms
e73d82f+dirty 1207.52 ms 1216.73 ms 9.21 ms
148f924+dirty 1214.76 ms 1215.73 ms 0.97 ms
fe13591+dirty 1208.25 ms 1219.53 ms 11.28 ms
1faf8e3+dirty 1214.87 ms 1222.83 ms 7.97 ms
484813b+dirty 1222.45 ms 1220.79 ms -1.66 ms
4a6664f+dirty 1209.49 ms 1208.63 ms -0.86 ms
e5bc97b+dirty 1230.63 ms 1234.83 ms 4.20 ms
15c80ab+dirty 1223.74 ms 1228.96 ms 5.22 ms

App size

Revision Plain With Sentry Diff
12427f4+dirty 2.36 MiB 2.88 MiB 530.38 KiB
5446992+dirty 2.36 MiB 2.88 MiB 531.94 KiB
e73d82f+dirty 2.36 MiB 3.08 MiB 734.23 KiB
148f924+dirty 2.36 MiB 3.04 MiB 696.25 KiB
fe13591+dirty 2.36 MiB 3.10 MiB 752.40 KiB
1faf8e3+dirty 2.36 MiB 3.08 MiB 736.75 KiB
484813b+dirty 2.36 MiB 3.08 MiB 734.18 KiB
4a6664f+dirty 2.36 MiB 3.04 MiB 696.39 KiB
e5bc97b+dirty 2.36 MiB 3.10 MiB 753.14 KiB
15c80ab+dirty 2.36 MiB 2.83 MiB 474.49 KiB

@krystofwoldrich krystofwoldrich changed the base branch from main to kw/fix/type-check November 6, 2024 15:21
Base automatically changed from kw/fix/type-check to main November 6, 2024 16:35
Copy link
Contributor

github-actions bot commented Nov 8, 2024

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 390.04 ms 454.89 ms 64.85 ms
Size 7.15 MiB 8.35 MiB 1.20 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
76d1baf+dirty 339.02 ms 408.65 ms 69.63 ms
86d6d2c+dirty 267.21 ms 325.24 ms 58.04 ms
f06c879+dirty 361.27 ms 407.88 ms 46.61 ms
8900e1a+dirty 371.40 ms 377.70 ms 6.31 ms
e540498+dirty 408.56 ms 480.00 ms 71.44 ms
62a750b+dirty 370.78 ms 376.73 ms 5.96 ms
2ec71da+dirty 375.64 ms 431.59 ms 55.95 ms
c639edf+dirty 363.39 ms 414.78 ms 51.39 ms
acadc0f+dirty 259.04 ms 304.67 ms 45.63 ms
27ef4ee+dirty 296.71 ms 351.00 ms 54.29 ms

App size

Revision Plain With Sentry Diff
76d1baf+dirty 7.15 MiB 8.09 MiB 964.41 KiB
86d6d2c+dirty 7.15 MiB 8.09 MiB 962.69 KiB
f06c879+dirty 7.15 MiB 8.12 MiB 997.78 KiB
8900e1a+dirty 7.15 MiB 8.03 MiB 901.79 KiB
e540498+dirty 7.15 MiB 8.38 MiB 1.23 MiB
62a750b+dirty 7.15 MiB 8.21 MiB 1.06 MiB
2ec71da+dirty 7.15 MiB 8.38 MiB 1.23 MiB
c639edf+dirty 7.15 MiB 8.35 MiB 1.20 MiB
acadc0f+dirty 7.15 MiB 8.03 MiB 903.20 KiB
27ef4ee+dirty 7.15 MiB 8.08 MiB 959.49 KiB

+ (NSArray *_Nonnull)getReplayRNRedactClasses:(NSDictionary *_Nullable)replayOptions
{
NSMutableArray *_Nonnull classesToRedact = [[NSMutableArray alloc] init];
// We can't import RNSentryReplayMask.h here because it's Objective-C++
// To avoid typos, we test the class existens in the tests
[classesToRedact addObject:@"RNSentryReplayMask"];
Copy link
Member Author

Choose a reason for hiding this comment

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

For Unmask to work this PR requires changes from getsentry/sentry-cocoa#4522

Add add call of PrivateSentrySDKOnly.setIgnoreWrapperClass(IgnoreWrapper.self)

@krystofwoldrich krystofwoldrich marked this pull request as ready for review November 11, 2024 09:50
const { Mask, Unmask } = require('../../src/js/replay/CustomMask');

expect(Mask).toBeDefined();
expect(Unmask).toBeDefined();
Copy link
Collaborator

@lucas-zimerman lucas-zimerman Nov 11, 2024

Choose a reason for hiding this comment

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

Could we also validate that logger.warn was called to make sure they are actually the fallback?
Same for the below tests with fallback.

+ (NSArray *_Nonnull)getReplayRNUnmaskClasses
{
// We can't import RNSentryReplayUnmask.h here because it's Objective-C++
// To avoid typos, we test the class existens in the tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// To avoid typos, we test the class existens in the tests
// To avoid typos, we test the class existent in the tests

@@ -56,6 +64,10 @@ + (NSArray *_Nonnull)getReplayRNRedactClasses:(NSDictionary *_Nullable)replayOpt

+ (void)postInit
{
// We can't import RNSentryReplayMask.h here because it's Objective-C++
// To avoid typos, we test the class existens in the tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// To avoid typos, we test the class existens in the tests
// To avoid typos, we test the class existent in the tests

const Example = () => {
return (
<View>
<Sentry.Mask>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: for now, <Sentry.Mask> / <Sentry.Unmask> are exclusive to native/ React Native?

RNSentryReplayBreadcrumbConverter *breadcrumbConverter =
[[RNSentryReplayBreadcrumbConverter alloc] init];
[PrivateSentrySDKOnly configureSessionReplayWith:breadcrumbConverter screenshotProvider:nil];
}

+ (Class) getMaskClass
{
return NSClassFromString(@"RNSentryReplayMask");
Copy link
Collaborator

Choose a reason for hiding this comment

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

l: This is a suggestion since the current approach works as expected and is covered by test cases.
Wdyt of using an objective-c wrapper to get the class names statically? Eg:

// RNSentryReplayWrapper.h

@interface RNSentryReplayWrapper : NSObject

+ (Class) getMaskClass;
+ (Class) getUnmaskClass

@end
// RNSentryReplayWrapper.mm

#import "RNSentryReplayWrapper.h"
#include "RNSentryReplayMask.h"
#include "RNSentryReplayUnmask.h"

@implementation MyObjectiveCWrapper

+ (Class)getMaskClass
{
    return RNSentryReplayMask.class;
}

+ (Class)getUnmaskClass
{
    return RNSentryReplayUnmask.class;
}

@end

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