feat: Added ODPEventManager implementation #789

mikechu-optimizely · 2022-09-30T15:22:57Z

Summary

Added OdpEventManager implementation.

Test plan

Unit tests added
Existing tests continue to pass

Jira

FSSDK-8443

needs tests and REST API sendEvents()

# Conflicts: # packages/optimizely-sdk/lib/plugins/odp/odp_client.ts # packages/optimizely-sdk/lib/plugins/odp/odp_event.ts # packages/optimizely-sdk/lib/utils/enums/index.ts

coveralls · 2022-09-30T15:26:56Z

Coverage decreased (-0.003%) to 93.463% when pulling 346da4e on mike/odp-event-manager into 8bc8ef5 on master.

mikechu-optimizely · 2022-09-30T15:37:20Z

Looks like there are older unit tests (around tests/v1EventProcessor.react_native.spec.ts) that are now failing.

Fortunately, this also matches what's going on on my local dev machine. I'll try correcting these unit tests and create a predecessor PR that we can merge before this PR.

I still think code reviews for Event Manager here can continue.

mikechu-optimizely · 2022-10-03T15:09:20Z

This is ready for review @jaeopt & @zashraf1985 whenever you're available. I'll appreciate your thorough review.

packages/optimizely-sdk/lib/plugins/odp/odp_config.ts

packages/optimizely-sdk/lib/plugins/odp/odp_event_dispatcher.ts

packages/optimizely-sdk/lib/plugins/odp/odp_event_manager.ts

Co-authored-by: Zeeshan Ashraf <35262377+zashraf1985@users.noreply.github.com>

Co-authored-by: Jae Kim <45045038+jaeopt@users.noreply.github.com>

mikechu-optimizely · 2022-10-11T12:26:47Z

@zashraf1985, Thanks again for pair programming the round of reviews. I'm ready for another look by you and @jaeopt when you have a moment.

jaeopt

Changes looks good. I see a possible failure in batching and a couple of suggestions to consider.

packages/optimizely-sdk/lib/plugins/odp/odp_config.ts

packages/optimizely-sdk/lib/plugins/odp/odp_event_dispatcher.ts

packages/optimizely-sdk/lib/plugins/odp/odp_event_manager.ts

packages/optimizely-sdk/tests/odpEventManager.spec.ts

packages/optimizely-sdk/lib/plugins/odp/odp_event_manager.ts

zashraf1985

The overall logic of event queue looks very good. I just have a concern in one asynchronous scenario that we can discuss on call. Also suggested a few minor changes here and there.

packages/optimizely-sdk/lib/plugins/odp/odp_event_dispatcher.ts

packages/optimizely-sdk/lib/plugins/odp/odp_event_manager.ts

mikechu-optimizely · 2022-10-12T12:25:40Z

Good morning @jaeopt & @zashraf1985. You'll never guess, but I'm ready for another code review. 👍 😄

mikechu-optimizely · 2022-10-12T12:50:50Z

packages/optimizely-sdk/tests/odpEventManager.spec.ts

+  it('should attempt to flush an empty queue at flush intervals', async () => {
+    const eventManager = new OdpEventManager({
+      odpConfig, apiManager, logger, clientEngine, clientVersion,
+      flushInterval: 100,
+    });
+    const spiedEventManager = spy(eventManager);
+
+    eventManager.start();
+    // do not add events to the queue, but allow for...
+    await pause(400); // at least 3 flush intervals executions (giving a little longer)
+
+    verify(spiedEventManager['processQueue'](anything())).atLeast(3);
+  });


@jaeopt Is this a good test for ensuring flush intervals?

zashraf1985

Looks Good! Great Work!

jaeopt

LGTM

mikechu-optimizely · 2022-10-12T17:08:10Z

Thanks @jaeopt and @zashraf1985

mikechu-optimizely added 17 commits August 30, 2022 16:15

WIP initial ODP config and event manager

6569fdf

Merge branch 'master' into mike/odp-event-manager

cfc2938

WIP skeleton of methods

3f1594c

WIP task queue

cecfe65

Remove task queue work

f587aca

WIP event manager

9cb9f3a

WIP ODP event manager

a5ca291

needs tests and REST API sendEvents()

Async and lint warns silenced

6c6ff2f

ODP Event Manager test layout

010370e

Finish bug fixes

11659c8

Merge branch 'master' into mike/odp-event-manager

bbe5013

Merge branch 'master' into mike/odp-event-manager

e9ac4b1

# Conflicts: # packages/optimizely-sdk/lib/plugins/odp/odp_client.ts # packages/optimizely-sdk/lib/plugins/odp/odp_event.ts # packages/optimizely-sdk/lib/utils/enums/index.ts

WIP ODP event manager unit tests

c83d584

WIP help from John

f28430a

WIP observer patternizification

dfda1cd

Event manager and dispatcher with unit tests

e2808d5

Fixes to for unit tests

b2b8be0

mikechu-optimizely added the feature label Sep 30, 2022

mikechu-optimizely requested a review from a team as a code owner September 30, 2022 15:22

optimizely-codereview-bot assigned mikechu-optimizely Sep 30, 2022

mikechu-optimizely requested review from zashraf1985 and jaeopt and removed request for a team September 30, 2022 15:23

Merge branch 'master' into mike/odp-event-manager

0668890

zashraf1985 suggested changes Oct 3, 2022

View reviewed changes

mikechu-optimizely and others added 2 commits October 3, 2022 17:23

Update packages/optimizely-sdk/lib/plugins/odp/odp_config.ts

74032bf

Co-authored-by: Zeeshan Ashraf <35262377+zashraf1985@users.noreply.github.com>

Review code changes

7e0a881

mikechu-optimizely and others added 8 commits October 4, 2022 09:16

Update packages/optimizely-sdk/lib/plugins/odp/odp_event_manager.ts

0157b90

Co-authored-by: Jae Kim <45045038+jaeopt@users.noreply.github.com>

Code review changes

98c75d7

Code review edits and JS Doc

58a6bd5

Fix message

60d435a

Code review requested changes

ab63fab

JS Docs

b50d357

Fix data_source_version

dc14b73

Remove tsconfig.spec.json

1d2abb8

mikechu-optimizely closed this Oct 7, 2022

mikechu-optimizely reopened this Oct 7, 2022

opti-jnguyen mentioned this pull request Oct 8, 2022

feat: Added ODP Segment Manager Implementation #790

Merged

Read client engine and version from OptiOptions

f82fbff

mikechu-optimizely requested review from zashraf1985 and jaeopt October 10, 2022 15:40

mikechu-optimizely added 2 commits October 10, 2022 17:33

Code review changes

46c2720

Require client engine & version for ODP Event Manager

554773e

jaeopt requested changes Oct 11, 2022

View reviewed changes

zashraf1985 suggested changes Oct 11, 2022

View reviewed changes

Code review changes

32a0c10

mikechu-optimizely requested review from jaeopt and zashraf1985 October 11, 2022 20:26

Fix setting timeout; add multiple flush test

346da4e

mikechu-optimizely commented Oct 12, 2022

View reviewed changes

zashraf1985 approved these changes Oct 12, 2022

View reviewed changes

jaeopt approved these changes Oct 12, 2022

View reviewed changes

mikechu-optimizely merged commit 510d247 into master Oct 12, 2022

mikechu-optimizely deleted the mike/odp-event-manager branch October 12, 2022 17:06

feat: Added ODPEventManager implementation #789

feat: Added ODPEventManager implementation #789

Uh oh!

Conversation

mikechu-optimizely commented Sep 30, 2022

Summary

Test plan

Jira

Uh oh!

coveralls commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikechu-optimizely commented Sep 30, 2022

Uh oh!

mikechu-optimizely commented Oct 3, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mikechu-optimizely commented Oct 11, 2022

Uh oh!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zashraf1985 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mikechu-optimizely commented Oct 12, 2022

Uh oh!

mikechu-optimizely Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

zashraf1985 left a comment

Choose a reason for hiding this comment

Uh oh!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

mikechu-optimizely commented Oct 12, 2022

Uh oh!

Uh oh!

coveralls commented Sep 30, 2022 •

edited

Loading