feat(flag-decisions): Add support for sending flag decisions #599

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

yavorona merged 15 commits into master from pnguen/send-flag-decisions

Oct 28, 2020

Contributor

yavorona commented Oct 13, 2020 •

edited

Loading

Summary

Add experiment key, feature flag key, and decision type to impression events.
Send events for ALL decision types if sendFlagDecisions is enabled.
Add metadata field to capture metadata.flag_key, metadata.rule_key, metadata.rule_type, metadata.variation_key

Test plan

All FSC and unit tests

yavorona added the WIP label

yavorona requested a review from a team as a code owner

October 13, 2020 16:50

yavorona self-assigned this

coveralls commented Oct 13, 2020 •

edited

Loading

Coverage increased (+0.04%) to 96.707% when pulling 1e896cc on pnguen/send-flag-decisions into 6c62080 on master.

yavorona closed this

yavorona reopened this

Contributor Author

yavorona commented Oct 16, 2020

This pr depends on #601.


          Update activate and isFeatureEnabled to send decision events

865b852

Include metaData in ImpressionEvent

Send decisions when variation is null

Update existing event_helpers tests

Add metadata to impressionEventParams in getImpressionEventParams method

Add ruleKey parameter to ImpressionEvent

Change experimentKey to ruleKey

Update existing unit tests

Remove sendFlagDecisions from common event params

Update event-processor to include metadata in makeDecisionSnapshot

Update event-processos variationKey type to string | null

Update variationKey type to string | null

Update existing optimizely module tests part 1

Start adding new tests

Update unit tests impression params when decision.experiment is undefined

Clean up

Change layer id type to be string | null

Update variationId to bull as default

yavorona force-pushed the pnguen/send-flag-decisions branch from 88b84dc to 865b852 Compare

October 16, 2020 23:48


          Upgrade event-processor to 0.7.0

4e6b706

yavorona closed this

yavorona reopened this

yavorona closed this

yavorona reopened this


          Add createEventProcessor factory function

0eb0ee2

yavorona removed the WIP label

yavorona requested review from zashraf1985, pawels-optimizely and mjc1283

October 20, 2020 18:15

yavorona removed their assignment


          Add tests for getSendFlagDecisionsValue method

5b407d5

yavorona force-pushed the pnguen/send-flag-decisions branch 2 times, most recently from 8608a7b to dd43047 Compare

October 20, 2020 19:00


          Update variationKey type to be string

c4d1d51

yavorona force-pushed the pnguen/send-flag-decisions branch from dd43047 to c4d1d51 Compare

October 20, 2020 19:04

optimizely-codereview-bot added 🤖 4 hrs 🤖 8 hrs and removed 🤖 4 hrs labels

mjc1283 suggested changes

View reviewed changes

Contributor

mjc1283 left a comment

My main suggestion is to try passing around the decision object that includes as much as possible (experiment, variation, rule key, rule type, etc.). The potential benefit is to reduce the number of additional parameters and repeated checking for null/empty string.

packages/optimizely-sdk/lib/optimizely/index.ts Show resolved Hide resolved

packages/optimizely-sdk/lib/optimizely/index.ts Outdated

+                  experimentKey: string,
+                  variationKey: string,
+                  flagKey: string,
+                  ruleType: string,

Contributor

mjc1283 Oct 21, 2020

flagKey and ruleType have been added as new parameters here, so they can be passed on down to emitNotificationCenterActivate and buildImpressionEvent. We have 3 new functions that all need this new data.

All these functions need experimentKey, variationKey, flagKey, and ruleType. When it's called from isFeatureEnabled, these originate from the decision object.

What if instead we passed around the decision object instead of these 4 individual values? This refactoring would take a little work, but the result could be cleaner. We may be able to eliminate some of the repetitive checking of keys for null or empty string. Decision could even become a class with methods if we wanted to avoid repeating such checks (i.e. something like decision.getExperimentKey()).

packages/optimizely-sdk/lib/optimizely/index.ts Outdated

@@ @@ -641,6 +677,29 @@ export default class Optimizely { @@
                     const decision = this.decisionService.getVariationForFeature(configObj, feature, userId, attributes);
                     const variation = decision.variation;
+                    if (
+                      decision.decisionSource === DECISION_SOURCES.ROLLOUT &&
+                      projectConfig.getSendFlagDecisionsValue(configObj) === true

Contributor

mjc1283 Oct 21, 2020

getSendFlagDecisions returns a boolean.

Suggested change

      
                    projectConfig.getSendFlagDecisionsValue(configObj) === true
          
                    projectConfig.getSendFlagDecisionsValue(configObj)

packages/optimizely-sdk/lib/optimizely/index.ts Outdated

Comment on lines 684 to 692

+                      let experimentKey = '';
+                      if (decision.experiment !== null) {
+                        experimentKey = decision.experiment.key;
+                      }
+                      let variationKey = '';
+                      if (variation) {
+                        variationKey = variation.key
+                      }

Contributor

mjc1283 Oct 21, 2020 •

edited

Loading

After upgrading TS we have access to some new language features that I believe could make this section more succinct: optional chaining and nullish coalescing. Check these out and let me know what you think.

packages/optimizely-sdk/lib/optimizely/index.ts Outdated

@@ @@ -486,7 +522,7 @@ export default class Optimizely { @@
                  *                                      then clear the existing experiment-to-variation mapping.
                  * @return {boolean}                    A boolean value that indicates if the set completed successfully.
                  */
-                setForcedVariation(experimentKey: string, userId: string, variationKey: string | null): boolean {
+                setForcedVariation(experimentKey: string, userId: string, variationKey: string): boolean {

Contributor

mjc1283 Oct 21, 2020

Is this an unrelated fix, or does it matter to flag decisions?

Contributor Author

yavorona Oct 23, 2020 •

edited

Loading

Let's keep it as it was, it should not matter to flag decisions. I updated DecisionService.prototype.setForcedVariation comments to avoid confusion.

optimizely-codereview-bot removed the 🤖 8 hrs label

optimizely-codereview-bot assigned yavorona

yavorona added 5 commits

October 22, 2020 11:10


          Refactor isFeatureEnabled

6f755d2


          Pass decision object instead of 4 individual values

a0f873c


          Update DecisionService setForcedVariation signature and comments

d90f509


          Use optional chaining and nullish coalescing operator

bfb33ab


          Create decision helper functions

4df5b3f

yavorona requested a review from mjc1283

October 26, 2020 17:24

yavorona removed their assignment

mjc1283 suggested changes

View reviewed changes

Contributor

mjc1283 left a comment

This looks great! The changes in the Optimizely class are so much cleaner 💯 .

Requesting changes for adding a few unit tests in the decision module.

packages/optimizely-sdk/lib/core/decision/index.ts

		@@ -0,0 +1,44 @@
		/**

Contributor

mjc1283 Oct 26, 2020

Let's add some basic unit tests for this module. I think we should have some coverage for when experiment or variation are null or undefined.

packages/optimizely-sdk/lib/core/event_processor/index.ts

		@@ -0,0 +1,11 @@
		import { LogTierV1EventProcessor, LocalStoragePendingEventsDispatcher } from '@optimizely/js-sdk-event-processor';

Contributor

mjc1283 Oct 26, 2020

Suggested change

      
            import { LogTierV1EventProcessor, LocalStoragePendingEventsDispatcher } from '@optimizely/js-sdk-event-processor';
          
            /**
          
             * Copyright 2020, Optimizely
          
             *
          
             * Licensed under the Apache License, Version 2.0 (the "License");
          
             * you may not use this file except in compliance with the License.
          
             * You may obtain a copy of the License at
          
             *
          
             * http://www.apache.org/licenses/LICENSE-2.0
          
             *
          
             * Unless required by applicable law or agreed to in writing, software
          
             * distributed under the License is distributed on an "AS IS" BASIS,
          
             * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          
             * See the License for the specific language governing permissions and
          
             * limitations under the License.
          
             */
          
            import { LogTierV1EventProcessor, LocalStoragePendingEventsDispatcher } from '@optimizely/js-sdk-event-processor';

packages/optimizely-sdk/lib/core/project_config/index.js Outdated

                */
               export var getVariationIdFromExperimentAndVariationKey = function(projectConfig, experimentKey, variationKey) {
                 var experiment = projectConfig.experimentKeyMap[experimentKey];
-                if (experiment.variationKeyMap.hasOwnProperty(variationKey)) {
+                if (variationKey !== '' && experiment.variationKeyMap.hasOwnProperty(variationKey)) {

Contributor

mjc1283 Oct 26, 2020

Is this change really necessary? It seems like it would only matter in the case where variationKeyMap has a '' property, which I don't think will happen (we don't allow empty strings as variation keys).

Contributor Author

yavorona Oct 26, 2020

Good point. I will remove it.

packages/optimizely-sdk/lib/index.d.ts Outdated

@@ @@ -97,7 +97,7 @@ declare module '@optimizely/optimizely-sdk' { @@
                     userId: string,
                     attributes?: UserAttributes
                   ): string | null;
-                  setForcedVariation(experimentKey: string, userId: string, variationKey: string | null): boolean;
+                  setForcedVariation(experimentKey: string, userId: string, variationKey: string): boolean;

Contributor

mjc1283 Oct 26, 2020

This is a breaking API change - let's not do this in the current PR.

optimizely-codereview-bot assigned yavorona


          Address comments

69c0133

yavorona force-pushed the pnguen/send-flag-decisions branch 2 times, most recently from e2525d2 to 69c0133 Compare

October 26, 2020 22:28


          Clean up

3a338a4

yavorona requested a review from mjc1283

October 27, 2020 00:37

yavorona removed their assignment


          Fix experiment decision object

d1e1f9a

mjc1283 approved these changes

View reviewed changes

Contributor

mjc1283 left a comment

Please add an entry to CHANGELOG.md. Otherwise, LGTM.


          Change variation_key to '' instead of null in metadata

1e896cc

pawels-optimizely approved these changes

View reviewed changes

pawels-optimizely left a comment

lgtm


          Update Changelog

ad8ace8

yavorona merged commit 6f60a6b into master

yavorona deleted the pnguen/send-flag-decisions branch

October 28, 2020 17:28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet