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

Session Replay start() method do not work after stop() in some cases #7192

Closed
3 tasks done
Tracked by #7540
expcapitaldev opened this issue Feb 15, 2023 · 12 comments
Closed
3 tasks done
Tracked by #7540

Comments

@expcapitaldev
Copy link

expcapitaldev commented Feb 15, 2023

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using? If you use the CDN bundles, please specify the exact bundle (e.g. bundle.tracing.min.js) in your SDK setup.

@sentry/angular

SDK Version

7.37.1

Framework Version

"@sentry/angular": "7.37.1",

Link to Sentry event

No response

SDK Setup

Sentry.init({
dns: myDNS,
// without Replay intergation here, because we do not want create webworker now
...,
replaysSessionSampleRate: 0, // I understand that 0 do not start session, I want to start and init with delay
replaysOnErrorSampleRate: 0,
});
...
private myStart(){
const client: Client | undefined = Sentry.getCurrentHub().getClient();
// enableReplay for example from variable from server
if (client) {
	const currentReplayIntegration: Replay | null = client.getIntegration(Replay);
	if (enableReplay) {
		const options: BrowserOptions = client.getOptions();
		options.replaysSessionSampleRate = 1; // I understand that I need to start with 1
		if (currentReplayIntegration) {
			currentReplayIntegration.flush();
			currentReplayIntegration.stop();
// TODO if I have current session and I do not know now it is is progress or not I can not check it and I want to finish current replay session and start new or start new, I can safety use flush and stop
// in some cases session do not started (I see in network that POST request do not send to sentry server), also it is do not work with timeout for example, for example after 3-5 calling method myStart() you can check that issue
			currentReplayIntegration.start();
		} else {
			client.addIntegration(this.getSentryReplayIntegration());
		}
	} else {
		if (currentReplayIntegration) {
			currentReplayIntegration.flush();
			currentReplayIntegration.stop();
		}
	}
} else {
	// unexpected case, log error here
}
}

Steps to Reproduce

see above

Expected Result

after ReplayIntergation.stop() I can call .start() and new session should be started and send to the server

Actual Result

looks like after stop() new session can not started but I see new Worker created with new ID but POST requests do not send to the server in all

@mydea
Copy link
Member

mydea commented Feb 15, 2023

Hello,

the way start works, is that it will look at the sample rate and determine if the session should be sampled = recorded or not.

With your code, it would never actually start recording, because the sample rate of 0 will always result in an unsampled session.

What you need to do to achieve what you want is something like this:

const replay = new Replay();

Sentry.init({
  replaysSessionSampleRate: 0, 
  replaysOnErrorSampleRate: 0,
  integrations: [replay]
});

// No need to call `stop`, as it will never be started do to sample rate being 0
// later, when you want to start, do:

const options = client.getOptions();
options.replaysSessionSampleRate = 1.0; // <-- ensure any `start` will _always_ result in a sampled session
replay.start();
// now you can stop/start it again as you see fit.

We may look into making this easier in the future - I opened an issue to track this here: #7193
But for now, this approach should work well!

@expcapitaldev
Copy link
Author

in my case I set options.replaysSessionSampleRate = 1; every time pls check

@mydea
Copy link
Member

mydea commented Feb 16, 2023

Hey @expcapitaldev,

we currently do not really optimize/explicitly support this use case. Really, the only thing we properly support are the following three cases:

  1. Start replay from the get-go, by adding it to integrations in Sentry.init()
  2. Start replay later, via client.addIntegration()
  3. Stop the replay

Stopping/starting the replay repeatedly may not work perfectly.

Could you rewrite your app this way:

Sentry.init({
  dns: myDNS,
  // set to 1 so once we add the integration, it will def. be sampled
  replaysSessionSampleRate: 1,
  replaysOnErrorSampleRate: 1,
  integrations: []; // not added here
});

 // enableReplay for example from variable from server
function toggleReplay(enableReplay){
  const client: Client | undefined = Sentry.getCurrentHub().getClient();
  if (!client) {
    return;
  }

  if (enableReplay) {
    const existingReplay = client.getIntegration(Replay);
    if (existingReplay) {
      return;
    }
    const replay = new Replay();
    client.addIntegration(replay);
  } else {
    const replay = client.getIntegration(Replay);
   replay?.stop();
  }
}

This would allow to start the replay later, and potentially stop it if needed. It would not allow re-starting the replay, but maybe that's good enough?
this would def. be considerably more robust and should work well with the current state of Replay!

@hubertdeng123
Copy link
Member

going to move this to Status: In Progress since it seems like it's being actively responded to and right now is being tracked for triage response

@jakub300
Copy link

Hi @mydea, I'm setting up Replay integration for a project and found this issue because I'm confused why start is not working.

The Session Replay documentation explicitly mentions "Manually calling the replay.start() method" as a method to start a recording.

https://github.com/getsentry/sentry-docs/blob/6006b5e0f37eaf57cfe79c3ffbd5ef7b46f3c455/src/docs/product/session-replay/index.mdx?plain=1#L17-L21

Based on the comments above this seems to not be correct?

And to be clear, would following combination be possible currently or not?:

  • start replay recording on error with sample rate 0.1 (10%)
  • start replay recording manually (ignoring sample rate)

@billyvg
Copy link
Member

billyvg commented Feb 17, 2023

@jakub300 Yeah we will have to update the docs here, thanks.

Can you explain your use-case for starting the recording manually?

@jakub300
Copy link

Hi, we're still exploring but possible use cases:

  • manually control sample rate for specific errors - for example set on error sample rate to 10% and use beforeSend to always start recording for specific issues (separate feature request here would be to make sure errors are referencing replays in such scenario)
  • use replays for purposes other than error tracking - triggering them based on user actions to better understand user behavior

@jason-hwang
Copy link
Contributor

jason-hwang commented Mar 31, 2023

@mydea

In your example, can' Client' be BrowserClient of @sentry/browser?

import { BrowserClient, Replay } from "@sentry/browser";

const client: BrowserClient | undefined = Sentry.getCurrentHub().getClient();

Additionally, the above example is working well.

@brotherko
Copy link

Hey @expcapitaldev,

we currently do not really optimize/explicitly support this use case. Really, the only thing we properly support are the following three cases:

  1. Start replay from the get-go, by adding it to integrations in Sentry.init()
  2. Start replay later, via client.addIntegration()
  3. Stop the replay

Stopping/starting the replay repeatedly may not work perfectly.

Could you rewrite your app this way:

Sentry.init({
  dns: myDNS,
  // set to 1 so once we add the integration, it will def. be sampled
  replaysSessionSampleRate: 1,
  replaysOnErrorSampleRate: 1,
  integrations: []; // not added here
});

 // enableReplay for example from variable from server
function toggleReplay(enableReplay){
  const client: Client | undefined = Sentry.getCurrentHub().getClient();
  if (!client) {
    return;
  }

  if (enableReplay) {
    const existingReplay = client.getIntegration(Replay);
    if (existingReplay) {
      return;
    }
    const replay = new Replay();
    client.addIntegration(replay);
  } else {
    const replay = client.getIntegration(Replay);
   replay?.stop();
  }
}

This would allow to start the replay later, and potentially stop it if needed. It would not allow re-starting the replay, but maybe that's good enough? this would def. be considerably more robust and should work well with the current state of Replay!

I had a similar setup to track a set of specific users. but my problem is that once the user become inactive(According to the doc it's 5mins of inactivity right?). Then even the user becomes active again. It won't restart the replay session. I'm curious if this is the expected behavior? if so how to reconfigure it such that the session restart? I'm happy with either it continues the original session or creating a new session. Thanks!!

@mydea
Copy link
Member

mydea commented Apr 11, 2023

@mydea

In your example, can' Client' be BrowserClient of @sentry/browser?

import { BrowserClient, Replay } from "@sentry/browser";

const client: BrowserClient | undefined = Sentry.getCurrentHub().getClient();

Additionally, the above example is working well.

Yes, that should be correct, BrowserClient implements the base client!

I had a similar setup to track a set of specific users. but my problem is that once the user become inactive(According to the doc it's 5mins of inactivity right?). Then even the user becomes active again. It won't restart the replay session. I'm curious if this is the expected behavior? if so how to reconfigure it such that the session restart? I'm happy with either it continues the original session or creating a new session. Thanks!!

The session should not be stopped when it becomes inactive, but paused. When activity resumes, the replay should also resume. Are you saying that for you a session is stopped when it becomes inactive for 5 minutes, and never restarted when it becomes active again? if so, could you open a new issue for this, as this would be unrelated to the start/stop topic here - this should not happen!

@billyvg
Copy link
Member

billyvg commented May 1, 2023

Hi @mydea, I'm setting up Replay integration for a project and found this issue because I'm confused why start is not working.

The Session Replay documentation explicitly mentions "Manually calling the replay.start() method" as a method to start a recording.

https://github.com/getsentry/sentry-docs/blob/6006b5e0f37eaf57cfe79c3ffbd5ef7b46f3c455/src/docs/product/session-replay/index.mdx?plain=1#L17-L21

Based on the comments above this seems to not be correct?

And to be clear, would following combination be possible currently or not?:

  • start replay recording on error with sample rate 0.1 (10%)
  • start replay recording manually (ignoring sample rate)

This should now be possible by configuring the replaysErrorSampleRate to 0.1, and then calling replay.stop() and replay.start() if you decide to start a session-based recording (which will discard any buffered events that were due to the error sample rate).

@billyvg
Copy link
Member

billyvg commented May 1, 2023

We made some fixes in 7.50.0 that should handle start()/stop() better. stop() will end the current recording + session, and start() will start a new session + recording (provided a recording is not currently in progress). Please let us know if it's working as you'd expect, thanks!

Closed by #7768 and released in 7.50.0

@billyvg billyvg closed this as completed May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants