feat: Added Offline storage support to Event processor for React Native Apps #517

zashraf1985 · 2020-07-03T16:44:10Z

Summary

Added Offline storage support to Event Processor for React Native Apps.

Stores events when device is offline and then redispatches when the device is online again, new event is ready to be dispatched or SDK is re-initialized.
Adds a new eventMaxQueueSize config option which represents number of offline events SDK will store at a time. Deault is 10k for now.
Adds a connectivity listener to redispatch events when device is back online.

Follow-up changes expected in optimizely-sdk package.

Add a new option to EventProcessor constructor to represent maxQueueSize in React Native entry point.
Change callback parameter to http status in DefaultEventDispacher for browser entry point which is also used for React Native entry point.
Add AsyncStorage module configuration for umd bundles in rollup config.

Test plan

Wrote new unit tests
Tested offline with FSC suite to make sure it does not break existing event processor functionality.
Tested sequence of redispatching events using a local mock server and timestamps.

coveralls · 2020-07-03T16:57:06Z

Coverage remained the same at 96.709% when pulling dd7c837 on zeeshan/rn-event-processor-proto into 8ca4242 on master.

mjc1283 · 2020-07-08T17:04:58Z

packages/event-processor/src/index.ts

Why change the exports like this?

Because i exported both RN and normal event processor from the single file. i will probably separate them out.

mjc1283 · 2020-07-08T17:08:26Z

packages/event-processor/src/eventDispatcher.ts

-export type EventDispatcherResponse = {
-  statusCode: number
-} | {
-  status: number
-}


The Node event dispatcher calls the ED callback with an object like this ({ statusCode: 200 }). Is that a problem? Why change this to accept a number?

I think i changed it initially to try something quickly and then forgot to revert back. i will reverts it

mjc1283 · 2020-07-08T17:29:05Z

packages/event-processor/src/reactNativeEventsStore.ts

+// This results in race condition when two items are added to the map or array in parallel.
+// for ex. Req 1 gets the map. Req 2 gets the map. Req 1 sets the map. Req 2 sets the map. The map now loses item from Req 1.
+// This synchronizer makes sure the operations are atomic using promises.
+class Synchronizer {


This Synchronizer is similar to the RequestTracker. Can we use RequestTracker instead of adding this? We can consider renaming RequestTracker to something more generic.

mjc1283 · 2020-07-09T16:33:36Z

packages/event-processor/src/reactNativeEventsStore.ts

+// This stores individual events generated from the SDK till they are part of the pending buffer.
+// The store is cleared right before the event is formatted to be dispatched.
+// This is to make sure that individual events are not lost when app closes before the buffer was flushed.
+export class ReactNativeEventBufferStore {


Why do we need two different classes ReactNativePendingEventStore and ReactNativeEventBufferStore? They seem very similar. Also I think they should both have a limit on how much they will store.

mjc1283 · 2020-07-09T16:44:12Z

packages/event-processor/src/v1/v1EventProcessor.ts

@@ -27,3 +28,13 @@ export class LogTierV1EventProcessor extends AbstractEventProcessor {
    }
  }
 }
+
+export class LogTierV1ReactNativeEventProcessor extends AbstractReactNativeEventProcessor {


I don't think we can put this here. This causes the RN code to be included in the main bundle.

mjc1283 · 2020-07-09T18:26:06Z

packages/event-processor/src/reactNativeEventProcessor.ts

+      await this.pendingEventsStore.set(cacheKey, formattedEvent)
+
+      // Clear buffer because the buffer has become a formatted event and is already stored in pending cache.
+      this.eventBufferStore.clear()


Do we need to await the completion of this clear?

mjc1283 · 2020-07-09T18:27:25Z

packages/event-processor/src/reactNativeEventProcessor.ts

+  private isProcessingPendingEvents: boolean = false
+  private pendingEventsPromise: Promise<void> | null = null


Do we need isProcessingPendingEvents? Can we say if pendingEventsPromise is not null, then it is processing pending events?

mjc1283 · 2020-07-09T18:54:21Z

packages/event-processor/src/reactNativeEventProcessor.ts

+    this.pendingEventsPromise = new Promise(async (resolvePendingEventPromise) => {
+      this.isProcessingPendingEvents = true
+      const formattedEvents: {[key: string]: any} = await this.pendingEventsStore.getEventsMap()
+      const eventKeys = Object.keys(formattedEvents)


Suggest: use utils.objectEntries for iteration here rather than for loop.

mjc1283 · 2020-07-09T18:59:37Z

packages/event-processor/src/reactNativeEventProcessor.ts

+    if (this.isProcessingPendingEvents && this.pendingEventsPromise) {
+      return this.pendingEventsPromise
+    }
+    this.pendingEventsPromise = new Promise(async (resolvePendingEventPromise) => {


It's hard to follow what's going on with two nested Promise constructors, event dispatcher callbacks, the loop continue, etc. I would try refactoring this to use async/await as much as possible. To deal with the callback-based event dispatcher, create a helper async function that hides the complexity away from the main logic of processPendingEvents.

mjc1283 · 2020-07-09T19:00:40Z

packages/event-processor/src/reactNativeEventProcessor.ts

+      const eventKeys = Object.keys(formattedEvents)
+      for (let i = 0; i < eventKeys.length; i++) {
+        const eventKey = eventKeys[i]
+        if (this.eventsInProgress[eventKey]) {


Why do we need this check? How does the same event get dispatched twice?

2. moved connectivity listener to start

…ent processor implementation

mjc1283

LGTM. I added a few suggestions which you can consider (non-blocking). Please have @jaeopt approve also.

mjc1283 · 2020-07-13T22:06:16Z

packages/event-processor/src/v1/v1EventProcessor.react_native.ts

+/**
+ * React Native Events Processor with Caching support for events when app is offline.
+ */
+export abstract class LogTierV1EventProcessor implements EventProcessor {


Can we name it ReactNativeEventProcessor? Or something with React Native in the name to differentiate from the standard one.

packages/event-processor/src/v1/v1EventProcessor.react_native.ts

mjc1283 · 2020-07-13T22:16:30Z

packages/event-processor/src/v1/v1EventProcessor.react_native.ts

+
+  process(event: ProcessableEvent): void {
+    // Adding events to buffer store. If app closes before dispatch, we can reprocess next time the app initializes
+    this.eventBufferStore.set(generateUUID(), event)


It is awkward that we have to generate the UUID to use as a key here, but we don't retrieve it using this key, we just get all of them by calling getEventsList. Consider changing the interface so you can set/append without passing a key.

Co-authored-by: Matt Carroll <mjc1283@users.noreply.github.com>

jaeopt

Not sure about the implications, but I see a few places of out-of-order event dispatch. Check it out.
A couple of other suggestions too.

jaeopt · 2020-07-14T20:35:56Z

packages/event-processor/src/reactNativeEventsStore.ts

+    await this.synchronizer.getLock()
+    const eventsMap: {[key: string]: T} = await this.cache.get(this.storeKey) || {}
+    this.synchronizer.releaseLock()
+    return objectValues(eventsMap)


Does this list guarantee FIFO ordering from map?

Yes. Looping through map always iterates in the order of insertion.

jaeopt · 2020-07-14T21:30:45Z

packages/event-processor/src/v1/v1EventProcessor.react_native.ts

+
+  async drainQueue(buffer: ProcessableEvent[]): Promise<void> {
+    // Retry pending failed events while draining queue
+    await this.processPendingEvents()


Not sure about the implication of this out-of-order dispatching, but if "processPendingEvents" fail to flush all pending events, the new dispatch below (line 147) can be fired before previous buffered events. Check it out.

fixed. skipping subsequent events on failure now.

jaeopt · 2020-07-14T21:33:27Z

packages/event-processor/src/v1/v1EventProcessor.react_native.ts

+    for (const [eventKey, event] of eventEntries) {
+      await this.dispatchEvent(eventKey, event)
+    }


Another case of out-of-order delivery. If some events failed with errors, they stay in the buffer for retry later (out-of-order) since other events buffered later can be dispatched ok.

jaeopt · 2020-07-14T21:42:49Z

packages/event-processor/src/v1/v1EventProcessor.react_native.ts

+  constructor({
+    dispatcher,
+    flushInterval = 30000,
+    batchSize = 3000,


Check out this huge batch size default value. I understand we have max batched event size. 3000 events will be batched to be > 3MB. Not sure if this fits in the restrictions.
I think default batch size can be much smaller like 10-100.

Good point. I actually borrowed the value from the existing event processor. @mjc1283 ! Do you suggest to change it to something else.

actually @jaeopt you are correct. this particular default never gets used by optimizely SDK, the sdk package already sets default of 10 if the user does not provide a value. This default will only take effect if someone uses event processor package directly. I will change it to 10 to make it consistent with SDK default.

…omplete. might add more tests to the list

jaeopt

LGTM

zashraf1985 added 2 commits July 2, 2020 17:08

first implementation of offline events. Need to do code cleanup

091aaf2

added new eventMaxQueueSize configuration option.

8076ef4

optimizely-codereview-bot assigned zashraf1985 Jul 3, 2020

zashraf1985 added 4 commits July 3, 2020 13:45

some cleanup and refactor

6d338d2

added some comments

ab29e0e

added connectivity listener

cbb6aef

fixed existing unit tests

b26d407

zashraf1985 marked this pull request as ready for review July 6, 2020 07:19

zashraf1985 requested a review from a team as a code owner July 6, 2020 07:19

zashraf1985 removed their assignment Jul 6, 2020

zashraf1985 requested a review from jaeopt July 6, 2020 18:13

Merge branch 'master' into zeeshan/rn-event-processor-proto

edbfaba

optimizely-codereview-bot added 🤖 4 hrs 🤖 8 hrs and removed 🤖 4 hrs labels Jul 6, 2020

zashraf1985 mentioned this pull request Jul 7, 2020

refactor: Made PendingEventsStore interface asynchronous #513

Closed

optimizely-codereview-bot added 🤖 12 hrs and removed 🤖 8 hrs labels Jul 7, 2020

zashraf1985 added 2 commits July 7, 2020 19:46

sequences the pending events one after the other

0621882

Sequenced the buffered events after pending events on the start

e94ebdc

optimizely-codereview-bot added 🤖 16 hrs 🤖 20 hrs and removed 🤖 12 hrs 🤖 16 hrs labels Jul 8, 2020

zashraf1985 added 2 commits July 8, 2020 17:29

added some extra checks to fix the behavior of stop

35b5d4f

modified the resolvable promise to make it work with jest

7db47a6

optimizely-codereview-bot added 🤖 24 hrs and removed 🤖 20 hrs labels Jul 9, 2020

mjc1283 suggested changes Jul 9, 2020

View reviewed changes

optimizely-codereview-bot assigned zashraf1985 Jul 9, 2020

zashraf1985 added 8 commits July 9, 2020 13:56

1. added await while clearing buffer store

d99fb66

2. moved connectivity listener to start

re factored the hierarchy of event processor classes

9918024

refactored how pending events are dispatched. simplified the whole ev…

8018de0

…ent processor implementation

removed localhost url

134c5df

fixed existing unit tests

8467a91

renamed file to reflect its react native version of event processor

a49a6fa

simplified synchronizer

a19f66f

using only one store for both pending events and events buffer

bb7024e

zashraf1985 removed their assignment Jul 13, 2020

optimizely-codereview-bot requested a review from mjc1283 July 13, 2020 20:58

mjc1283 approved these changes Jul 13, 2020

View reviewed changes

zashraf1985 and others added 3 commits July 13, 2020 15:26

Update packages/event-processor/src/v1/v1EventProcessor.react_native.ts

01ef03e

Co-authored-by: Matt Carroll <mjc1283@users.noreply.github.com>

added some logs

3411d52

Added unit tests for React Native store

108fea6

jaeopt requested changes Jul 14, 2020

View reviewed changes

optimizely-codereview-bot assigned zashraf1985 Jul 14, 2020

incorporated review feedback from jae and owais

8dadf85

zashraf1985 removed their assignment Jul 16, 2020

optimizely-codereview-bot requested a review from jaeopt July 16, 2020 01:49

Added skeleton for react native event processor unit tests. Its not c…

b5267fb

…omplete. might add more tests to the list

optimizely-codereview-bot added the 🤖 4 hrs label Jul 16, 2020

jaeopt approved these changes Jul 17, 2020

View reviewed changes

zashraf1985 added 5 commits July 16, 2020 19:12

moved cache code from utils to this repo to resolve dependency issues

6c3c40d

added unit tests and fixed some issues found during unit testing

a028ffd

added more test and fixed an issue with buffer store

b92d95d

added more tests

2b1594d

reduced timeouts to make tests faster

dd7c837

mjc1283 merged commit d2208ff into master Jul 21, 2020

mjc1283 deleted the zeeshan/rn-event-processor-proto branch July 21, 2020 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Added Offline storage support to Event processor for React Native Apps #517

feat: Added Offline storage support to Event processor for React Native Apps #517

zashraf1985 commented Jul 3, 2020 •

edited

Loading

coveralls commented Jul 3, 2020 •

edited

Loading

mjc1283 Jul 8, 2020

zashraf1985 Jul 9, 2020

mjc1283 Jul 8, 2020

zashraf1985 Jul 9, 2020

mjc1283 Jul 8, 2020

mjc1283 Jul 9, 2020

mjc1283 Jul 9, 2020

mjc1283 Jul 9, 2020

mjc1283 Jul 9, 2020

mjc1283 Jul 9, 2020

mjc1283 Jul 9, 2020

mjc1283 Jul 9, 2020

mjc1283 left a comment

mjc1283 Jul 13, 2020

mjc1283 Jul 13, 2020

jaeopt left a comment

jaeopt Jul 14, 2020

zashraf1985 Jul 14, 2020

jaeopt Jul 14, 2020

zashraf1985 Jul 16, 2020

jaeopt Jul 14, 2020

jaeopt Jul 14, 2020

zashraf1985 Jul 14, 2020

zashraf1985 Jul 16, 2020

jaeopt left a comment

		private isProcessingPendingEvents: boolean = false
		private pendingEventsPromise: Promise<void> \| null = null

feat: Added Offline storage support to Event processor for React Native Apps #517

feat: Added Offline storage support to Event processor for React Native Apps #517

Conversation

zashraf1985 commented Jul 3, 2020 • edited Loading

Summary

Test plan

coveralls commented Jul 3, 2020 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjc1283 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaeopt 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

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

jaeopt left a comment

Choose a reason for hiding this comment

zashraf1985 commented Jul 3, 2020 •

edited

Loading

coveralls commented Jul 3, 2020 •

edited

Loading