Sameeran/ff 924 update js sdk common with allocation key #12

sameerank · 2023-09-11T20:26:28Z

labels: mergeable

Motivation and Context

Description

Renames experimentKey to flagKey
Extracts allocationKey from output of getAssignmentInternal
Updates onPostAssignment to optionally include allocationKey
Update logAssignment to use allocationKey in creation of the event object that goes to the logger (here's where we get the allocation, experiment, and featureFlag event attributes)

How has this been tested?

…te post assignment functions to optionally use it

sameerank · 2023-09-11T20:37:31Z

src/assignment-hooks.ts

   * @public
   */
-  onPostAssignment(experimentKey: string, subject: string, variation: EppoValue | null): void;
+  onPostAssignment(flagKey: string, subject: string, variation: EppoValue | null, allocationKey?: string | null): void;


This is, I think, the one decision point in this PR where there was some ambiguity.

After assignment, we could either:

keep the original experimentKey argument and pass in the experiment key (${flagKey}-${allocationKey}) instead of the just the flagKey

change the first arg to flagKey argument and pass in the allocation key as an optional argument

keep this function entirely unchanged

I went with option 2. here because

It keeps all the original functionality of the onPostAssignment function (i.e. not changing the format of the string that gets passed into the function's first argument) and consequently doesn't require an immediate code change

It also enables custom allocation-level post assignment functionality if necessary, but would require the user to be aware of allocationKeys to use this correctly

If I went with 1, the onPostAssignment code (customer-end) would need to be updated to handle the new key format, which works against our goal of making this transition to logging allocationKeys as friction-free as possible

If we went with 3, customers would not be able to run onPostAssignment code with the same granularity as the key we use for experiment analysis, and they likely will want that since running experiments is the primary reason for using Experiment Allocations

Option 2 makes the most sense to me as well, for the reasons you outlined. You could also

Another option could be to allow the function to also take an options object argument (which we'd detect by the first argument being an object), but that would probably be messier.

Ah the options idea is a cool one, but agreed that it's a bit messy

aarsilv

Nice work! Approving with some consistency & cleanup thoughts that you can take or leave as you see fit!

aarsilv · 2023-09-11T23:47:47Z

src/assignment-hooks.ts

   * @public
   */
-  onPostAssignment(experimentKey: string, subject: string, variation: EppoValue | null): void;
+  onPostAssignment(flagKey: string, subject: string, variation: EppoValue | null, allocationKey?: string | null): void;


Option 2 makes the most sense to me as well, for the reasons you outlined. You could also

Another option could be to allow the function to also take an options object argument (which we'd detect by the first argument being an object), but that would probably be messier.

aarsilv · 2023-09-11T23:48:49Z

src/client/eppo-client.ts

   *
   * @param subjectKey an identifier of the experiment subject, for example a user ID.
-   * @param experimentKey experiment identifier
+   * @param flagKey feature flag identifier


I like the more detailed docs in this SDK, I wonder if we should port those PHP and others 🤔

aarsilv · 2023-09-11T23:53:53Z

src/client/eppo-client.ts

+    if (assignment !== null && allocationKey !== null)
+      this.logAssignment(flagKey, allocationKey, assignment, subjectKey, subjectAttributes);


Nice, logAssignment() creates an event under the hood which is passed to the user provided logging function, so changing the order of the arguments here is ok 👍

aarsilv · 2023-09-11T23:56:15Z

src/client/eppo-client.ts

    subjectKey: string,
-    experimentKey: string,
+    flagKey: string,
    subjectAttributes: Record<string, EppoValue> = {},
    assignmentHooks?: IAssignmentHooks | undefined,
  ): string | null {
-    const assignment = this.getAssignmentInternal(
+    const { allocationKey, assignment } = this.getAssignmentInternal(
      subjectKey,
-      experimentKey,
+      flagKey,
      subjectAttributes,
      assignmentHooks,
      ValueType.StringType,
    );
-    assignmentHooks?.onPostAssignment(experimentKey, subjectKey, assignment);
+    assignmentHooks?.onPostAssignment(flagKey, subjectKey, assignment, allocationKey);

-    if (assignment !== null)
-      this.logAssignment(experimentKey, assignment, subjectKey, subjectAttributes);
+    if (assignment !== null && allocationKey !== null)
+      this.logAssignment(flagKey, allocationKey, assignment, subjectKey, subjectAttributes);

    return assignment?.stringValue ?? null;


The code was already like this, but there is a lot of duplication which may be able to be folded into a private helper function that calls getAssignmentInternal(), onPostAssignment(), logAssignment() and returns the relevant property from assignment. If up for it, it would probably clean things up a bunch. (Had it been done before, you'd only need these changes in one place instead of four)

Yeah I feel like that's basically what we had with the get_assignment_variation function in the dynamically typed languages. I also liked the early return logic to prevent logging events. I felt it was more readable than keeping track of nulls and performing actions based on whether something is null. I might try cleaning this up

aarsilv · 2023-09-12T00:00:27Z

src/client/eppo-client.ts


-    if (assignment !== null)
-      this.logAssignment(experimentKey, assignment, subjectKey, subjectAttributes);
+    if (assignment !== null && allocationKey !== null)


allocationKey will be null for overrides returned by getAssignmentInternal, so this is how prevent logging an assignment for an overridden subject. Nice. Howeve,r I think before this change, this SDK was logging overrides 😬

Hmmm I'm thinking about this more and now wondering if I should keep the original functionality of logging overrides ..

So there's the onPreAssignment function, which is one way to override the assignment

And there's subject overrides (see getSubjectVariationOverride) which uses the "overrides" attribute in the RAC. Also I don't see any UI for supporting this (but I do see one non-empty entry for opted_in_subjects in the experiment_variations table), so this seems to be a custom solution we built for a specific purpose.

The onPreAssignment call doesn't exist in any of the other SDKs, so maybe this is all an edge case that doesn't require us to actively work on making all the other SDKs consistent, as far as whether we log or don't log overrides.

BUT I feel like any customer using these features might actually want to see these overridden assignments logged or how else would they be able to verify that the overrides are working?

Actually nevermind, I don't know what to do with the experiment key when there's no allocation key. I can't leave out the allocation key because we're already doing a '${flagKey}' or '${flagKey}-${allocationKey}' to find experiment assignments and putting in any placeholder text for the allocationKey like '${flagKey}-override` is fragile if anyone makes a feature flag key that ends with '-override'

Maybe we can take the stance that overrides are not assignments and so we leave them out of analysis and we leave it to the customers to track this information if they want it, e.g. with the onPre/PostAssignment hooks

aarsilv · 2023-09-12T00:06:27Z

src/client/eppo-client.ts

    switch (valueType) {
      case ValueType.BoolType:
-        return EppoValue.Bool(assignedVariation as boolean);
+        return { ...allocationKey, assignment: EppoValue.Bool(assignedVariation as boolean) };


Not your code, but I believe by telling TypeScript to treat these things as a type, we could actually end up passing in a numeric and will just be a number stored (and returned from) boolValue. It only matters in the edge case of a developer requesting the incorrect variant type for an experiment. But this is a different behavior than our PHP SDK (which would return null) and Java SDK (which would cast)

I agree that this behavior is too divergent from the other SDKs and needs more guardrails. I just added a isExpectedType method to EppoValue like I did in the python and ruby SDKs and return NullType if there's a mismatch. I do a typeof <numeric_variable> === 'number' comparison. That should suffice, right?

…signment method was called

sameerank added 3 commits September 11, 2023 12:15

Rename experimentKey to flagKey and add new keys to event interface

b08d910

Include allocation key in get assignment internal output and and upda…

76ff1ca

…te post assignment functions to optionally use it

Include allocationKey in logAssignment call

9b07f83

sameerank requested review from leoromanovsky, vpai and petzel September 11, 2023 20:26

sameerank commented Sep 11, 2023

View reviewed changes

Fix missing allocationKey null check

5dc24c0

sameerank marked this pull request as ready for review September 11, 2023 20:41

sameerank requested review from aarsilv and removed request for petzel September 11, 2023 21:06

aarsilv approved these changes Sep 12, 2023

View reviewed changes

aarsilv assigned sameerank Sep 12, 2023

sameerank added 6 commits September 12, 2023 14:40

Update language in test to flagKey

c16d76b

Make getAssignmentVariation function to reduce code repetition

cc2e5b3

Add isExpectedType method and return null if the the wrong getTypedAs…

1ba1dcd

…signment method was called

eslint warnings

cd83ff8

More eslint

a081cbc

Use eslint prettier to format

983afe3

sameerank merged commit 10f2fcc into main Sep 13, 2023

sameerank deleted the sameeran/ff-924-update-js-sdk-common-with-allocation-key branch September 13, 2023 17:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Sameeran/ff 924 update js sdk common with allocation key #12

Sameeran/ff 924 update js sdk common with allocation key #12

Uh oh!

sameerank commented Sep 11, 2023

Uh oh!

sameerank Sep 11, 2023 •

edited

Loading

Uh oh!

aarsilv Sep 11, 2023

Uh oh!

sameerank Sep 12, 2023

Uh oh!

aarsilv left a comment

Uh oh!

aarsilv Sep 11, 2023

Uh oh!

aarsilv Sep 11, 2023

Uh oh!

aarsilv Sep 11, 2023

Uh oh!

aarsilv Sep 11, 2023

Uh oh!

sameerank Sep 12, 2023

Uh oh!

aarsilv Sep 12, 2023

Uh oh!

sameerank Sep 12, 2023 •

edited

Loading

Uh oh!

sameerank Sep 13, 2023 •

edited

Loading

Uh oh!

aarsilv Sep 12, 2023

Uh oh!

sameerank Sep 13, 2023

Uh oh!

Uh oh!

		if (assignment !== null && allocationKey !== null)
		this.logAssignment(flagKey, allocationKey, assignment, subjectKey, subjectAttributes);

Sameeran/ff 924 update js sdk common with allocation key #12

Sameeran/ff 924 update js sdk common with allocation key #12

Uh oh!

Conversation

sameerank commented Sep 11, 2023

labels: mergeable

Motivation and Context

Description

How has this been tested?

Uh oh!

sameerank Sep 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

aarsilv 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!

sameerank Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sameerank Sep 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

sameerank Sep 11, 2023 •

edited

Loading

sameerank Sep 12, 2023 •

edited

Loading

sameerank Sep 13, 2023 •

edited

Loading