-
Notifications
You must be signed in to change notification settings - Fork 5
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.
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
feat: Auto-assignment tracking #22
Changes from 7 commits
e1f679c
2dd1794
f39a1cd
76e5cd3
b7ad7ba
8d1bc38
237ccd4
801729e
89507ca
261413a
bcfe571
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
import { Assignment, AssignmentFilter } from 'src/assignment/assignment'; | ||
import { DAY_MILLIS } from 'src/assignment/assignment-service'; | ||
import { Cache } from 'src/util/cache'; | ||
|
||
export const DEFAULT_FILTER_CAPACITY = 65536; | ||
|
||
export class InMemoryAssignmentFilter implements AssignmentFilter { | ||
private readonly cache: Cache<number>; | ||
|
||
constructor(size: number) { | ||
this.cache = new Cache<number>(size, DAY_MILLIS); | ||
} | ||
|
||
public shouldTrack(assignment: Assignment): boolean { | ||
bgiori marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const canonicalAssignment = assignment.canonicalize(); | ||
const track = this.cache.get(canonicalAssignment) == null; | ||
if (track) { | ||
this.cache.put(canonicalAssignment, 0); | ||
} | ||
return track; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
import { BaseEvent } from '@amplitude/analytics-types'; | ||
import { CoreClient } from '@amplitude/analytics-types'; | ||
import { | ||
Assignment, | ||
AssignmentFilter, | ||
AssignmentService, | ||
} from 'src/assignment/assignment'; | ||
import { hashCode } from 'src/util/hash'; | ||
|
||
export const DAY_MILLIS = 24 * 60 * 60 * 1000; | ||
const FLAG_TYPE_MUTUAL_EXCLUSION_GROUP = 'mutual-exclusion-group'; | ||
|
||
export class AmplitudeAssignmentService implements AssignmentService { | ||
private readonly amplitude: CoreClient; | ||
private readonly assignmentFilter: AssignmentFilter; | ||
|
||
constructor(amplitude: CoreClient, assignmentFilter: AssignmentFilter) { | ||
this.amplitude = amplitude; | ||
this.assignmentFilter = assignmentFilter; | ||
} | ||
|
||
async track(assignment: Assignment): Promise<void> { | ||
if (this.assignmentFilter.shouldTrack(assignment)) { | ||
bgiori marked this conversation as resolved.
Show resolved
Hide resolved
|
||
this.amplitude.logEvent(this.toEvent(assignment)); | ||
bgiori marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
public toEvent(assignment: Assignment): BaseEvent { | ||
const event: BaseEvent = { | ||
event_type: '[Experiment] Assignment', | ||
user_id: assignment.user.user_id, | ||
device_id: assignment.user.device_id, | ||
event_properties: {}, | ||
user_properties: {}, | ||
}; | ||
|
||
for (const resultsKey in assignment.results) { | ||
event.event_properties[`${resultsKey}.variant`] = | ||
assignment.results[resultsKey].value; | ||
} | ||
|
||
const set = {}; | ||
const unset = {}; | ||
for (const resultsKey in assignment.results) { | ||
if ( | ||
assignment.results[resultsKey].type == FLAG_TYPE_MUTUAL_EXCLUSION_GROUP | ||
) { | ||
continue; | ||
} else if (assignment.results[resultsKey].isDefaultVariant) { | ||
unset[`[Experiment] ${resultsKey}`] = '-'; | ||
} else { | ||
set[`[Experiment] ${resultsKey}`] = | ||
assignment.results[resultsKey].value; | ||
} | ||
} | ||
event.user_properties['$set'] = set; | ||
event.user_properties['$unset'] = unset; | ||
|
||
event.insert_id = `${event.user_id} ${event.device_id} ${hashCode( | ||
assignment.canonicalize(), | ||
)} ${assignment.timestamp / DAY_MILLIS}`; | ||
return event; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,31 @@ | ||||||||
import { ExperimentUser } from 'src/types/user'; | ||||||||
import { Results } from 'src/types/variant'; | ||||||||
|
||||||||
export class Assignment { | ||||||||
public user: ExperimentUser; | ||||||||
public results: Results; | ||||||||
public timestamp: number = Date.now(); | ||||||||
|
||||||||
public constructor(user: ExperimentUser, results: Results) { | ||||||||
this.user = user; | ||||||||
this.results = results; | ||||||||
} | ||||||||
|
||||||||
public canonicalize(): string { | ||||||||
let sb = | ||||||||
this.user.user_id?.trim() + ' ' + this.user.device_id?.trim() + ' '; | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: You can use a template string to start with, and you should use a different variable name,
Suggested change
|
||||||||
for (const key of Object.keys(this.results).sort()) { | ||||||||
const value = this.results[key]; | ||||||||
sb += key.trim() + ' ' + value?.value?.trim() + ' '; | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use template string here. |
||||||||
} | ||||||||
return sb; | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
export interface AssignmentService { | ||||||||
track(assignment: Assignment): Promise<void>; | ||||||||
} | ||||||||
|
||||||||
export interface AssignmentFilter { | ||||||||
shouldTrack(assignment: Assignment): boolean; | ||||||||
} | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Absolute super minor nit: I prefer interface definitions at the top of the file :) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,11 @@ | ||
import * as amplitude from '@amplitude/analytics-node'; | ||
import evaluation from '@amplitude/evaluation-js'; | ||
import { Assignment, AssignmentService } from 'src/assignment/assignment'; | ||
import { | ||
DEFAULT_FILTER_CAPACITY, | ||
InMemoryAssignmentFilter, | ||
} from 'src/assignment/assignment-filter'; | ||
import { AmplitudeAssignmentService } from 'src/assignment/assignment-service'; | ||
|
||
import { FetchHttpClient } from '../transport/http'; | ||
import { | ||
|
@@ -25,6 +32,7 @@ export class LocalEvaluationClient { | |
private readonly config: LocalEvaluationConfig; | ||
private readonly poller: FlagConfigPoller; | ||
private flags: FlagConfig[]; | ||
private readonly assignmentService: AssignmentService; | ||
|
||
/** | ||
* Directly access the client's flag config cache. | ||
|
@@ -60,6 +68,26 @@ export class LocalEvaluationClient { | |
this.config.flagConfigPollingIntervalMillis, | ||
this.config.debug, | ||
); | ||
this.assignmentService = this.createAssignmentService(); | ||
} | ||
|
||
private createAssignmentService(): AssignmentService | null { | ||
if (this.config.assignmentConfiguration) { | ||
bgiori marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const instance = amplitude.createInstance(); | ||
instance.init( | ||
this.config.assignmentConfiguration.apiKey, | ||
this.config.assignmentConfiguration, | ||
); | ||
const filterCapacity = this.config.assignmentConfiguration?.filterCapacity | ||
? this.config.assignmentConfiguration?.filterCapacity | ||
: DEFAULT_FILTER_CAPACITY; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use object spreading to merge the actual input configuration with the default configuration if it exists. My logic here is hard to follow so let's pair on Monday to implement a more sane and javascript'y configuration logic. |
||
const assignmentService = new AmplitudeAssignmentService( | ||
instance, | ||
new InMemoryAssignmentFilter(filterCapacity), | ||
); | ||
return assignmentService; | ||
} | ||
return null; | ||
} | ||
|
||
/** | ||
|
@@ -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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. filter results before passing to track. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, last thing, we always want to include flags with type This is a bit of a hack until we add topological sorting before evaluation in this SDK also :) |
||
const variants: Variants = {}; | ||
const filter = flagKeys && flagKeys.length > 0; | ||
for (const flagKey in results) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,9 @@ | ||
import https from 'https'; | ||
|
||
import { NodeOptions } from '@amplitude/analytics-types'; | ||
|
||
import { LocalEvaluationClient } from '..'; | ||
|
||
import { FlagConfig } from './flag'; | ||
|
||
/** | ||
|
@@ -140,8 +144,27 @@ export type LocalEvaluationConfig = { | |
* The agent used to send http requests. | ||
*/ | ||
httpAgent?: https.Agent; | ||
|
||
/** | ||
* Configuration for automatically tracking assignment events after an | ||
* evaluation. | ||
*/ | ||
assignmentConfiguration?: AssignmentConfiguration; | ||
}; | ||
|
||
export type AssignmentConfiguration = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You need to export this & |
||
/** | ||
* The analytics API key and NOT the experiment deployment key | ||
*/ | ||
apiKey: string; | ||
/** | ||
* The maximum number of assignments stored in the assignment cache | ||
* | ||
* Default: 65536 | ||
*/ | ||
filterCapacity?: number; | ||
} & NodeOptions; | ||
|
||
bgiori marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/** | ||
Defaults for {@link LocalEvaluationConfig} options. | ||
|
||
|
@@ -160,4 +183,5 @@ export const LocalEvaluationDefaults: LocalEvaluationConfig = { | |
bootstrap: {}, | ||
flagConfigPollingIntervalMillis: 30000, | ||
httpAgent: null, | ||
assignmentConfiguration: null, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,113 @@ | ||
class ListNode<T> { | ||
prev: ListNode<T> | null; | ||
next: ListNode<T> | null; | ||
data: T; | ||
|
||
constructor(data: T) { | ||
this.prev = null; | ||
this.next = null; | ||
this.data = data; | ||
} | ||
} | ||
|
||
interface CacheItem<T> { | ||
key: string; | ||
value: T; | ||
createdAt: number; | ||
} | ||
|
||
export class Cache<T> { | ||
private readonly capacity: number; | ||
private readonly ttlMillis: number; | ||
private cache: Map<string, ListNode<CacheItem<T>>>; | ||
private head: ListNode<CacheItem<T>> | null; | ||
private tail: ListNode<CacheItem<T>> | null; | ||
|
||
constructor(capacity: number, ttlMillis: number) { | ||
this.capacity = capacity; | ||
this.ttlMillis = ttlMillis; | ||
this.cache = new Map(); | ||
this.head = null; | ||
this.tail = null; | ||
} | ||
|
||
put(key: string, value: T): void { | ||
if (this.cache.has(key)) { | ||
this.removeFromList(key); | ||
} else if (this.cache.size >= this.capacity) { | ||
this.evictLRU(); | ||
} | ||
|
||
const cacheItem: CacheItem<T> = { | ||
key, | ||
value, | ||
createdAt: Date.now(), | ||
}; | ||
|
||
const node = new ListNode(cacheItem); | ||
this.cache.set(key, node); | ||
this.insertToList(node); | ||
} | ||
|
||
get(key: string): T | undefined { | ||
const node = this.cache.get(key); | ||
if (node) { | ||
const timeElapsed = Date.now() - node.data.createdAt; | ||
if (timeElapsed > this.ttlMillis) { | ||
this.remove(key); | ||
return undefined; | ||
} | ||
|
||
this.removeFromList(key); | ||
this.insertToList(node); | ||
return node.data.value; | ||
} | ||
return undefined; | ||
} | ||
bgiori marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
remove(key: string): void { | ||
this.removeFromList(key); | ||
this.cache.delete(key); | ||
} | ||
|
||
clear(): void { | ||
this.cache.clear(); | ||
this.head = null; | ||
this.tail = null; | ||
} | ||
|
||
private evictLRU(): void { | ||
if (this.head) { | ||
this.remove(this.head.data.key); | ||
} | ||
} | ||
|
||
private removeFromList(key: string): void { | ||
const node = this.cache.get(key); | ||
if (node) { | ||
if (node.prev) { | ||
node.prev.next = node.next; | ||
} else { | ||
this.head = node.next; | ||
} | ||
|
||
if (node.next) { | ||
node.next.prev = node.prev; | ||
} else { | ||
this.tail = node.prev; | ||
} | ||
} | ||
} | ||
|
||
private insertToList(node: ListNode<CacheItem<T>>): void { | ||
if (this.tail) { | ||
this.tail.next = node; | ||
node.prev = this.tail; | ||
node.next = null; | ||
this.tail = node; | ||
} else { | ||
this.head = node; | ||
this.tail = node; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
export function hashCode(s: string): number { | ||
let hash = 0, | ||
i, | ||
chr, | ||
len; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is very weird syntax.... I dont like it, can we do something more legible? |
||
if (s.length === 0) return hash; | ||
for (i = 0, len = s.length; i < len; i++) { | ||
chr = s.charCodeAt(i); | ||
hash = (hash << 5) - hash + chr; | ||
hash |= 0; | ||
} | ||
return hash; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theconfig.ts
file. Then object spread the input with the defaults (as we do with the base configuration) in local evaluation client constructor.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_syntax#spread_in_object_literals