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 mobile platforms to onboarding sidebar #76709

Merged
merged 14 commits into from
Sep 3, 2024

Conversation

michellewzhang
Copy link
Member

@michellewzhang michellewzhang commented Aug 28, 2024

closes #67695

android:

Screen.Recording.2024-08-29.at.11.54.50.AM.mov

react native:

Screen.Recording.2024-08-29.at.11.55.57.AM.mov

apple:

Screen.Recording.2024-08-29.at.11.56.19.AM.mov

@michellewzhang michellewzhang requested a review from a team as a code owner August 28, 2024 22:13
@michellewzhang michellewzhang requested review from a team and bruno-garcia August 28, 2024 22:13
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Aug 28, 2024
Copy link
Contributor

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

Apple onboarding looks good

Comment on lines 87 to 90
Sentry.mobileReplayIntegration({
maskAllText: true,
maskAllImages: true,
}),
Copy link
Member

Choose a reason for hiding this comment

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

These values are repeated in the configuration snippet, I would leave them out here since they are not needed and will make the init snipped a bit less overwhelming.

Also it will match the Browser JS which also by default doesn't show any config options.

Suggested change
Sentry.mobileReplayIntegration({
maskAllText: true,
maskAllImages: true,
}),
Sentry.mobileReplayIntegration(),

Copy link
Member Author

Choose a reason for hiding this comment

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

oh yes, good point -- i copied this from the docs so maybe we want to update it there too?

https://docs.sentry.io/platforms/react-native/session-replay/

Copy link
Member Author

Choose a reason for hiding this comment

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

@krystofwoldrich
Copy link
Member

krystofwoldrich commented Aug 29, 2024

In our other guides the Install step shows how to add the SDK to a project, do we want it here as well?

@krystofwoldrich
Copy link
Member

Just a two small nits, otherwise RN looks good!

@michellewzhang
Copy link
Member Author

In our other guides the Install step shows how to add the SDK to a project, do we want it here as well?

yup, I can add that

Copy link
Member

@billyvg billyvg left a comment

Choose a reason for hiding this comment

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

I did not check the android/ios syntax but everything else looks good

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

Sentry.init({
dsn = "${params.dsn.public}",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
dsn = "${params.dsn.public}",
dsn: "${params.dsn.public}",

Copy link
Member Author

@michellewzhang michellewzhang Aug 29, 2024

Choose a reason for hiding this comment

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

oh if this is the right syntax we should also update our docs, it's wrong there too https://docs.sentry.io/platforms/react-native/session-replay/

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this was an oversight in the docs, thank you for fixing it there as well.

Comment on lines +82 to +85
_experiments: {
replaysSessionSampleRate: 1.0,
replaysOnErrorSampleRate: 1.0,
},
Copy link
Member

Choose a reason for hiding this comment

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

I'm not up to date on the RN sdk but should this still be in _experiments?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Yes, _experiments is correct.

Copy link
Member

@krystofwoldrich krystofwoldrich left a comment

Choose a reason for hiding this comment

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

RN Looks good!

Copy link
Contributor

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

Apple looks good!

{
label: 'Kotlin',
value: 'kotlin',
language: 'java',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
language: 'java',
language: 'kotlin',

Copy link
Member Author

Choose a reason for hiding this comment

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

ah okay, didn't know what was the standard since other onboardings use java for the language for kotlin

Copy link
Member Author

Choose a reason for hiding this comment

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

updated those here: #76876

options.isDebug = true

// Currently under experimental options:
options.experimental.sessionReplay.errorSampleRate = 1.0
Copy link
Member

Choose a reason for hiding this comment

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

FYI: i've already merged the PR that changes the name to onErrorSampleRate - do we wanna wait for the new SDK release and change it now, or rather ship this and follow-up later?

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm @romtsn let's follow up perhaps, since we need to update the docs anyway - can do both at the same time. feel free to ping me to update when the SDK release is out

Copy link
Member

@romtsn romtsn left a comment

Choose a reason for hiding this comment

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

left a few comments, but otherwise LGTM! If we don't wanna wait for the new Android SDK version, let's merge it

@michellewzhang michellewzhang merged commit 448060a into master Sep 3, 2024
42 of 43 checks passed
@michellewzhang michellewzhang deleted the mz/onboarding-mobile branch September 3, 2024 19:04
@github-actions github-actions bot locked and limited conversation to collaborators Sep 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Replay Index] Add onboarding for mobile platforms
7 participants