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(toolbar): add a replay panel for start/stop current replay #75403

Merged
merged 31 commits into from
Aug 8, 2024

Conversation

aliu39
Copy link
Member

@aliu39 aliu39 commented Jul 31, 2024

Closes #74583
Closes #74452

getReplay returns undefined:
Screenshot 2024-08-08 at 10 28 18 AM

SentrySDK doesn't have getReplay method:
Screenshot 2024-08-08 at 10 22 25 AM

SentrySDK is falsey (failed to import the package):
Screenshot 2024-08-08 at 10 22 02 AM

If you want to checkout this branch to test it, you need to run dev-ui in getsentry and make local changes:

Notes:

  • "last recorded replay" is persisted with sessionStorage
  • stopping then starting will make a new replay
  • uses some try-catch logic to handle older SDK versions where the recording fxs throw.

Follow-ups before merging

  • Analytics context provider and start/stop button analytics (todo in this PR)
  • comment on SDK versioning
    • exact release of getReplay/public API is unknown, but it was ~2yr ago near the release of the whole product
  • test with v8.18
  • remove the debug flag
  • make the links work for dev mode (can hard-code the sentry-test/app-frontend org/proj)

Follow-ups after merging

  • test the links work in prod
  • account for minimum replay duration (so users don't stop too early)
  • add more content to the panel! Open to ideas. Can do this in a separate PR
  • keep dogfooding for bugs, w/different sample rates, sdk versions

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Jul 31, 2024
Copy link

codecov bot commented Jul 31, 2024

Bundle Report

Changes will increase total bundle size by 7.23kB ⬆️

Bundle name Size Change
app-webpack-bundle-array-push 28.65MB 7.23kB ⬆️

@aliu39 aliu39 changed the title Aliu/start replay feat(toolbar): add a replay panel for start/stop current replay Aug 1, 2024
package.json Outdated Show resolved Hide resolved
@aliu39

This comment was marked as resolved.

Copy link

codecov bot commented Aug 3, 2024

Codecov Report

Attention: Patch coverage is 0% with 57 lines in your changes missing coverage. Please review.

Project coverage is 78.22%. Comparing base (14c8790) to head (1fc533d).
Report is 23 commits behind head on master.

Files Patch % Lines
.../components/devtoolbar/hooks/useReplayRecorder.tsx 0.00% 45 Missing ⚠️
...nents/devtoolbar/components/replay/replayPanel.tsx 0.00% 10 Missing ⚠️
...p/components/devtoolbar/components/panelRouter.tsx 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #75403      +/-   ##
==========================================
- Coverage   80.06%   78.22%   -1.84%     
==========================================
  Files        6996     6830     -166     
  Lines      312723   303280    -9443     
  Branches    52171    52182      +11     
==========================================
- Hits       250370   237244   -13126     
+ Misses      61911    59661    -2250     
- Partials      442     6375    +5933     
Files Coverage Δ
static/app/bootstrap/initializeSdk.tsx 35.45% <ø> (ø)
...pp/components/devtoolbar/components/navigation.tsx 0.00% <ø> (ø)
...pp/components/devtoolbar/hooks/useToolbarRoute.tsx 0.00% <ø> (ø)
...p/components/devtoolbar/components/panelRouter.tsx 0.00% <0.00%> (ø)
...nents/devtoolbar/components/replay/replayPanel.tsx 0.00% <0.00%> (ø)
.../components/devtoolbar/hooks/useReplayRecorder.tsx 0.00% <0.00%> (ø)

... and 1787 files with indirect coverage changes

@aliu39

This comment was marked as outdated.

package.json Outdated Show resolved Hide resolved
export default function useReplayRecorder(): ReplayRecorderState {
const {SentrySDK} = useConfiguration();
const replay =
SentrySDK && 'getReplay' in SentrySDK ? SentrySDK.getReplay() : undefined;
Copy link
Member

Choose a reason for hiding this comment

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

you can use SentrySDK.getIntegrationByName(), it's just a bit more annoying dealing with types

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm my VSCode can't find this method in browser or react packages..

Copy link
Member

Choose a reason for hiding this comment

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

hmm it might be on the SDK client instead of the Sentry namespace... e.g. SentrySDK.getClient().getIntegrationByName

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it, does this have any improvements over the current approach? Right now I'm using these conditions to get the disabledReason (see screenshots I just added in the description)

Copy link
Member

Choose a reason for hiding this comment

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

Ah just as a fallback if the SDK is too old to support getReplay as it was a relatively recent addition, but it's fine not to support older

Comment on lines 28 to 33
organizationSlug:
process.env.NODE_ENV !== 'development' ? 'sentry' : 'sentry-test',
projectId: process.env.NODE_ENV !== 'development' ? 11276 : 5270453,
projectPlatform: 'javascript',
projectSlug: 'javascript',
projectSlug:
process.env.NODE_ENV !== 'development' ? 'javascript' : 'app-frontend',
Copy link
Member

Choose a reason for hiding this comment

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

Should this be in here?

Copy link
Member Author

Choose a reason for hiding this comment

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

While this is PoC we are hard coding the org/project slugs, this allows the links to work in dev mode (assuming the replay integration is enabled)

Copy link
Member

@ryan953 ryan953 Aug 8, 2024

Choose a reason for hiding this comment

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

can you test by running the app with yarn dev-ui. my workflow depends on that command

Copy link
Member Author

@aliu39 aliu39 Aug 8, 2024

Choose a reason for hiding this comment

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

It works, but you need to comment out this condition in getsentry's useReplayInit: https://github.com/getsentry/getsentry/blob/2a1da081f3a9e4e4111b577d5551fa24691da374/static/getsentry/gsApp/utils/useReplayInit.tsx#L85-L87

I think it's disabled so we don't record everyone's dev-ui sessions. Maybe we could uncomment but set both sample rates to 0?

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'll make a ticket and add it to the dev toolbar project

Copy link
Member

Choose a reason for hiding this comment

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

probably hold off on a ticket to change useReplayInit. The toolbar should support whatever env it finds itself inside of. So if sometimes there's no replayIntegration at all, that should be ok. We'll be able to test that case in dev, and on prod there will be a replayIntegration so we can test that case as well!

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 wasn't thinking about the other panels, this change would break them for dev. I'm done testing, let's just leave this panel disabled in dev.

disabled={isDisabled || buttonLoading}
onClick={async () => {
setButtonLoading(true);
isRecordingSession ? await stopRecording() : await startRecordingSession();
Copy link
Member Author

Choose a reason for hiding this comment

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

Not allowing users to stop buffering.


const startRecordingSession = useCallback(async () => {
let success = false;
if (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.

Don't want to try-catch here because we can already condition to make sure we're in a safe state.

@aliu39 aliu39 merged commit a055c3b into master Aug 8, 2024
41 of 42 checks passed
@aliu39 aliu39 deleted the aliu/start-replay branch August 8, 2024 21:34
@github-actions github-actions bot locked and limited conversation to collaborators Aug 24, 2024
This pull request was closed.
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
4 participants