feat: Auto-assignment tracking #22

tyiuhc · 2023-07-17T16:46:23Z

Summary

Added auto-assignment tracking feature

Checklist

Does your PR title have the correct title format?
Does your PR have a breaking change?: No

…city

bgiori

will review again once other changes are in

bgiori · 2023-07-17T19:46:11Z

packages/node/src/types/variant.ts

@@ -23,8 +23,8 @@ export type Variants = {

 export type FlagResult = {
  value: string;
-  // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  payload: any | null | undefined;
+  payload: string;


Why has payload been converted to a string here?

bgiori

Awesome work Tim! Lets meet on Monday to go over these review comments.

bgiori · 2023-07-29T04:41:15Z

packages/node/src/assignment/assignment.ts

+    let sb =
+      this.user.user_id?.trim() + ' ' + this.user.device_id?.trim() + ' ';


nit: You can use a template string to start with, and you should use a different variable name, sb is used in java due to the StringBuilder class which is not used in javascript. I would prefer:

Suggested change

let sb =

this.user.user_id?.trim() + ' ' + this.user.device_id?.trim() + ' ';

let canonical = `${this.user.user_id?.trim()} ${this.user.device_id?.trim()} `;

bgiori · 2023-07-29T04:42:03Z

packages/node/src/assignment/assignment.ts

+      this.user.user_id?.trim() + ' ' + this.user.device_id?.trim() + ' ';
+    for (const key of Object.keys(this.results).sort()) {
+      const value = this.results[key];
+      sb += key.trim() + ' ' + value?.value?.trim() + ' ';


Use template string here.

bgiori · 2023-07-29T04:43:04Z

packages/node/src/assignment/assignment.ts

+export interface AssignmentService {
+  track(assignment: Assignment): Promise<void>;
+}
+
+export interface AssignmentFilter {
+  shouldTrack(assignment: Assignment): boolean;
+}


Absolute super minor nit: I prefer interface definitions at the top of the file :)

bgiori · 2023-07-29T04:47:21Z

packages/node/src/assignment/assignment-filter.ts

+import { DAY_MILLIS } from 'src/assignment/assignment-service';
+import { Cache } from 'src/util/cache';
+
+export const DEFAULT_FILTER_CAPACITY = 65536;


I know you're just transcribing my code, so this is really my fault, but: Remove this default from this file, set the default in a DefaultAssignmentConfiguration object in the config.ts file. Then object spread the input with the defaults (as we do with the base configuration) in local evaluation client constructor.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_syntax#spread_in_object_literals

packages/node/src/assignment/assignment-filter.ts

bgiori · 2023-07-29T05:20:37Z

packages/node/test/local/assignment/assignment-filter.test.ts

+import { sleep } from 'src/util/time';
+import { MockAssignmentFilter } from 'test/local/util/mockAssignmentFilter';
+
+test('filter - single assignment', async () => {


I'm skimming the tests, just note:
Please add more tests if you don't understand some part of the code and want to make sure it's doing what you expect

You will have to fix all the bugs that eventually make it into production :)

bgiori · 2023-07-29T05:23:22Z

packages/node/test/local/util/mockAssignmentFilter.ts

+import { Assignment, AssignmentFilter } from 'src/assignment/assignment';
+import { Cache } from 'src/util/cache';
+
+export class MockAssignmentFilter implements AssignmentFilter {


Can you explain why we need an explicit mock class not just using jest?

bgiori · 2023-07-29T05:24:30Z

packages/node/test/local/client.test.ts

+  });
+  await client.start();
+  await client.evaluate({ user_id: 'tim.yiu@amplitude.com' });
+  await sleep(4000);


Whats the purpose of these 4000 sleeps?

bgiori · 2023-07-29T05:24:33Z

packages/node/test/local/client.test.ts

+  const client = new LocalEvaluationClient(apiKey, {});
+  await client.start();
+  await client.evaluate({ user_id: 'tim.yiu@amplitude.com' });
+  await sleep(4000);


Whats the purpose of these 4000 sleeps?

packages/node/src/util/cache.ts

…Service construction and AssignmentConfiguration in local client.ts

bgiori

Looks awesome 🥇

Couple of comments, please implement assignment filtering and export the new config before merging.

bgiori · 2023-08-04T00:35:37Z

packages/node/src/types/config.ts

 };

+export type AssignmentConfiguration = {


You need to export this & NodeOptions in the index.ts file.

bgiori · 2023-08-04T00:44:14Z

packages/node/src/local/client.ts

@@ -85,6 +113,7 @@ export class LocalEvaluationClient {
      this.flags,
    );
    const results: Results = evaluation.evaluate(this.flags, user);
+    void this.assignmentService?.track(new Assignment(user, results));


filter results before passing to track.

Oh, last thing, we always want to include flags with type mutual-exclusion-group or holdout-group in the assignment.

This is a bit of a hack until we add topological sorting before evaluation in this SDK also :)

bgiori and others added 3 commits July 6, 2023 14:07

feat: automatic assignment tracking

e1f679c

feat: automatic assignment tracking

2dd1794

docs: removed comments and added description to assignment filterCapa…

f39a1cd

…city

tyiuhc requested a review from bgiori July 17, 2023 16:46

tyiuhc changed the title ~~Auto-assignment tracking~~ feat: auto-assignment tracking Jul 17, 2023

tyiuhc changed the title ~~feat: auto-assignment tracking~~ feat: Auto-assignment tracking Jul 17, 2023

bgiori reviewed Jul 18, 2023

View reviewed changes

Tim Yiu added 4 commits July 18, 2023 11:28

refactor: updated assignment filter and cache with time-to-live logic

76e5cd3

update AssignmentConfiguration to extend NodeOptions

b7ad7ba

minor filter update - current time no longer stored as cache item value

8d1bc38

reverted change to FlagResult payload types

237ccd4

bgiori requested changes Jul 29, 2023

View reviewed changes

Tim Yiu added 2 commits July 31, 2023 09:58

improved syntax, removed moveAssignmentFilter, reformatted Assignment…

801729e

…Service construction and AssignmentConfiguration in local client.ts

Improved syntax for createAssignmentService

89507ca

bgiori approved these changes Aug 4, 2023

View reviewed changes

only track assignments for selected flags

261413a

bgiori approved these changes Aug 8, 2023

View reviewed changes

renamed assignmentConfig option from filterCapacity to cacheCapacity

bcfe571

tyiuhc merged commit 6866739 into main Aug 8, 2023

tyiuhc deleted the assignment-tracking-2 branch August 8, 2023 22:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Auto-assignment tracking #22

feat: Auto-assignment tracking #22

tyiuhc commented Jul 17, 2023

bgiori left a comment

bgiori Jul 17, 2023

bgiori left a comment

bgiori Jul 29, 2023

bgiori Jul 29, 2023

bgiori Jul 29, 2023

bgiori Jul 29, 2023

bgiori Jul 29, 2023

bgiori Jul 29, 2023

bgiori Jul 29, 2023

bgiori Jul 29, 2023

bgiori Jul 29, 2023

bgiori left a comment

bgiori Aug 4, 2023

bgiori Aug 4, 2023

bgiori Aug 4, 2023

		let sb =
		this.user.user_id?.trim() + ' ' + this.user.device_id?.trim() + ' ';

	let sb =
	this.user.user_id?.trim() + ' ' + this.user.device_id?.trim() + ' ';
	let canonical = `${this.user.user_id?.trim()} ${this.user.device_id?.trim()} `;

feat: Auto-assignment tracking #22

feat: Auto-assignment tracking #22

Conversation

tyiuhc commented Jul 17, 2023

Summary

Checklist

bgiori left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgiori 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

bgiori 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