-
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 3 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,25 @@ | ||
import { Assignment, AssignmentFilter } from 'src/assignment/assignment'; | ||
import { DAY_MILLIS } from 'src/assignment/assignment-service'; | ||
import { LRUCache } from 'src/util/lrucache'; | ||
|
||
export const DEFAULT_FILTER_CAPACITY = 65536; | ||
|
||
export class LRUAssignmentFilter implements AssignmentFilter { | ||
private readonly cache: LRUCache<number>; | ||
|
||
constructor(size: number) { | ||
this.cache = new LRUCache<number>(size); | ||
} | ||
|
||
public shouldTrack(assignment: Assignment): boolean { | ||
bgiori marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const now = Date.now(); | ||
const canonicalAssignment = assignment.canonicalize(); | ||
const lastSent = this.cache.get(canonicalAssignment); | ||
if (lastSent == null || now > lastSent + DAY_MILLIS) { | ||
this.cache.set(canonicalAssignment, now); | ||
return true; | ||
} else { | ||
return false; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
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; | ||
|
||
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].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, | ||
LRUAssignmentFilter, | ||
} 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,21 @@ export class LocalEvaluationClient { | |
this.config.flagConfigPollingIntervalMillis, | ||
this.config.debug, | ||
); | ||
|
||
if (config.assignmentConfiguration) { | ||
const instance = amplitude.createInstance(); | ||
instance.init( | ||
config.assignmentConfiguration.apiKey, | ||
config.assignmentConfiguration, | ||
); | ||
const filterCapacity = config.assignmentConfiguration?.filterCapacity | ||
? config.assignmentConfiguration?.filterCapacity | ||
: DEFAULT_FILTER_CAPACITY; | ||
this.assignmentService = new AmplitudeAssignmentService( | ||
instance, | ||
new LRUAssignmentFilter(filterCapacity), | ||
); | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -85,6 +108,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,7 @@ | ||
import https from 'https'; | ||
|
||
import { NodeConfig } from '@amplitude/analytics-node/lib/cjs/config'; | ||
|
||
import { FlagConfig } from './flag'; | ||
|
||
/** | ||
|
@@ -140,8 +142,23 @@ 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 maximum number of assignments stored in the assignment cache | ||
* | ||
* Default: 65536 | ||
*/ | ||
filterCapacity?: number; | ||
} & NodeConfig; | ||
|
||
bgiori marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/** | ||
Defaults for {@link LocalEvaluationConfig} options. | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
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. Why has payload been converted to a string here? |
||
isDefaultVariant: boolean; | ||
expKey: string | null | undefined; | ||
deployed: boolean; | ||
type: string; | ||
|
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; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
// Define an interface for the cache items with key-value pairs | ||
interface CacheItem<T> { | ||
key: string; | ||
value: T; | ||
} | ||
|
||
export class LRUCache<T> { | ||
private readonly maxSize: number; | ||
private cache: Map<string, CacheItem<T>>; | ||
|
||
constructor(maxSize: number) { | ||
this.maxSize = maxSize; | ||
this.cache = new Map<string, CacheItem<T>>(); | ||
} | ||
|
||
set(key: string, value: T): void { | ||
if (this.cache.size >= this.maxSize) { | ||
const lruKey = this.cache.keys().next().value; | ||
this.cache.delete(lruKey); | ||
} | ||
this.cache.set(key, { key, value }); | ||
} | ||
|
||
get(key: string): T | undefined { | ||
const item = this.cache.get(key); | ||
if (item) { | ||
this.cache.delete(key); | ||
this.cache.set(key, item); | ||
return item.value; | ||
} | ||
return undefined; | ||
} | ||
|
||
delete(key: string): void { | ||
this.cache.delete(key); | ||
} | ||
|
||
clear(): void { | ||
this.cache.clear(); | ||
} | ||
} |
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