feat: Added ODPManager implementation #322

mikechu-optimizely · 2022-11-29T19:52:01Z

Summary

Added ODPManager Implementation which does the following.

Initializes and provides access to ODPEventManager and ODPSegmentManager
Provides updated ODPConfig settings to event manager and segment manager.
Stops Event Manager thread when closed.

Test plan

Manually tested thoroughly
Added unit tests.

Issues

FSSDK-8453

mikechu-optimizely · 2022-11-29T20:20:55Z

OptimizelySDK/OptimizelySDK.csproj

    <Compile Include="Odp\IOdpSegmentManager.cs" />
-    <Compile Include="Odp\OdpConfig.cs" />


This was already declared on line 112/113 below. Added OdpManager.cs in its place.

msohailhussain

please address.

msohailhussain · 2022-11-30T07:22:53Z

OptimizelySDK/Odp/IOdpManager.cs

+        /// <param name="apiHost">Host portion of the URL of ODP</param>
+        /// <param name="segmentsToCheck">Audience segments to consider</param>
+        /// <returns>True if settings were update otherwise False</returns>
+        bool UpdateSettings(string apiKey, string apiHost, List<string> segmentsToCheck);


shouldn't it be OdpConfig in args

Generally, I too like to pass around objects, but I'm looking towards the Swift and also the Python as references.

msohailhussain · 2022-11-30T07:23:36Z

OptimizelySDK/Odp/IOdpManager.cs

+        /// <param name="userId">FS User ID</param>
+        /// <param name="options">Options used during segment cache handling</param>
+        /// <returns>Qualified segments for the user from the cache or the ODP server</returns>
+        List<string> FetchQualifiedSegments(string userId, List<OdpSegmentOption> options);


nit: This should be Array

Oh, yes. Sorry. Old habit from previous work.

msohailhussain · 2022-11-30T07:24:36Z

OptimizelySDK/Odp/IOdpManager.cs

+        /// <summary>
+        /// Sends signal to stop Event Manager and clean up ODP Manager use
+        /// </summary>
+        void Close();


This should be Dispose. My understanding is if it's closed, then can't use again.

I think you're right here.

I didn't look carefully at the Java close() method as part of implementing AutoCloseable in that language. I judged that we wanted Close() after looking at Python.

C#'s IDisposable makes way more sense.

msohailhussain · 2022-11-30T07:29:38Z

OptimizelySDK/Odp/OdpConfig.cs

+        /// </summary>
+        /// <param name="toCompare">OdpConfig object to compare current instance against</param>
+        /// <returns>True if equal otherwise False</returns>
+        public bool Equals(OdpConfig toCompare)


what's the reason to add this method. This will raise exception if ApiKey is null. Secondly use equals method where needed in this class and implement IEqu... interface as well.

The goal of this is to control the equality determination of two ODP Configs.

Thanks for the callout on null exception. I'll change to use IEqualityComparer.

Oh maybe IEquatable<T> might work instead of having a comparer. 🤔

msohailhussain · 2022-11-30T07:30:17Z

OptimizelySDK/Odp/OdpManager.cs

+        public bool UpdateSettings(string apiKey, string apiHost, List<string> segmentsToCheck)
+        {
+            var newConfig = new OdpConfig(apiKey, apiHost, segmentsToCheck);
+            if (_odpConfig.Equals(newConfig))


can't we use ==

Yes Sir. I'll update once using the IEqualityComparer from OdpConfig

I tried overriding the implicit operator, but it gets messy and unclear. I'd like to stay with the use of .Equals()

msohailhussain · 2022-11-30T07:31:00Z

OptimizelySDK/Odp/OdpManager.cs

+
+            _odpConfig = newConfig;
+
+            EventManager.Flush();


add comments in a code, why flushing

// because @jaeopt did it in Swift 😁

msohailhussain · 2022-11-30T07:32:12Z

OptimizelySDK/Odp/OdpManager.cs

+        /// <returns>Qualified segments for the user from the cache or the ODP server</returns>
+        public List<string> FetchQualifiedSegments(string userId, List<OdpSegmentOption> options)
+        {
+            if (!_enabled || SegmentManager == null || !_odpConfig.IsReady())


a minor comment,
SegmentManager == null || !_enabled || !_odpConfig.IsReady()

I can re-arrange (I think that's what you're asking)

I thought that perhaps !_enabled would evaluate to true more often than the others, but happy to switch.

msohailhussain · 2022-11-30T07:33:12Z

OptimizelySDK/Odp/OdpManager.cs

+            Dictionary<string, object> data
+        )
+        {
+            if (!_enabled || EventManager == null || !_odpConfig.IsReady())


This validations should be in a single method and return true or false. please refactor it.

My copy-paste finger started hurting, and I thought of that too.

L89, would have me injecting the Manager that should be checked for null so I just left it.

Let me play with some options....

mikechu-optimizely

Thanks for the comments @msohailhussain .

I'll hold for @jaeopt 's too before pushing to the repo

mikechu-optimizely · 2022-12-01T13:50:28Z

OptimizelySDK/Odp/IOdpManager.cs

+        /// <param name="apiHost">Host portion of the URL of ODP</param>
+        /// <param name="segmentsToCheck">Audience segments to consider</param>
+        /// <returns>True if settings were update otherwise False</returns>
+        bool UpdateSettings(string apiKey, string apiHost, List<string> segmentsToCheck);


Generally, I too like to pass around objects, but I'm looking towards the Swift and also the Python as references.

mikechu-optimizely · 2022-12-01T13:58:05Z

OptimizelySDK/Odp/IOdpManager.cs

+        /// <summary>
+        /// Sends signal to stop Event Manager and clean up ODP Manager use
+        /// </summary>
+        void Close();


I think you're right here.

I didn't look carefully at the Java close() method as part of implementing AutoCloseable in that language. I judged that we wanted Close() after looking at Python.

C#'s IDisposable makes way more sense.

mikechu-optimizely · 2022-12-01T14:03:07Z

OptimizelySDK/Odp/OdpConfig.cs

+        /// </summary>
+        /// <param name="toCompare">OdpConfig object to compare current instance against</param>
+        /// <returns>True if equal otherwise False</returns>
+        public bool Equals(OdpConfig toCompare)


The goal of this is to control the equality determination of two ODP Configs.

Thanks for the callout on null exception. I'll change to use IEqualityComparer.

mikechu-optimizely · 2022-12-01T14:03:42Z

OptimizelySDK/Odp/OdpManager.cs

+        public bool UpdateSettings(string apiKey, string apiHost, List<string> segmentsToCheck)
+        {
+            var newConfig = new OdpConfig(apiKey, apiHost, segmentsToCheck);
+            if (_odpConfig.Equals(newConfig))


Yes Sir. I'll update once using the IEqualityComparer from OdpConfig

mikechu-optimizely · 2022-12-01T14:06:57Z

OptimizelySDK/Odp/OdpManager.cs

+        /// <returns>Qualified segments for the user from the cache or the ODP server</returns>
+        public List<string> FetchQualifiedSegments(string userId, List<OdpSegmentOption> options)
+        {
+            if (!_enabled || SegmentManager == null || !_odpConfig.IsReady())


I can re-arrange (I think that's what you're asking)

I thought that perhaps !_enabled would evaluate to true more often than the others, but happy to switch.

mikechu-optimizely · 2022-12-01T14:08:48Z

OptimizelySDK/Odp/OdpManager.cs

+            Dictionary<string, object> data
+        )
+        {
+            if (!_enabled || EventManager == null || !_odpConfig.IsReady())


My copy-paste finger started hurting, and I thought of that too.

L89, would have me injecting the Manager that should be checked for null so I just left it.

Let me play with some options....

mikechu-optimizely · 2022-12-01T14:09:38Z

OptimizelySDK/Odp/IOdpManager.cs

+        /// <param name="userId">FS User ID</param>
+        /// <param name="options">Options used during segment cache handling</param>
+        /// <returns>Qualified segments for the user from the cache or the ODP server</returns>
+        List<string> FetchQualifiedSegments(string userId, List<OdpSegmentOption> options);


Oh, yes. Sorry. Old habit from previous work.

jaeopt

Looks good. A few changes suggested.

jaeopt · 2022-12-01T21:12:18Z

OptimizelySDK/Odp/OdpManager.cs

+            // NOTE: It should be rare but possible that odp public key is changed for the same datafile (sdkKey).
+            // - Try to send all old events with the previous public key.
+            // - If it fails to flush all the old events here (network error), remaining events will be discarded.
+            EventManager.Flush();


How can we guarantee all pending events use the old ODPConfig with the non-blocking flush?

That's a good consideration. I think this .Flush() should grab a copy of the _odpConfig in the OdpEventManager and inject it into the _flushSignal object. The secondary thread would then have the config to use for flush requests.

Does this seem like a suitable solution, or is this a corner case?
How does Swift (or Python) handle this situation?
What's your direction?

@mikechu-optimizely We can consider -

(swift) the current key and host are captures in the flush thread (so they can be used until flushing completed) before updating odpConfig or

(python) delay config update until flush is completed with the current key and host.

jaeopt · 2022-12-01T21:18:12Z

OptimizelySDK/Odp/OdpManager.cs

+                return false;
+            }
+
+            _odpConfig = newConfig;


When ODPConfig is updated, the OdpConfig object itself is replaced in ODPManager, while copying fields in ODPEventManager and ODPSegmentManager. Can we make it consistent in either way?

jaeopt · 2022-12-01T21:20:42Z

OptimizelySDK/Odp/OdpManager.cs

+            if (!EventManager.IsStarted)
+            {
+                _logger.Log(LogLevel.DEBUG,
+                    "ODP identify event not dispatched (ODP not integrated).");
+                return;
+            }


We can drop this. It should be rare and it'll be filtered out in EventManager anyway.

jaeopt · 2022-12-01T21:23:20Z

OptimizelySDK/Odp/OdpManager.cs

+        {
+            if (EventManagerOrConfigNotReady())
+            {
+                _logger.Log(LogLevel.DEBUG, "ODP event not dispatched (ODP disabled).");


Suggested change

_logger.Log(LogLevel.DEBUG, "ODP event not dispatched (ODP disabled).");

_logger.Log(LogLevel.ERROR, "ODP event not dispatched (ODP disabled).");

Implicit event sending (IdentifyUser) should be DEBUG level, but when clients try to send events when ODP is not ready, it should be ERROR.

jaeopt · 2022-12-01T21:24:33Z

OptimizelySDK/Odp/OdpManager.cs

+            if (!EventManager.IsStarted)
+            {
+                _logger.Log(LogLevel.DEBUG,
+                    "ODP event not dispatched (ODP not integrated).");
+                return;
+            }


We can drop this too.

jaeopt · 2022-12-01T21:33:28Z

OptimizelySDK/Odp/OdpManager.cs

+            {
+                var manager = new OdpManager
+                {
+                    _odpConfig = _odpConfig,


_odpConfig can be null if not set withOdpConfig. Is it safe?

I put a check in the Build() for you 😄

jaeopt · 2022-12-01T21:35:42Z

OptimizelySDK/Odp/OdpManager.cs

+
+                _errorHandler = _errorHandler ?? new NoOpErrorHandler();
+
+                if (_eventManager == null)


if _enabled == false, then we do not need instantiate EventManager and SegmentManager.

msohailhussain

lgtm

jaeopt

A few more changes suggested.

jaeopt · 2022-12-02T21:16:06Z

OptimizelySDK/Odp/OdpConfig.cs

@@ -81,27 +81,6 @@ public OdpConfig(string apiKey, string apiHost, List<string> segmentsToCheck)
            SegmentsToCheck = segmentsToCheck ?? new List<string>(0);
        }

-        /// <summary>


If the entire odpconfig is replaced for update, then we do not need t"volitile" for internal fields.

jaeopt · 2022-12-02T21:18:59Z

OptimizelySDK/Odp/OdpEventManager.cs

@@ -349,7 +349,7 @@ public void IdentifyUser(string userId)
        /// <param name="odpConfig">Configuration object containing new values</param>
        public void UpdateSettings(OdpConfig odpConfig)
        {
-            _odpConfig.Update(odpConfig.ApiKey, odpConfig.ApiHost, odpConfig.SegmentsToCheck);
+            _odpConfig = odpConfig;


If you use to replace _odpConfig, don't we need to set it to "volitile"?

volatile can only be set on some simple/primitive types.

Replacing the config as a whole now (instead of individual field values on the OdpConfig) should not be good. I'm not seeing where a child thread updates on the config.

Oh wait, this is a Reference type. Duh.

I can mark it as volatile if you see where other threads may update the config on this instance of the EventManager.

jaeopt · 2022-12-02T21:19:21Z

OptimizelySDK/Odp/OdpSegmentManager.cs

@@ -149,7 +149,7 @@ private static string GetCacheKey(string userKey, string userValue)
        /// <param name="odpConfig">New ODP Configuration to apply</param>
        public void UpdateSettings(OdpConfig odpConfig)
        {
-            _odpConfig.Update(odpConfig.ApiKey, odpConfig.ApiHost, odpConfig.SegmentsToCheck);
+            _odpConfig = odpConfig;


jaeopt · 2022-12-02T21:23:51Z

OptimizelySDK/Odp/OdpManager.cs

+                _logger = _logger ?? new DefaultLogger();
+                _errorHandler = _errorHandler ?? new NoOpErrorHandler();
+
+                if (_odpConfig == null)


What about setting if (odpConfig == null) _odpConfig = new ODPConfig() default.
Initially that's the best they can do when initializing ODPManager.

jaeopt · 2022-12-02T21:38:49Z

OptimizelySDK/Odp/OdpManager.cs

+            // NOTE: It should be rare but possible that odp public key is changed for the same datafile (sdkKey).
+            // - Try to send all old events with the previous public key.
+            // - If it fails to flush all the old events here (network error), remaining events will be discarded.
+            EventManager.Flush();


@mikechu-optimizely We can consider -

(swift) the current key and host are captures in the flush thread (so they can be used until flushing completed) before updating odpConfig or

(python) delay config update until flush is completed with the current key and host.

jaeopt · 2022-12-02T21:49:27Z

OptimizelySDK/Odp/OdpManager.cs


                if (_eventManager == null)
                {
                    var eventApiManager = new OdpEventApiManager(_logger, _errorHandler);

-                    manager.EventManager = new OdpEventManager.Builder().WithOdpConfig(_odpConfig).
+                    manager.EventManager = new OdpEventManager.Builder().


Wondering why using builder for OdpEventManager while not for OdpSegmentManager. Looks weird with 2 different patterns for similar objects.

Segment Manager is not too complex to instantiate. There's not a lot of logic that needed to be in the constructor, so I let a standard constructor do that work.

With Event Manager and ODP Manager, the combinations of instantiation parameters and logic in those combinations warranted (in my opinion) to use the Builder Pattern.

In practice, I don't have a hard limit of constructor length before breaking the logic out.

jaeopt

Looks good. One more fix for thread-safe.

jaeopt · 2022-12-07T00:03:57Z

OptimizelySDK/Odp/OdpManager.cs

@@ -35,7 +35,7 @@ public class OdpManager : IOdpManager, IDisposable
        /// <summary>
        /// Configuration used to communicate with ODP
        /// </summary>
-        private volatile OdpConfig _odpConfig;
+        private OdpConfig _odpConfig;


Why do you remove "volitile" here? Multiple thread read/write it here too.

jaeopt

LGTM

mikechu-optimizely added 7 commits November 23, 2022 14:30

Initial ODP Manager

dd028bf

Outline unit tests

a317407

WIP Unit tests + changes to CUD

443164c

Implement fetch, identify, and send events with Builder pattern

faea784

WIP Fixing final unit test

191a6ec

Finish unit tests

f6276bc

Add documentation

4f629fa

mikechu-optimizely added the feature label Nov 29, 2022

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

Lint fixes

bbf8ac1

mikechu-optimizely requested review from jaeopt and msohailhussain November 29, 2022 20:04

mikechu-optimizely marked this pull request as ready for review November 29, 2022 20:04

mikechu-optimizely requested a review from a team as a code owner November 29, 2022 20:04

mikechu-optimizely removed the request for review from a team November 29, 2022 20:17

mikechu-optimizely commented Nov 29, 2022

View reviewed changes

msohailhussain suggested changes Nov 30, 2022

View reviewed changes

mikechu-optimizely commented Dec 1, 2022

View reviewed changes

mikechu-optimizely added 3 commits December 1, 2022 11:40

Pull request code revisions + additional unit tests

4a54b4c

Lint fixes

0995694

Change to use string array over List

88b6684

mikechu-optimizely requested a review from msohailhussain December 1, 2022 16:55

mikechu-optimizely added 3 commits December 1, 2022 11:59

Clean usings

29514ff

Add new OdpConfig test

22b04ae

Small refactors

dcafcee

jaeopt requested changes Dec 1, 2022

View reviewed changes

Pull request code review changes

8747556

mikechu-optimizely requested a review from jaeopt December 2, 2022 18:50

msohailhussain approved these changes Dec 2, 2022

View reviewed changes

jaeopt requested changes Dec 2, 2022

View reviewed changes

Code review changes

dfaa27b

mikechu-optimizely requested a review from jaeopt December 6, 2022 20:15

Mark _odpConfig as volatile

c528dce

jaeopt requested changes Dec 7, 2022

View reviewed changes

Return volatile denotation

a5b28e6

mikechu-optimizely requested a review from jaeopt December 7, 2022 16:00

jaeopt approved these changes Dec 7, 2022

View reviewed changes

mikechu-optimizely merged commit bdaef7f into master Dec 7, 2022

mikechu-optimizely deleted the mike/odp-manager branch December 7, 2022 16:43

		<Compile Include="Odp\IOdpSegmentManager.cs" />
		<Compile Include="Odp\OdpConfig.cs" />

	_logger.Log(LogLevel.DEBUG, "ODP event not dispatched (ODP disabled).");
	_logger.Log(LogLevel.ERROR, "ODP event not dispatched (ODP disabled).");


		_errorHandler = _errorHandler ?? new NoOpErrorHandler();

		if (_eventManager == null)

feat: Added ODPManager implementation #322

feat: Added ODPManager implementation #322

Conversation

mikechu-optimizely commented Nov 29, 2022

Summary

Test plan

Issues

Choose a reason for hiding this comment

msohailhussain 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

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

mikechu-optimizely 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msohailhussain left a 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

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

jaeopt left a comment

Choose a reason for hiding this comment