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

Minimum time for initial flush breaks replays for non-SPA #9301

Open
billyvg opened this issue Oct 18, 2023 · 30 comments
Open

Minimum time for initial flush breaks replays for non-SPA #9301

billyvg opened this issue Oct 18, 2023 · 30 comments
Labels
Package: replay Issues related to the Sentry Replay SDK

Comments

@billyvg
Copy link
Member

billyvg commented Oct 18, 2023

For non-SPA where there is normal browser navigation: we can lose chunks of the replay if the user navigates quickly in and out of pages. This leads to broken looking replays as we do not capture the follow-up screens because they never get sent at all. This can lead to false dead clicks as well (I'm guessing due to bfcache?).

@mydea
Copy link
Member

mydea commented Oct 19, 2023

This can be configured, maybe we need to state this more explicitly in docs, but you can set new Replay({ minReplayDuration: 0 }) to basically disable this check.

Not sure how that would affect dead clicks, though, because these are only sent after the timeout (7s) passed. So you'd simply not get any dead click if moving away before 7s, I think!

@aaronjensen
Copy link

(I'm the reporter of this)

I'm may be in the minority here as we have an application that isn't a single page app, but I would suggest finding a way to make this work out of the box with what is the default behavior of a web browser and web page. Ideally without having to adjust a parameter that I may still want to have set to several seconds considering the fact that it (as far as I understand it) has a direct impact on cost.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 Oct 19, 2023
@aaronjensen
Copy link

I just tested this and it didn't actually help. Here is my Sentry.init:

Sentry.init({
  dsn: dsn,
  environment: window.Settings.environmentName,
  release: window.Settings.releaseName,

  integrations: [
    new Sentry.BrowserTracing(),
    new Sentry.Integrations.Replay({
      maskAllText: true,
      // Necessary to make this work with regular browser apps
      // https://github.com/getsentry/sentry-javascript/issues/9301
      // - Aaron, Thu Oct 19 2023
      minReplayDuration: 0
    })
  ],

  tracesSampleRate: 1.0,
  replaysSessionSampleRate: 1.0,
  replaysOnErrorSampleRate: 1.0,

  ignoreErrors: ["AbortError"],
  beforeSend: (event) => {
    if (ignoreEvent(event)) {
      return null
    }

    return event
  }
});

And here's a replay ID: 70816a3030b04ca3aa076791bbff3fdb

Am I doing anything wrong in my config?

@mydea
Copy link
Member

mydea commented Oct 20, 2023

Could you provide a link to your replay on Sentry SaaS, then I can take a look. What exactly is not working for you? Is the replay not being sent at all, or is there data missing, ...?

@aaronjensen
Copy link

@mydea i have a ticket open with support. Details are there. It’s #105109

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 Oct 20, 2023
@getsantry getsantry bot removed the status in GitHub Issues with 👀 Oct 24, 2023
@getsantry getsantry bot moved this to Waiting for: Community in GitHub Issues with 👀 Nov 6, 2023
@aaronjensen
Copy link

Is this actually waiting for community? Let me know if you need anything from me, it would be great for replays to work with Non-SPAs (people do still build these...) Thanks!

@getsantry getsantry bot moved this from Waiting for: Community to Waiting for: Product Owner in GitHub Issues with 👀 Nov 26, 2023
@bruno-garcia
Copy link
Member

Is this actually waiting for community?

This was set because the previous message was:

Could you provide a link to your replay on Sentry SaaS, then I can take a look. What exactly is not working for you? Is the replay not being sent at all, or is there data missing, ...?

@aaronjensen sorry about the confusion, we got your ticket on zendesk and I see there are links there to Sentry: https://sentry.zendesk.com/agent/tickets/105109

I looked into a recording and it seems the page isn't navigating. We'll investigate further.

it would be great for replays to work with Non-SPAs (people do still build these...)

We totally understand this. And it's in our plan to improve this overall story, through:

In addition we're aware of some false positives even on SPA's, but there were improvements in the version https://github.com/getsentry/sentry-javascript/releases/tag/7.78.0 where we changed the detection through:

@aaronjensen
Copy link

Sounds good, looking forward to it, thank you.

@bruno-garcia
Copy link
Member

Thank you for your patience and understanding!

@bruno-garcia
Copy link
Member

I just validated the release 7.81.0 fixed a Dead click false positive here:

Could you please try this one version out (or latest of course).

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Jun 25, 2024
@mydea
Copy link
Member

mydea commented Jun 26, 2024

thanks for the update! Did you also add this:

window.addEventListener("beforeunload", () => {
  Sentry.getReplay()?.flush();
});

This may help with capturing additional data. We will look into this some more, it is sadly where hard to debug this because in all our tests (which are more on the simple side, of course...) stuff works reasonably well - but I can totally see how there may be a bunch of not-uncommon edge cases where stuff may break 😬

@aaronjensen
Copy link

@mydea There is no getReplay on Sentry, at least when we use the CDN version.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Jun 26, 2024
@aaronjensen
Copy link

aaronjensen commented Jun 26, 2024

I was able to use the instance created by replayIntegration:

const replay =
  Sentry.replayIntegration({
    maskAllText: true,
    // Necessary to make this work with regular browser apps
    // https://github.com/getsentry/sentry-javascript/issues/9301
    // - Aaron, Thu Oct 19 2023
    minReplayDuration: 0,
    flushMinDelay: 200,
    flushMaxDelay: 250
  })

Sentry.init({
  dsn: dsn,

  integrations: [
    Sentry.browserTracingIntegration({
      enableInp: true
    }),
    replay
  ],
});

// https://github.com/getsentry/sentry-javascript/issues/9301#issuecomment-2190978271
window.addEventListener("beforeunload", () => {
  replay.flush();
});

@aaronjensen
Copy link

aaronjensen commented Jun 26, 2024

Unfortunately, it does not work even in my basic tests locally. Here is an example: b85b47e18ef648eb97348d1f6ae365d7

Another one: 5dba4965a50c4a4bbf577384b8c550c1

I'm confounded as to how you are unable to reproduce this. It does not work when you click a link that goes to another page and then hit the back button. It shows up as a dead click. My individual requests show up in the "Trace" tab, but the web page does not show up in the replay.

@billyvg
Copy link
Member Author

billyvg commented Jun 26, 2024

I think the dead clicks are caused by the bfcache, i've created a ticket to track this separately.

@aaronjensen can you add the following snippet and show us the console logs that get printed? (you may need to enable Preserve log in the Console settings)

window.addEventListener('beforeunload', (event) => {
  const size = replay._replay.eventBuffer._used._totalSize;
  console.log('beforeunload: ', size);
  replay.flush().then(() => {
    console.log('beforeunload flushed');
  });
});

window.addEventListener('pagehide', (event) => {
  const size = replay._replay.eventBuffer._used._totalSize;
  console.log('pagehide: ', size, event.persisted);
  replay.flush().then(() => {
    console.log('pagehide flushed');
  });
});

@aaronjensen
Copy link

CleanShot 2024-06-26 at 19 34 27@2x

beforeunload doesn't flush when clicking a link, but it seems to when hitting back.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Jun 26, 2024
@mydea
Copy link
Member

mydea commented Jun 28, 2024

I think the absence of the log does not necessarily mean it does not flush, it depends if the flushing http request is finished in the background 🤔 which it in general should, but apparently, it is not (always?) doing.

We need to investigate some more, I am not sure what we may do in addition to having an beforeunload handler, to ensure no data is lost 🤔

@aaronjensen
Copy link

The things I would investigate are: put the data in session storage in beforeunload in case it’s not able to be flushed and load it on the next page load. Alternatively, it may be that offloading it to a service worker would work.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Jun 28, 2024
@getsantry getsantry bot removed the status in GitHub Issues with 👀 3 Jul 1, 2024
@AbhiPrasad AbhiPrasad added the Package: replay Issues related to the Sentry Replay SDK label Nov 1, 2024
@sidpremkumar
Copy link

sidpremkumar commented Dec 31, 2024

Is there any update here?

I can confirm that adding:

minReplayDuration: 0,
flushMinDelay: 200
flushMaxDelay: 250

does help, but it increases the network requests 10x which is unideal.

Also I've tried adding

        // Use synchronous flushing and handle navigation
        window.addEventListener('beforeunload', (event) => {
          // Synchronously flush
          try {
            Sentry.getReplay()?.flush();
                    console.log("Flushed beforeunload")
                  
          } catch (e) {
            console.error('Flush failed:', e);
          }
         
        });

      

        // For modern browsers, use the 'visibilitychange' event
        document.addEventListener('visibilitychange', () => {
            if (document.visibilityState === 'hidden') {
            // Use the Beacon API for more reliable delivery
           Sentry.getReplay()?.flush();
              console.log("Flushed visabilitychange")
            }
        });

But as mentioned above this also does not work 100% (honestly not sure this changed much at all).

I believe this is due to multi-page website, as single page applications like react have no issue here. It's almost always caused during page changes.

Happy to share anything else! I agree with @aaronjensen approach above to save the data to local storage somehow and send anything that hasn't been sent yet on page load.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Dec 31, 2024
@Lms24
Copy link
Member

Lms24 commented Jan 2, 2025

No update from our end at this point (cc @billyvg just in case you looked into this). We'll post updates here once we get to it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: replay Issues related to the Sentry Replay SDK
Projects
Status: No status
Status: No status
Development

No branches or pull requests

9 participants