feat: Added ODPEventManager implementation #320

mikechu-optimizely · 2022-11-15T22:19:38Z

Summary

Added OdpEventManager implementation.

Test plan

Unit tests added
Existing tests continue to pass

Jira

FSSDK-8444

# Conflicts: # OptimizelySDK.Tests/OptimizelySDK.Tests.csproj # OptimizelySDK.sln.DotSettings # OptimizelySDK/Odp/Entity/OdpEvent.cs # OptimizelySDK/OptimizelySDK.csproj

Needed control over threading/non-parallel test execution

This reverts commit d0528e7.

Minimum that still gives me SingleThreaded. Hopefully monodevel will be okay with it

There's gotta be a better way.

Thanks to finding Java's CountDownLatch :-)

to resolve the GH Actions linting

match current supported version of ubuntu image in use

GH Actions Log: >>>> xbuild tool is deprecated and will be removed in future updates, use msbuild instead <<<<

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

…e/odp-event-manager

msohailhussain

lgtm but please address some nits.

msohailhussain · 2022-11-16T05:28:07Z

OptimizelySDK/Odp/Entity/OdpEvent.cs


        /// <summary>
        /// Event data in a key-value pair format
        /// </summary>
-        public Dictionary<string, object> Data { get; }
+        public Dictionary<string, object> Data { get; set; }


can we make it private set or what's the need of it?

We mutate the Data in OdpEventManager.cs's SendEvent() with a call to AugmentCommonData()

msohailhussain · 2022-11-16T05:30:53Z

OptimizelySDK/Odp/IOdpEventManager.cs

+
+namespace OptimizelySDK.Odp
+{
+    public interface IOdpEventManager


comments please.

msohailhussain · 2022-11-16T05:32:31Z

OptimizelySDK/Odp/OdpConfig.cs

+        /// <returns>true if ODP configuration can be used otherwise false</returns>
+        public virtual bool IsReady()
+        {
+            return !string.IsNullOrWhiteSpace(ApiKey) && !string.IsNullOrWhiteSpace(ApiHost);


I will prefer one not instead of two.
!a && !b = !(a || b)

msohailhussain · 2022-11-16T05:33:40Z

OptimizelySDK/Odp/OdpEventManager.cs

+            "Guid",
+        };
+
+        private readonly Dictionary<string, object> _commonData = new Dictionary<string, object>


please move this data initialization into constructor.

msohailhussain · 2022-11-16T18:25:03Z

OptimizelySDK/Odp/OdpEventManager.cs

+        }
+
+        /// <summary>
+        /// Immediately send all ODP events in queue


Immediately send a signal to send all events in queue.

msohailhussain · 2022-11-16T18:25:27Z

OptimizelySDK/Odp/OdpEventManager.cs

+            _eventQueue.Add(_flushSignal);
+        }
+
+        private void FlushQueue()


this is the one, sends events in queue. please add summary.

jaeopt

Looks good! I see a couple of more thread-safety cases and a few small issues. See my comments.

jaeopt · 2022-11-16T18:04:30Z

OptimizelySDK/Odp/OdpEventManager.cs

+        /// </summary>
+        public void Flush()
+        {
+            _flushingIntervalDeadline = DateTime.Now.MillisecondsSince1970() +


Do we need this update here? This is only place we share flushingIntervalDeadline between threads.

jaeopt · 2022-11-16T18:05:27Z

OptimizelySDK/Odp/OdpEventManager.cs

+        /// <param name="odpEvent"></param>
+        private void AddToBatch(OdpEvent odpEvent)
+        {
+            lock (_mutex)


Can we remove this lock?

jaeopt · 2022-11-16T18:11:44Z

OptimizelySDK/Odp/OdpEventManager.cs

+            }
+
+            List<OdpEvent> toProcessBatch;
+            lock (_mutex)


Why lock here? All looks like in the same thread.

OptimizelySDK/Odp/OdpEventManager.cs

jaeopt · 2022-11-16T18:21:55Z

OptimizelySDK/Odp/OdpEventManager.cs

+            if (Disposed || !IsStarted || !_odpConfig.IsReady())
+            {
+                _logger.Log(LogLevel.WARN, Constants.ODP_NOT_INTEGRATED_MESSAGE);
+                return;
+            }


We need to separate cases. ODP_NOT_INTEGRATED may be a wrong message for other conditions.

jaeopt · 2022-11-16T18:28:35Z

OptimizelySDK/Odp/OdpEventManager.cs

+                !_validOdpDataTypes.Any(t =>
+                    t.Equals(item.Value.GetType().Name, StringComparison.OrdinalIgnoreCase))


We can consider to improve this. For a long data set and valid type set, this can be slow for every event.

jaeopt · 2022-11-16T18:46:25Z

OptimizelySDK/Odp/OdpEventManager.cs

+
+        /// <summary>
+        /// Add additional common data including an idempotent ID and execution context to event data
+        /// </summary>


Can we add a comment that sourceData has a higher priority if keys conflict?

jaeopt · 2022-11-16T18:49:11Z

OptimizelySDK/Odp/OdpConfig.cs

+        /// <summary>
+        /// Public API key for the ODP account from which the audience segments will be fetched (optional).
+        /// </summary>
+        public string ApiKey { get; private set; }


Don't we need to lock-protect all these "ApiKey", "ApiHost" and "SegmentsToCheck" get/set for thread-safety?

mikechu-optimizely · 2022-11-17T18:37:28Z

@jaeopt, let me know if you have any further changes you'd like to see. Thanks. 😃

jaeopt

LGTM - I see one log message inappropriate. Other than all looks good!

jaeopt · 2022-11-18T18:17:16Z

OptimizelySDK/Odp/OdpEventManager.cs

+
+            if (IsStarted && !Disposed)
+            {
+                _logger.Log(LogLevel.WARN, Constants.ODP_NOT_ENABLED_MESSAGE);


Is this message correct for when IsStarted is already ON?

I agree with you. It doesn't seem quite right.

I couldn't decide which message to provide from the TDD's "3.6 Errors/Log Messages".

I was hoping for something like "ODP has not been started."

@mikechu-optimizely It should be something like "already started". Not related to ODP.

mikechu-optimizely added 30 commits October 13, 2022 17:14

WIP initial ODP event manager

4367d19

Finish implementing ODP Event Manager; scaffolding tests

9d6fea9

WIP ODP Event Manager unit tests

c540923

WIP unit tests & fixes

ed51cc9

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

d01ad3e

# Conflicts: # OptimizelySDK.Tests/OptimizelySDK.Tests.csproj # OptimizelySDK.sln.DotSettings # OptimizelySDK/Odp/Entity/OdpEvent.cs # OptimizelySDK/OptimizelySDK.csproj

Post merge fixes

2f2e5a9

WIP Fixing code & tests

6908e19

Additional unit tests added

6b14f27

Upgraded NUnit+fixed upgrade errors; finished tests

5d6a3dc

Needed control over threading/non-parallel test execution

Internal doc + fix NuGet ref

3d6cf7b

Upgrade NUnitTestAdapter

d0528e7

Revert "Upgrade NUnitTestAdapter"

a65dff0

This reverts commit d0528e7.

Downgrade to minimum NUnit 3 version

b3df0c1

Minimum that still gives me SingleThreaded. Hopefully monodevel will be okay with it

Downgrade back to NUnit 2

582d9c8

Adjust test's wait

5be348f

Wait longer?

630a287

There's gotta be a better way.

Rollback non-ODP unit test changes

a58d92e

Doc & fix

9ab755e

Use CountdownEvent

0069620

Thanks to finding Java's CountDownLatch :-)

Add GITHUB_TOKEN to Actions

c9396ff

Test changing to lf

c808bfe

to resolve the GH Actions linting

Updates to GitHub Actions

1f4d27a

Pull request code review changes

4abb614

Fix batch detection

82ee2bf

Legacy changes rollback; Adjust Event Manager test

654de45

Fix lint whitespace

f5b7af0

Correction to packages.config

d5ed5c4

Add name to JIRA ref check

f639819

NIT Jira Action name; Update mono version

22a2f94

match current supported version of ubuntu image in use

Change to msbuild

0b1dd57

GH Actions Log: >>>> xbuild tool is deprecated and will be removed in future updates, use msbuild instead <<<<

mikechu-optimizely and others added 10 commits November 9, 2022 14:42

PR review changes

bd5d2b7

Pull request changes

8f55906

Fix lint

7e1c906

Test and lint fixes

cdbccd5

Update OptimizelySDK/Odp/OdpEventManager.cs

f9c21c3

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

Additional lock when emptying queue

ef830ff

Merge remote-tracking branch 'origin/mike/odp-event-manager' into mik…

e3be457

…e/odp-event-manager

Rollback select files

4ae9389

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

cd27fb6

Requested code changes to model BatchEventProcessor

d59baab

mikechu-optimizely added the feature label Nov 15, 2022

optimizely-codereview-bot assigned mikechu-optimizely Nov 15, 2022

mikechu-optimizely requested review from msohailhussain and jaeopt November 15, 2022 22:20

mikechu-optimizely marked this pull request as ready for review November 15, 2022 22:21

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

Remove unneeded test

1104fb7

mikechu-optimizely removed the request for review from a team November 16, 2022 15:52

msohailhussain approved these changes Nov 16, 2022

View reviewed changes

jaeopt requested changes Nov 16, 2022

View reviewed changes

Pull Request changes

9369918

mikechu-optimizely requested a review from jaeopt November 16, 2022 21:50

Linter corrections

d67e9c5

Bug fix

43211a1

jaeopt approved these changes Nov 18, 2022

View reviewed changes

mikechu-optimizely added 2 commits November 18, 2022 14:36

Pull request code review changes

1ace1cd

Revert accidental file inclusion

5567e81

mikechu-optimizely merged commit 3eb53bd into master Nov 18, 2022

mikechu-optimizely deleted the mike/odp-event-manager branch November 18, 2022 20:20

		!_validOdpDataTypes.Any(t =>
		t.Equals(item.Value.GetType().Name, StringComparison.OrdinalIgnoreCase))

feat: Added ODPEventManager implementation #320

feat: Added ODPEventManager implementation #320

Uh oh!

Conversation

mikechu-optimizely commented Nov 15, 2022

Summary

Test plan

Jira

Uh oh!

msohailhussain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikechu-optimizely commented Nov 17, 2022

Uh oh!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!