feat: Integrated ODPManager with UserContext and Optimizely client #323

mikechu-optimizely · 2022-12-15T16:50:08Z

Summary

This PR integrates ODP functionality with UserContext and Optimizely client to make it available for users. There are two ways to integrate ODPManager.

😬 Sorry about the numerous auto-format changes polluting the core enhancements. Need to satisfy the linter.

Test plan

Manually tested thoroughly
Will add unit tests after the first review passes.

Issues

FSSDK-8476

Includes lint/format updates...Sorry PR-Reviewer :-/

Sorry for the linter formatting updates

jaeopt

Overall it looks good. See my first-round comments. Key changes suggested -

a logic to extract segments from audiences is missing. This is a key to ODP integration.
a fix for async fetchQualifiedSegments.

jaeopt · 2022-12-15T18:33:13Z

OptimizelySDK/Optimizely.cs

+                WithLogger(Logger).
+                WithErrorHandler(ErrorHandler).
+                Build(!SdkSettings.DisableOdp);
+            OdpManager.EventManager.Start();


I see "eventManager.start()" inside OdpManagher.Build()", which looks good. Do we need it here again?

jaeopt · 2022-12-15T18:56:26Z

OptimizelySDK/Config/DatafileProjectConfig.cs

@@ -373,6 +408,7 @@ private void Initialize()
            var integration = Integrations.FirstOrDefault(i => i.Key.ToLower() == "odp");
            HostForOdp = integration?.Host;
            PublicKeyForOdp = integration?.PublicKey;
+            Segments = TypedAudiences.Select(a => a.Name).ToArray();


This is not correct to get all segments from audiences. This is critical for ODP integration.
You can check with other SDKs and need more testing for this.

Looking 👀 into this....

I'm still working on how to parse odp. typed audiences based on other sdk work.

Can you review your other notes to see if they're ok before you go on holiday?

jaeopt · 2022-12-15T18:58:23Z

OptimizelySDK/Optimizely.cs

+            var optimizelyConfig =
+                new OptimizelyConfigService(config).GetOptimizelyConfig();
+            var allSegments = optimizelyConfig.Audiences.Select(a => a.Name).ToList();


You already have "Segments" in DatafileProjectConfig. Can't we use it directly.

jaeopt · 2022-12-15T19:02:46Z

OptimizelySDK/Optimizely.cs

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


This should not be public. It'll be called from OptimizelyUserContext only. "internal"?

jaeopt · 2022-12-15T19:02:58Z

OptimizelySDK/Optimizely.cs

+        /// Send identification event to Optimizely Data Platform
+        /// </summary>
+        /// <param name="userId">FS User ID to send</param>
+        public void IdentifyUser(string userId)


This should not be public. It'll be called from OptimizelyUserContext only. "internal"?

jaeopt · 2022-12-15T19:04:27Z

OptimizelySDK/Optimizely.cs

+            if (OdpManager == null)
+            {
+                Logger.Log(LogLevel.ERROR, Constants.ODP_NOT_ENABLED_MESSAGE);
+                return;
+            }


Why don't we have this guard for "fetchQualifiedSegments" and "identifyUser" above?

Oops, I meant to remove this guard here in SendOdpEvent too. Instantiating OdpManager should be guaranteed via InitializeComponents call be constructor.

Should add the guards back, just in case things change? What's your thought?

No need for guard here if it's guaranteed non-null.

jaeopt · 2022-12-15T19:11:24Z

OptimizelySDK/OptimizelyUserContext.cs

+            List<OdpSegmentOption> segmentOptions = null
+        )
+        {
+            var segments = Optimizely.FetchQualifiedSegments(userId,


This should be non-blocking if callback is provided (async API).
We can consider splitting the API into 2 (sync and async).

bool FetchQualifiedSegments(userId, options)

void FetchQaulifiedSegments(userId, options, callback)

hopefully ;-)

mikechu-optimizely · 2023-01-24T14:57:31Z

@mnoman09 I see where the FSC is failing. Is that what #325 is for?

jaeopt

Changes look good! One more suggestion to consider for compatibility support.

jaeopt · 2023-01-24T17:25:10Z

OptimizelySDK/Config/HttpProjectConfigManager.cs


        private HttpProjectConfigManager(TimeSpan period, string url, TimeSpan blockingTimeout,
-            bool autoUpdate, ILogger logger, IErrorHandler errorHandler, string sdkKey
+            bool autoUpdate, ILogger logger, IErrorHandler errorHandler, string sdkKey = null


This default null sdkKey parameter will make existing clients compile ok but it'll fail to run because of missing sdkKey for ODP notification.
What about removing the default null value (breaking changes note in release), so they are all forced to change to use ODP?

Can we also keep the list of all breaking changes, so we won't miss them in the release note?

Great concept. Are we planning to release ATS as a v4.0 or v3.12? For breaking changes, I'd like to go 4.0 with @trishahanlon 's documentation to back up the reasoning.

This default null sdkKey parameter will make existing clients compile ok...

If we're planning to release ODP as a minor semantic version, I'd like to keep the default. I guess this is a point of internal clarification in standup maybe.

It looks like we have a few breaking changes (new interface methods and new sdkKey constructor param) so we have to bump a major version to 4.0.

@jaeopt Thanks for the guidance. I hope this looks better now.

jaeopt · 2023-01-24T18:41:22Z

OptimizelySDK/Config/ProjectConfigManager.cs

+        /// <summary>
+        /// SDK key in use for this project
+        /// </summary>
+        string SdkKey { get; }


@mikechu-optimizely in addition to doc update, we should list all these breaking changes in release note.

mikechu-optimizely · 2023-01-24T21:44:48Z

Still holding for @msohailhussain 's approval (maybe if he's time).

@mnoman09 I'll hold for your merge and Approval too.

jaeopt

LGTM
@msohailhussain can you double check if you see any concerns about compatibility?

* Some suggested changes to fix null pointer exception * - Removed batchsize support from ODP event manager - Made some general fixes * Flush previous event * reverting this change Co-authored-by: mnoman09 <m.nomanshoaib09@gmail.com>

mikechu-optimizely · 2023-01-25T21:03:28Z

@msohailhussain @mnoman09 It looks like some FSC tests are erroring for this branch. Shall I start investigating the test app?

mnoman09 · 2023-01-26T13:54:45Z

@msohailhussain @mnoman09 It looks like some FSC tests are erroring for this branch. Shall I start investigating the test app?

I can look at the testapp. Most of these tests are failing because we haven't merged the testapp PR: 79. Once we merge that PR FSC should pass.

mnoman09

Overall, these changes lgtm.

mikechu-optimizely · 2023-01-26T13:55:38Z

Great. Thank you.

mikechu-optimizely · 2023-01-27T13:42:24Z

Is there something I can be helping with for getting FSC/test-app scenarios corrected?

@mnoman09 @msohailhussain

msohailhussain · 2023-01-30T22:09:34Z

Is there something I can be helping with for getting FSC/test-app scenarios corrected?

@mnoman09 @msohailhussain

I will do by tomorrow.

msohailhussain

have few more comments. please address.

msohailhussain · 2023-01-31T16:10:27Z

OptimizelySDK.Tests/AudienceConditionsTests/SegmentsTests.cs

+        [TestFixtureSetUp]
+        public void Setup()
+        {
+            _qualifiedMatchCondition = new BaseCondition


not addressed.

OptimizelySDK/Odp/LruCache.cs

msohailhussain · 2023-01-31T16:13:15Z

OptimizelySDK.Tests/OdpTests/LruCacheTest.cs

@@ -67,7 +67,7 @@ public void ShouldSaveAndLookupMultipleItems()
            cache.Save("user2", _segments3And4);
            cache.Save("user3", _segments5And6);

-            var cacheKeys = cache._readCurrentCacheKeys();
+            var cacheKeys = cache.CurrentCacheKeysForTesting();


Please use reflection to get private properties.

msohailhussain · 2023-01-31T16:16:11Z

OptimizelySDK.Tests/OptimizelySDK.Tests.csproj

@@ -151,6 +153,9 @@
    </ProjectReference>
  </ItemGroup>
  <ItemGroup>
+    <None Include="..\keypair.snk">


why this added?

This allows for signing the Test project so InternalsVisibleTo would work when testing runs in GitHub Actions.

msohailhussain · 2023-01-31T16:17:50Z

OptimizelySDK/Config/HttpProjectConfigManager.cs

+        private string _sdkKey = string.Empty;
+
+        private HttpProjectConfigManager(TimeSpan period, string url, TimeSpan blockingTimeout,
+            bool autoUpdate, ILogger logger, IErrorHandler errorHandler, string sdkKey


constructor is private, not a breaking change.

OptimizelySDK/Odp/OdpEventManager.cs

OptimizelySDK/Odp/OdpManager.cs

mikechu-optimizely · 2023-01-31T21:44:22Z

I'm starting to understand FSC and the C# testapp. Working to figure out failing tests...

msohailhussain

thanks. lgtm

mikechu-optimizely · 2023-02-01T09:37:11Z

Thanks for the approval. FSC and the test app are my last blocker

* Refactored OdpEventManager flushEvent function. * To wait for events using flushInterval when eventBatch is not empty. * Or wait indefinitely if no event is in batch until any event arrive. * Avoid calling flush on shutdown if no events are present * Avoid calling unnecessary flushQueue if _currentBatch.Count is zero. * Fixed the test as when there are no remaining items in queue then odpEventManager should not flush. * Removing mutex lock from notification registry instead using concurrentDictionary * Revert "Removing mutex lock from notification registry instead using concurrentDictionary" This reverts commit 64cdf80. * Added Thread.sleep which somehow enabling thread to shutdown gracefully. --------- Co-authored-by: mnoman09 <m.nomanshoaib09@gmail.com>

mikechu-optimizely · 2023-02-14T13:13:49Z

OMG @msohailhussain and @mnoman09 You guys are the best.

Valuable lesson for me is to keep PR's much smaller.

I'll start working on the full-repo linting.

Thanks again, you guys.

mikechu-optimizely added 12 commits December 8, 2022 16:49

Include ODP Manager instantiation in Optimizely.cs

57b6927

Includes lint/format updates...Sorry PR-Reviewer :-/

Add FetchQualifiedSegments to Optimizely.cs

3f27cfe

Add FetchQualifiedSegments to UserContext

040138c

Add IdentifyUser, fetch segments callback, & docs

bf9005a

WIP existing test fixes

d58a5a9

Fix legacy test (mock) constructors

65512ac

Add SendOdpEvent to Optimizely

6e498d8

WIP Init OdpManager

e7579cf

WIP Add OptimizelySdkSettings & fix some legacy tests

9a487bc

WIP fixes

3c8fc1c

Remove early UpdateOdpSettings

0a79a42

Add internal accessor for ProjectConfig Segments

0212708

Sorry for the linter formatting updates

mikechu-optimizely added the feature label Dec 15, 2022

mikechu-optimizely requested a review from jaeopt December 15, 2022 16:50

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

Linter fixes

d99e2cb

jaeopt requested changes Dec 15, 2022

View reviewed changes

mikechu-optimizely added 13 commits December 15, 2022 16:49

Preprocessing conditional compilation directives

3f29686

Add Segments array to ProjectConfig

002b115

Lint corrections

4edb545

More linter fixes

e4fbd0d

hopefully ;-)

WIP PR review changes

2f15f8d

Lint fix

19bac38

Lint fix

bc75c20

Add async FetchQualifiedSegments

3aa85d5

WIP parse TypedAudience to ODP Segments

6e818b9

WIP parsing Segments

4a0deea

More ODP test datafile more scenarios

06f4a6a

Revert IMultipleConditions concept

e7fec76

Correct Segments from datafile

dd16f7c

jaeopt requested changes Jan 24, 2023

View reviewed changes

mikechu-optimizely requested a review from jaeopt January 24, 2023 19:12

Require providing SDK key in HttpProjectConfigManager constructor

4653690

jaeopt approved these changes Jan 24, 2023

View reviewed changes

mnoman09 and others added 4 commits January 25, 2023 10:17

Fix: Some suggested changes related to ODP (#325)

c6c86ae

* Some suggested changes to fix null pointer exception * - Removed batchsize support from ODP event manager - Made some general fixes * Flush previous event * reverting this change Co-authored-by: mnoman09 <m.nomanshoaib09@gmail.com>

Merge fixes + lint

97678de

Lint fixes

5827136

Build fix and lint

90c75e6

mnoman09 approved these changes Jan 26, 2023

View reviewed changes

msohailhussain suggested changes Jan 31, 2023

View reviewed changes

optimizely deleted a comment from msohailhussain Jan 31, 2023

mikechu-optimizely added 2 commits January 31, 2023 16:14

PR change requests

d886133

Lint fix

597cb49

mikechu-optimizely requested a review from msohailhussain January 31, 2023 21:25

msohailhussain approved these changes Jan 31, 2023

View reviewed changes

mnoman09 closed this Feb 8, 2023

mnoman09 reopened this Feb 8, 2023

msohailhussain merged commit 2d8b71a into master Feb 14, 2023

msohailhussain deleted the mike/odp-usercontext-optimizelyclient branch February 14, 2023 06:13

feat: Integrated ODPManager with UserContext and Optimizely client #323

feat: Integrated ODPManager with UserContext and Optimizely client #323

Conversation

mikechu-optimizely commented Dec 15, 2022

Summary

Test plan

Issues

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

Choose a reason for hiding this comment

mikechu-optimizely commented Jan 24, 2023

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

mikechu-optimizely commented Jan 24, 2023

jaeopt left a comment

Choose a reason for hiding this comment

mikechu-optimizely commented Jan 25, 2023

mnoman09 commented Jan 26, 2023

mnoman09 left a comment

Choose a reason for hiding this comment

mikechu-optimizely commented Jan 26, 2023

mikechu-optimizely commented Jan 27, 2023

msohailhussain commented Jan 30, 2023

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

mikechu-optimizely commented Jan 31, 2023

msohailhussain left a comment

Choose a reason for hiding this comment

mikechu-optimizely commented Feb 1, 2023

mikechu-optimizely commented Feb 14, 2023