-
Notifications
You must be signed in to change notification settings - Fork 3
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: Accumulate Events #65
Changes from 3 commits
ee49b2a
d07079d
ccece9b
f1da631
06ef5a9
525234c
b8cde72
882075f
6a59727
dd6a6cb
1802c4e
9e62d8f
ded115c
719519c
c459dcb
d23fcb6
db97c66
a28bc36
5791d84
98a7ab2
90b37ba
eb5ae38
f638eb8
1238a5f
6bd68c6
7c07a7d
4fe767b
9eea9a5
2722ec2
8eb34e9
d34928b
772018d
2313806
8c4f560
c999b06
fbaa4f3
dfc72df
c2c4277
a0242f9
ab218ab
3549175
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,8 @@ | ||
root=true | ||
|
||
[*] | ||
end_of_line = lf | ||
insert_final_newline = true | ||
charset = utf-8 | ||
indent_style = space | ||
indent_size = 2 |
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,99 @@ | ||||||||
import type { CountlyWebSdk, IEventAccumulator, CountlyEvent, CountlyEventData } from 'countly-sdk-web' | ||||||||
|
||||||||
const eventDefaults: CountlyEventData = { | ||||||||
key: '', | ||||||||
count: 1, | ||||||||
sum: 1, | ||||||||
dur: Date.now(), | ||||||||
segmentation: {} | ||||||||
} | ||||||||
|
||||||||
interface eventStore { | ||||||||
eventData: CountlyEventData | ||||||||
startTime: number | ||||||||
timeout: NodeJS.Timeout | ||||||||
} | ||||||||
|
||||||||
/** | ||||||||
* EventAccumulator is a class that accumulates events and flushes them to the Countly server. | ||||||||
*/ | ||||||||
export class EventAccumulator implements IEventAccumulator { | ||||||||
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.
Suggested change
|
||||||||
private readonly metricsService: CountlyWebSdk | ||||||||
private readonly events: Record<string, eventStore> | ||||||||
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.
Suggested change
|
||||||||
private readonly flushInterval: number | ||||||||
|
||||||||
/** | ||||||||
* Create a new EventAccumulator | ||||||||
* | ||||||||
* @param {CountlyWebSdk} metricsService - instance | ||||||||
* @param {number} flushInterval - in milliseconds | ||||||||
*/ | ||||||||
constructor (metricsService: CountlyWebSdk, flushInterval: number = 5 * 60 * 1000) { | ||||||||
this.metricsService = metricsService | ||||||||
this.flushInterval = flushInterval | ||||||||
this.events = {} | ||||||||
whizzzkid marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
} | ||||||||
|
||||||||
/** | ||||||||
* Add an event to the accumulator | ||||||||
* | ||||||||
* @param {CountlyEvent} event - event to add | ||||||||
* @param {boolean} flush - optionally whether to flush the event immediately | ||||||||
*/ | ||||||||
addEvent (event: CountlyEvent, flush: boolean = false): void { | ||||||||
// create a new event object with defaults. | ||||||||
const newEvent: CountlyEventData = { ...eventDefaults, ...event } | ||||||||
const { key, count } = newEvent | ||||||||
|
||||||||
// validate event | ||||||||
if (key === '') { | ||||||||
throw new Error('Event key is required') | ||||||||
} | ||||||||
|
||||||||
// if event is not in the store, add it. | ||||||||
if (!(key in this.events)) { | ||||||||
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.
Suggested change
|
||||||||
this.events[key] = { | ||||||||
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.
Suggested change
|
||||||||
eventData: newEvent, | ||||||||
// set start time to now. This will be updated when the event is flushed. | ||||||||
startTime: Date.now(), | ||||||||
// set a timeout to flush the event after the flush interval. | ||||||||
timeout: setTimeout(() => { | ||||||||
this.flush(key) | ||||||||
}, this.flushInterval) | ||||||||
} | ||||||||
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.
Suggested change
|
||||||||
} else { | ||||||||
// if event is in the store, update the event data. | ||||||||
const { eventData } = this.events[key] | ||||||||
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.
Suggested change
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. fixed. |
||||||||
eventData.count += count | ||||||||
eventData.sum += 1 | ||||||||
eventData.segmentation = { ...eventData.segmentation, ...newEvent.segmentation } | ||||||||
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.
Suggested change
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 do I need to set it back? Map.get should get me reference to the object, modifying the object updates the referenced object. |
||||||||
} | ||||||||
whizzzkid marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
|
||||||||
// flush the event if flush is true. | ||||||||
if (flush) { | ||||||||
this.flush(key) | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
/** | ||||||||
* Flush an event from the accumulator | ||||||||
* | ||||||||
* @param {string} key - event key | ||||||||
*/ | ||||||||
flush (key: string): void { | ||||||||
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. should we have a flush_all? 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. added with tests |
||||||||
// if event is not in the store, return. | ||||||||
if (!(key in this.events)) { | ||||||||
return | ||||||||
} | ||||||||
|
||||||||
const { eventData, startTime, timeout } = this.events[key] | ||||||||
|
||||||||
// update duration to ms from start. | ||||||||
eventData.dur = Date.now() - startTime | ||||||||
whizzzkid marked this conversation as resolved.
Show resolved
Hide resolved
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. I think we should remove duration from the submitted event though, because countly doesn't really display them the way we think they should.. maybe we should chat about this while taking a peek at existing webui data in our countly server |
||||||||
// add event to the async queue. | ||||||||
this.metricsService.q.push(['add_event', eventData]) | ||||||||
clearTimeout(timeout) | ||||||||
// eslint-disable-next-line @typescript-eslint/no-dynamic-delete | ||||||||
delete this.events[key] | ||||||||
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. if we use a map for 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. yep |
||||||||
} | ||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,23 @@ | ||
import { COUNTLY_SETUP_DEFAULTS } from './config.js' | ||
import { EventAccumulator } from './EventAccumulator.js' | ||
|
||
import type { metricFeatures, CountlyWebSdk } from 'countly-sdk-web' | ||
import type { CountlyNodeSdk } from 'countly-sdk-nodejs' | ||
import type { CountlyWebSdk, metricFeatures } from 'countly-sdk-web' | ||
import type { consentTypes, consentTypesExceptAll } from './types/index.js' | ||
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 should update eslint so it's organizing imports instead of depending on your editor for that! otherwise they'll keep getting out of order ;) speaking of ordering imports. Can we group the imports? we've currently got two relative imports at the top, then 2 3p imports (countly-sdk-*), then another relative. |
||
|
||
export interface MetricsProviderConstructorOptions<T> { | ||
appKey: string | ||
autoTrack?: boolean | ||
interval?: number | ||
max_events?: number | ||
metricsService: T | ||
queue_size?: number | ||
session_update?: number | ||
url?: string | ||
metricsService: T | ||
} | ||
|
||
export default class MetricsProvider<T extends CountlyWebSdk & CountlyNodeSdk> { | ||
public readonly accumulate: EventAccumulator | ||
private readonly groupedFeatures: Record<consentTypes, metricFeatures[]> = this.mapAllEvents({ | ||
minimal: ['sessions', 'views', 'events'], | ||
performance: ['crashes', 'apm'], | ||
|
@@ -40,6 +42,7 @@ export default class MetricsProvider<T extends CountlyWebSdk & CountlyNodeSdk> { | |
url, | ||
require_consent: true | ||
}) | ||
this.accumulate = new EventAccumulator(metricsService) | ||
|
||
this.metricsService.init(serviceConfig) | ||
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. we need to revert this. appKey is not a valid argument for metricsService.init 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. it's not typechecked as expected, I fixed it like: const { appKey, ...remainderConfig } = config
const serviceConfig = {
...COUNTLY_SETUP_DEFAULTS,
...remainderConfig,
app_key: appKey
} I should throw if 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. yea we should. we can handle in a separate PR though. |
||
this.metricsService.group_features(this.groupedFeatures) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,12 @@ | ||
declare module 'countly-sdk-web' { | ||
interface CountlyEventData { | ||
export interface CountlyEventData { | ||
key: string | ||
count: number | ||
sum: number | ||
segmentation: Record<string, string | number> | ||
dur: number | ||
segmentation: Segments | ||
} | ||
interface CountlyEvent { | ||
export interface CountlyEvent { | ||
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. 👍 |
||
// name or id of the event | ||
key: string | ||
// how many times did event occur | ||
|
@@ -18,12 +19,18 @@ declare module 'countly-sdk-web' { | |
segmentation?: Segments | ||
} | ||
|
||
export interface IEventAccumulator { | ||
addEvent: (event: CountlyEvent, flush: boolean) => void | ||
flush: (key: string) => void | ||
} | ||
|
||
export type metricFeatures = 'apm' | 'attribution' | 'clicks' | 'crashes' | 'events' | 'feedback' | 'forms' | | ||
'location' | 'scrolls' | 'sessions' | 'star-rating' | 'users' | 'views' | ||
type Segments = Record<string, string> | ||
type IgnoreList = Array<string | RegExp> | ||
type CountlyEventQueueItem = [string, CountlyEventData] | [eventName: string, key: string] | [eventName: string] | ||
export interface CountlyWebSdk { | ||
accumulate: IEventAccumulator | ||
group_features: (arg0: Record<import('./index.js').consentTypes, metricFeatures[]>) => unknown | ||
check_consent: (consentFeature: metricFeatures | import('./index.js').consentTypes) => boolean | ||
add_consent: (consentFeature: import('./index.js').consentTypes | Array<import('./index.js').consentTypes>) => void | ||
|
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.
duration should be an amount of time, not a timestamp. This duration is currently going to be the full time since 1970.
We should probably default this to
0
or even leave it unset entirely.see
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.
this gets reset before the event gets flushed. setting default to 0.