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

On-demand flushing of offline transport #14746

Closed
joshkel opened this issue Dec 16, 2024 · 6 comments · Fixed by #14764
Closed

On-demand flushing of offline transport #14746

joshkel opened this issue Dec 16, 2024 · 6 comments · Fixed by #14764
Labels
Package: browser Issues related to the Sentry Browser SDK

Comments

@joshkel
Copy link
Contributor

joshkel commented Dec 16, 2024

Problem Statement

Our web application is designed to be used offline and only periodically connected to the Internet to sync data. We used Sentry's old, deprecated Offline integration 1 to handle Sentry events; it knew when the app was connected to the Internet, so it could sync on demand.

The new offline transport looks nice, but it apparently doesn't offer a way to flush on demand. I'm concerned that our users may not leave their apps connected to the Internet long enough for the (potentially) one hour retry delay to fire.

Solution Brainstorm

The { send; flush } object that makeOfflineTransport returns could add a flushNow method that immediately starts processing the queue.

    function flushNow(): void {
      flushIn(MIN_DELAY);
      retryDelay = START_DELAY;
    }

If this approach seems reasonable, I can open a PR.

Footnotes

  1. Technically, we used a fork of the Offline integration, where we implemented our own offline detection, due to limitations in the browser's standard feature.

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

I feel like we should just make it so that the flush we return actually flushes the offline queue alongside the transport queue.

@timfish does that feel reasonable? If so let's do this for v9 (we are working on the next major and this feels like an appropriate change for that).

@timfish
Copy link
Collaborator

timfish commented Dec 17, 2024

I feel like we should just make it so that the flush we return actually flushes the offline queue alongside the transport queue

Makes sense, although what would this mean for existing usages of flush, like in the Electron SDK where it's called before exit? Would we want to attempt to flush the offline queue before exit too? Most usages of flush will use a timeout but that would limit how much can actually be sent.

We used Sentry's old, deprecated Offline integration to handle Sentry events; it knew when the app was connected to the Internet, so it could sync on demand.

Maybe we should be adding this feature back to the latest offline transport?

Ah, just read this part:

Technically, we used a fork of the Offline integration, where we implemented our own offline detection, due to limitations in the browser's standard feature.

How did your offline detection work? Care to share the code?

@joshkel
Copy link
Contributor Author

joshkel commented Dec 17, 2024

How did your offline detection work? Care to share the code?

Sure. We had some of the same problems you mentioned in #6403:

  • Browser navigator.onLine does not offer a reliable way to determine if the machine is connected to the internet. It only tells you that the machine is connected to a network.
    • For example, on Windows, the Docker virtual network adaptor means that onLine is always true 😭

For our app in particular, it's designed to connect to non-Internet-connected IoT devices; since it has a network connection, navigator.onLine is generally true, even though there's no Internet connection.

So we made a simple interface:

export interface OfflineStatus {
  addOnlineEventListener(eventHandler: () => void): void;
  isOnline(): boolean | null;
}

We modified the Offline integration so that it defaulted to a navigator.onLine-based implementation of OfflineStatus but allowed overriding. Then we used an app-specific implementation that checks an HTTP endpoint on our back-end to determine whether it has Internet connectivity. (I can share more of the code if you're interested, but it's pretty straightforward.)

After looking at the new offline transport, I had just about decided that our approach was overly complex (or, at least, that it's too complex to justify for Sentry itself) and that Sentry's new approach of automatic retry on any error would work well, if augmented with Sentry exposing a flush or flushNow operation so individual applications could customize when needed.

Thinking about this more today, adding basic online event support to the offline transport should help us after all - changing the connection from our IoT devices to the Internet should cause the browser to temporarily see itself as offline, which could trigger sending events. So maybe it's worth both adding online event handling as well as, or instead of, adding a flush method?

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Dec 17, 2024
@timfish
Copy link
Collaborator

timfish commented Dec 17, 2024

changing the connection from our IoT devices to the Internet should cause the browser to temporarily see itself as offline, which could trigger sending events. So maybe it's worth both adding online event handling as well as, or instead of, adding a flush method?

Yes, however unreliable online status is, we should still probably retry if the state changes!

@timfish
Copy link
Collaborator

timfish commented Dec 17, 2024

@joshkel I tried to add you as a reviewer to the PR but it wouldn't let me so let me know what you think!

@joshkel
Copy link
Contributor Author

joshkel commented Dec 17, 2024

@timfish Looks great. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: browser Issues related to the Sentry Browser SDK
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants