Add integration for offline support #2778

davidmyersdev · 2020-07-28T03:39:07Z

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

If you've added code that should be tested, please add tests.
Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

Summary

Cache offline events to send when connectivity is reestablished.

This PR is an adaptation of #2216. Resolves #1633.

Offline events are cached in the browser with localforage.

davidmyersdev · 2020-07-29T02:24:12Z

Okay, I think I have this in a good place now, but I still need to write some tests. The final failing check is related to bundle size, so I'm not entirely sure how to proceed. I assume this is from the addition of the localforage dependency.

AbhiPrasad · 2020-07-29T14:31:54Z

This looks good at an initial glance, but before we do a deeper review, @voraciousdev do you mind if you could move this integration into our integrations package? I think this is the best place for features like this.

https://github.com/getsentry/sentry-javascript/tree/master/packages/integrations.

Also do you have a demo repo or something of the sort you've been testing this with? Would help with review/testing on our side (also to explore all the use cases).

Thanks for taking this on, the Sentry team appreciates it ❤️

As an aside it seems that the bundle check is failing because you are trying to merge from a fork? Don't think it is your problem, if you look all the size checks still pass.

Error creating comment. This can happen for PR's originating from a fork without write permissions.

We will investigate this on our side.

I thought this build was passing, but it looks like there is not an easy way to use a standard import for localforage in this library.

davidmyersdev · 2020-07-30T13:41:35Z

@AbhiPrasad I appreciate the feedback! I believe I moved this to the correct place now. The repo I've been using to test this is closed source, but I'll see if I can throw an example together today.

AbhiPrasad

Some more thoughts I had.

Once you get around to the tests, I'll do a more involved review (and clone the branch locally for some local testing).

packages/integrations/package.json

packages/integrations/src/offline.ts

AbhiPrasad · 2020-07-30T16:07:53Z

packages/integrations/src/offline.ts

+   * @inheritDoc
+   */
+  public constructor() {
+    this.offlineEventStore = localforage.createInstance({


Could createInstance fail? What happens to this.offlineEventStore then?

We can import localforage typings if they exist as well.

Also, what browsers does localforage support? It's fine if it doesn't match Sentry's (https://docs.sentry.io/platforms/javascript/#browser-table) - we just have to document it then.

Ah, good catch. I meant to wrap this in a try/catch. I will look into importing the type file too.

I think it meets the compatibility requirements. It is a wrapper around multiple storage mechanisms. It attempts to use IndexedDB, then falls back to WebSQL, and if all else fails, it falls back to localStorage.
https://github.com/localForage/localForage/wiki/Supported-Browsers-Platforms

Nice ok, makes sense. Just one last thing, what happens when local storage is full? Is it a queue, or do new events just get dropped?

@bruno-garcia, how do we handle offline queue in mobile right now?

Only envelopes are cached (no event json alone are cached anymore is what I mean). The order they were cached is kept when attempting to send things to Sentry (i.e device is back online). If maxItems is reached (by default 30 IIRC), the oldest file is deleted to make space to the newest one.

We don't use dependencies or a DB of any sort. It's just files written to a directory.

If session data is stored (release health), the init=true flag must be preserved. So when deleting a file to make room for a new one (max items reached), the envelope would need to be unwrapped, checked for sessions and if init=true exists, that must be moved to the next session update queued up for submission (from oldest to newest).
Events capture when session tracking is on must be in an envelope together with a session update.

Note that the order being kept means that if you get a call to captureException, you won't be sending that to Sentry if there's stuff cached. You must prioritize what's on the storage first.

Thanks Bruno for coming through 🚀. We can ignore all the sessions stuff, and envelope details - although important for me, probably not as for you @voraciousdev

Basically there are two things we need to keep in mind.

Enforce some kind of maxItems in local storage. This can be changed through an option.

Make sure that we send all events from local storage first before we continue on with other events. I know we are using an event listener right now to listen to online, do you think that is good enough? Maybe we need another check in the globalEventProcessor to be sure.

As for how this will work with sessions, we can get to that when release health comes to JS 😄

@AbhiPrasad the LocalForage.setItem promise will be rejected if there is not enough space on the device. With the way it's currently written in this integration, that would result in logger.warn('could not cache event while offline') being called.

Ok perfect we should be good then.

Make sure that we send all events from local storage first before we continue on with other events. I know we are using an event listener right now to listen to online, do you think that is good enough? Maybe we need another check in the globalEventProcessor to be sure.

@AbhiPrasad I'm not sure if this is directed at me, but there is definitely a possible race condition here regarding the online event. Given the nature of this integration (to support offline-capable apps), I think it's a real scenario that we could have real error events triggered as soon as the online event is fired. For offline apps, it's common practice to listen to the online event and run some code (which could then lead to an error being thrown/recorded).

Just added localforage typings as well. Good call on that. 👍

davidmyersdev · 2020-08-03T01:12:21Z

@AbhiPrasad I believe I have addressed all open comments, and I just added tests. Mind taking another look and letting me know if there's anything else you need me to change? Thanks!

AbhiPrasad

Nice LGTM, thanks for sticking with it. Just before approval, I would like to get a double check from @HazAT to see if there is anything else left (also to see if this might cause any issues with rate limiting we have to concern ourselves with). Once he gives to OK, I will approve and merge.

Also, I thought about the race condition a little bit more, but I think it's still fine to listen on online. We can come back to this if many people report a problem.

…sion

davidmyersdev · 2020-08-04T14:21:18Z

@AbhiPrasad I did some more testing in my own project, and I discovered a couple of small issues that I believe are now fixed via the extra commits you see here. To break them down:

Fixing lockfile after migrating to @sentry/integrations
- It turns out I had some lockfile changes that were committed when this was initially added to the @sentry/browser package.
Add TS compiler flag to allow import of localforage
- The import assignment was not working properly in my es6/webpack project. It looks like some of the builds were failing. After some more debugging, it looks like using a standard import (e.g. import localforage from 'localforage') actually does work. It just throws a typescript error due to the atypical nature of the package exports.
Handle scenario where app has stored events from previous offline session
- This is just a new scenario that I didn't previously think about. When an app is offline, just listening to the online event is not good enough. If the user navigates away from the app while offline and then navigates back while online, the online event would never be fired. Because of this, we will want to check for connectivity during the setupOnce call and send any previously stored events.

HazAT

First of all thanks for this, this is awesome a lot of users already requested this 🥇
Generally, it looks good.

I have one additional ask, please add an option for max stored offline events. I see that localForage has a limit but I would like to have more control over this.

So for example, maxStoredEvents: 30 and after we reach more than 30, it acts like a ring buffer dropping the oldest events and replaces it with new ones coming in.

We had troubles with other SDKs already where this was more or less unbound so we need to add a reasonable limit by default.

davidmyersdev · 2020-08-11T16:29:03Z

@HazAT thank you for following up! I can definitely manage that. I'll tag you for another review once it's ready.

davidmyersdev · 2020-08-22T17:56:15Z

@HazAT I finally got around to adding the maxStoredEvents config (with a default limit of 30 as suggested). Let me know if there's anything else you need from me!

HazAT

Hey @voraciousdev thanks for keeping the ball rolling, aside from the merge conflicts I think we are good to go :)

packages/integrations/package.json

davidmyersdev · 2020-08-24T14:20:09Z

@HazAT it looks like there were some lint rules that changed since I started this. I will get it updated.

davidmyersdev · 2020-08-24T16:14:06Z

I don't usually like merge commits, but the GitHub UI created one automatically when I resolved those conflicts. Let me know if you would prefer a rebase.

HazAT

We squash commit anyway :)
Thanks @voraciousdev contribution of the year 🥇

davidmyersdev · 2020-08-26T13:58:46Z

@HazAT any idea when this will be released?

kamilogorek · 2020-08-26T14:56:14Z

@voraciousdev tomorrow :)

piotr-cz · 2020-09-09T14:42:01Z

packages/integrations/src/offline.ts

+        if (this.hub) {
+          const newEventId = this.hub.captureEvent(event);
+
+          if (newEventId) {


The Hub.captureEvent method always returns new event ID, so event will always be purged from event store (even that client failed to send it).

Fixed in #2890

piotr-cz · 2020-09-09T14:51:24Z

packages/integrations/src/offline.ts

+      name: 'sentry/offlineEventStore',
+    });
+
+    if ('addEventListener' in this.global) {


Event listener should be set up after hub is set on instance (in setupOnce method) as event may be fired before hub is set (hub is used in _sendEvents).

IMHO it's bad practice to have business logic in constructor.

Fixed in #2889

* Add documentation for Offline integration See getsentry/sentry-javascript#2778 Remove backticks from plugin.mdx's description; PageGrid doesn't render them as monospace, so they appear as literals in https://docs.sentry.io/platforms/javascript/configuration/integrations/, and the default.mdx description doesn't use them for its class names. * Apply suggestions from code review Co-authored-by: Isabel <76437239+imatwawana@users.noreply.github.com> Co-authored-by: Isabel <76437239+imatwawana@users.noreply.github.com>

davidmyersdev requested a review from kamilogorek as a code owner July 28, 2020 03:39

davidmyersdev force-pushed the add-offline-support branch 3 times, most recently from 18c37fb to b0d0260 Compare July 28, 2020 04:09

davidmyersdev marked this pull request as draft July 28, 2020 15:07

davidmyersdev changed the title ~~WIP: Add integration for offline support~~ Add integration for offline support Jul 28, 2020

davidmyersdev force-pushed the add-offline-support branch from b0d0260 to 0fbf345 Compare July 29, 2020 00:56

Add integration for offline support

378c344

Offline events are cached in the browser with localforage.

davidmyersdev force-pushed the add-offline-support branch from 0fbf345 to 378c344 Compare July 29, 2020 01:11

Add logging statements

5ac1d5f

AbhiPrasad self-requested a review July 29, 2020 14:32

davidmyersdev added 2 commits July 29, 2020 22:41

Move 'offline' to the integrations package

fc6462b

Revert to import assignment

26dd7b0

I thought this build was passing, but it looks like there is not an easy way to use a standard import for localforage in this library.

AbhiPrasad reviewed Jul 30, 2020

View reviewed changes

davidmyersdev added 5 commits July 30, 2020 13:17

Use Hub.captureEvent instead of adding @sentry/minimal

b38a7af

Use localforage types

f122fb5

Remove async/await syntax to keep bundle size down

47cbc4c

Check for addEventListener in global

1df5a02

Add tests for Offline integration

de32dc3

davidmyersdev marked this pull request as ready for review August 3, 2020 01:17

AbhiPrasad reviewed Aug 3, 2020

View reviewed changes

AbhiPrasad requested a review from HazAT August 3, 2020 19:56

davidmyersdev added 2 commits August 3, 2020 23:09

Fixing lockfile after migrating to @sentry/integrations

3146924

Add TS compiler flag to allow import of localforage

efbee73

davidmyersdev force-pushed the add-offline-support branch from 10fd6b2 to efbee73 Compare August 4, 2020 03:37

Handle scenario where app has stored events from previous offline ses…

d758d22

…sion

davidmyersdev requested a review from AbhiPrasad August 7, 2020 21:32

HazAT mentioned this pull request Aug 11, 2020

feat: Offline Integration #2216

Closed

HazAT requested changes Aug 11, 2020

View reviewed changes

Add config for maxStoredEvents with a default limit of 30

bd3ce31

davidmyersdev requested a review from HazAT August 22, 2020 17:54

davidmyersdev mentioned this pull request Aug 24, 2020

Report offline Sentry errors davidmyersdev/octo#13

Closed

HazAT requested changes Aug 24, 2020

View reviewed changes

packages/integrations/package.json Outdated Show resolved Hide resolved

Merge branch 'master' into add-offline-support

ac720e2

Address new linter rules

83e4f07

davidmyersdev requested a review from HazAT August 24, 2020 16:27

HazAT approved these changes Aug 25, 2020

View reviewed changes

HazAT merged commit 8fbffe2 into getsentry:master Aug 25, 2020

nwaughachukwuma mentioned this pull request Aug 31, 2020

Uncaught TypeError: o.Offline is not a constructor #2868

Closed

7 tasks

jstewmon mentioned this pull request Sep 2, 2020

offline integration requires typescript dom lib in integration package #2877

Closed

piotr-cz reviewed Sep 9, 2020

View reviewed changes

joshkel mentioned this pull request Mar 4, 2021

Add documentation for Offline integration getsentry/sentry-docs#3191

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add integration for offline support #2778

Add integration for offline support #2778

davidmyersdev commented Jul 28, 2020 •

edited

Loading

davidmyersdev commented Jul 29, 2020

AbhiPrasad commented Jul 29, 2020 •

edited

Loading

davidmyersdev commented Jul 30, 2020

AbhiPrasad left a comment

AbhiPrasad Jul 30, 2020

davidmyersdev Jul 30, 2020

davidmyersdev Jul 30, 2020

AbhiPrasad Jul 30, 2020

bruno-garcia Jul 30, 2020 •

edited

Loading

AbhiPrasad Jul 30, 2020

davidmyersdev Jul 30, 2020 •

edited

Loading

AbhiPrasad Jul 30, 2020 •

edited

Loading

davidmyersdev Jul 30, 2020

davidmyersdev Jul 30, 2020

davidmyersdev commented Aug 3, 2020

AbhiPrasad left a comment

davidmyersdev commented Aug 4, 2020

HazAT left a comment

davidmyersdev commented Aug 11, 2020

davidmyersdev commented Aug 22, 2020

HazAT left a comment

davidmyersdev commented Aug 24, 2020

davidmyersdev commented Aug 24, 2020

HazAT left a comment

davidmyersdev commented Aug 26, 2020

kamilogorek commented Aug 26, 2020

piotr-cz Sep 9, 2020

piotr-cz Sep 10, 2020

piotr-cz Sep 9, 2020

piotr-cz Sep 10, 2020

Add integration for offline support #2778

Add integration for offline support #2778

Conversation

davidmyersdev commented Jul 28, 2020 • edited Loading

Summary

davidmyersdev commented Jul 29, 2020

AbhiPrasad commented Jul 29, 2020 • edited Loading

davidmyersdev commented Jul 30, 2020

AbhiPrasad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruno-garcia Jul 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidmyersdev Jul 30, 2020 • edited Loading

Choose a reason for hiding this comment

AbhiPrasad Jul 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidmyersdev commented Aug 3, 2020

AbhiPrasad left a comment

Choose a reason for hiding this comment

davidmyersdev commented Aug 4, 2020

HazAT left a comment

Choose a reason for hiding this comment

davidmyersdev commented Aug 11, 2020

davidmyersdev commented Aug 22, 2020

HazAT left a comment

Choose a reason for hiding this comment

davidmyersdev commented Aug 24, 2020

davidmyersdev commented Aug 24, 2020

HazAT left a comment

Choose a reason for hiding this comment

davidmyersdev commented Aug 26, 2020

kamilogorek commented Aug 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidmyersdev commented Jul 28, 2020 •

edited

Loading

AbhiPrasad commented Jul 29, 2020 •

edited

Loading

bruno-garcia Jul 30, 2020 •

edited

Loading

davidmyersdev Jul 30, 2020 •

edited

Loading

AbhiPrasad Jul 30, 2020 •

edited

Loading