Skip to content
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

Merged
merged 41 commits into from
Jan 27, 2023
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
ee49b2a
feat: editor config, sane!
whizzzkid Jan 19, 2023
d07079d
Add event accumulator class
whizzzkid Jan 19, 2023
ccece9b
feat: Update types and hookup accumulator
whizzzkid Jan 19, 2023
f1da631
fix: configs
whizzzkid Jan 20, 2023
06ef5a9
feat: moving types to set rootDir
whizzzkid Jan 20, 2023
525234c
adding test folder
whizzzkid Jan 20, 2023
b8cde72
adding relevant types
whizzzkid Jan 20, 2023
882075f
types migration
whizzzkid Jan 20, 2023
6a59727
fix: add_event
whizzzkid Jan 20, 2023
dd6a6cb
feat: adding accumulator test.
whizzzkid Jan 20, 2023
1802c4e
breaking down logic
whizzzkid Jan 20, 2023
9e62d8f
fix: import order
whizzzkid Jan 20, 2023
ded115c
fix: sinon stubs
whizzzkid Jan 20, 2023
719519c
feat: Adding impl using map
whizzzkid Jan 20, 2023
c459dcb
adding flushAll method
whizzzkid Jan 20, 2023
d23fcb6
Hooking up to the beforunload event
whizzzkid Jan 20, 2023
db97c66
lint
whizzzkid Jan 20, 2023
a28bc36
Merge branch 'main' into feat/accumulate-events
whizzzkid Jan 21, 2023
5791d84
fix: :twisted_rightwards_arrows: merged with upstream
whizzzkid Jan 21, 2023
98a7ab2
fix: :package: adding missing package after merge
whizzzkid Jan 21, 2023
90b37ba
fix: :test_tube: EventAccumulator
whizzzkid Jan 21, 2023
eb5ae38
fix: :adhesive_bandage: fix imports
whizzzkid Jan 21, 2023
f638eb8
fix: :truck: rename to spec.ts
whizzzkid Jan 21, 2023
1238a5f
fix: :recycle: cleanup
whizzzkid Jan 21, 2023
6bd68c6
fix: :package: package-lock.json
whizzzkid Jan 21, 2023
7c07a7d
fix: :pencil2: spec naming and scope
whizzzkid Jan 23, 2023
4fe767b
Merge branch 'main' into feat/accumulate-events
whizzzkid Jan 25, 2023
9eea9a5
fix: :wrench: don't disable rules globally
whizzzkid Jan 25, 2023
2722ec2
fix: :wrench: only apply eslint overrides to spec files.
whizzzkid Jan 25, 2023
8eb34e9
fix: :rotating_light: fixes lint.
whizzzkid Jan 26, 2023
d34928b
fix: :recycle: Moving tests to node folder
whizzzkid Jan 26, 2023
772018d
fix: :label: generics
whizzzkid Jan 26, 2023
2313806
fix: :necktie: instantiating at declaration
whizzzkid Jan 26, 2023
8c4f560
fix: :memo: adding missing documentation
whizzzkid Jan 26, 2023
c999b06
fix: :necktie: fixing unload event to handle both browser and node.
whizzzkid Jan 26, 2023
fbaa4f3
fix: :truck: digest -> accumulate
whizzzkid Jan 26, 2023
dfc72df
fix: generics
whizzzkid Jan 26, 2023
c2c4277
fix: :pencil2: dupe imports
whizzzkid Jan 26, 2023
a0242f9
fix: fixing appKey
whizzzkid Jan 26, 2023
ab218ab
feat: :zap: useFakeTimers instead of await
whizzzkid Jan 26, 2023
3549175
fix: :recycle: move call order
whizzzkid Jan 26, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@
"sourceType": "module"
},
"rules": {
"sort-imports": ["error"],
"@typescript-eslint/no-unused-expressions": "off"
"sort-imports": ["error"]
},
"overrides": [
{
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@
"release": "aegir release",
"build": "aegir build",
"test": "run-s test:*",
"test:node": "aegir test --target node --files 'test/**/*.spec.ts'",
"test:node": "aegir test --target node --files 'test/node/**/*.spec.ts'",
"storybook": "start-storybook -p 6006",
"build-storybook": "build-storybook"
},
Expand Down
26 changes: 17 additions & 9 deletions src/EventAccumulator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ interface eventStore {
/**
* EventAccumulator is a class that accumulates events and flushes them to the Countly server.
*/
export class EventAccumulator implements IEventAccumulator {
private readonly metricsService: CountlyWebSdk | CountlyNodeSdk
private readonly events: Map<string, eventStore>
export class EventAccumulator<T extends CountlyWebSdk | CountlyNodeSdk> implements IEventAccumulator {
private readonly metricsService: T
private readonly events: Map<string, eventStore> = new Map()
private readonly flushInterval: number

/**
Expand All @@ -29,17 +29,25 @@ export class EventAccumulator implements IEventAccumulator {
* @param {CountlyWebSdk} metricsService - instance
* @param {number} flushInterval - in milliseconds
*/
constructor (metricsService: CountlyWebSdk | CountlyNodeSdk, flushInterval: number = 5 * 60 * 1000) {
constructor (metricsService: T, flushInterval: number = 5 * 60 * 1000) {
this.metricsService = metricsService
this.flushInterval = flushInterval
this.events = new Map()
this.setupUnloadEvent()
}

/**
* Setup the beforeunload event to flush all events.
*/
private setupUnloadEvent (): void {
globalThis.addEventListener('beforeunload', () => {
const flushAllHandler = (): void => {
this.flushAll()
})
}

if (typeof globalThis.addEventListener === 'function') {
globalThis.addEventListener('beforeunload', flushAllHandler)
} else if (typeof globalThis.process?.on === 'function') {
globalThis.process.on('beforeExit', flushAllHandler)
}
}

/**
Expand Down Expand Up @@ -75,7 +83,7 @@ export class EventAccumulator implements IEventAccumulator {
*
* @param {CountlyEventData} newEventData - event data
*/
private digestEventData (newEventData: CountlyEventData): void {
private accumulateEventData (newEventData: CountlyEventData): void {
const { key, count, segmentation } = newEventData
// if event is in the store, update the event data.
const eventStore = this.events.get(key)
Expand Down Expand Up @@ -104,7 +112,7 @@ export class EventAccumulator implements IEventAccumulator {
throw new Error('Event key is required.')
}

this.digestEventData(eventData)
this.accumulateEventData(eventData)

// flush the event if flush is true.
if (flush) {
Expand Down
19 changes: 13 additions & 6 deletions src/MetricsProvider.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
import type { CountlyWebSdk, metricFeatures } from 'countly-sdk-web'
import type {
CountlyEvent,
CountlyWebSdk,
IgnoreList,
Segments,
metricFeatures
} from 'countly-sdk-web'
import type { consentTypes, consentTypesExceptAll } from '../types/index.js'
import { COUNTLY_SETUP_DEFAULTS } from './config.js'

import type { CountlyNodeSdk } from 'countly-sdk-nodejs'
import { EventAccumulator } from './EventAccumulator.js'
import type { StorageProvider } from './StorageProvider.js'
Expand All @@ -18,7 +25,7 @@ export interface MetricsProviderConstructorOptions<T> {
}

export default class MetricsProvider<T extends CountlyWebSdk | CountlyNodeSdk> {
public readonly accumulate: EventAccumulator
public readonly accumulate: EventAccumulator<T>
private readonly groupedFeatures: Record<consentTypes, metricFeatures[]> = this.mapAllEvents({
minimal: ['sessions', 'views', 'events'],
performance: ['crashes', 'apm'],
Expand All @@ -34,17 +41,17 @@ export default class MetricsProvider<T extends CountlyWebSdk | CountlyNodeSdk> {
private readonly initDone: boolean = false

constructor (config: MetricsProviderConstructorOptions<T>) {
const { appKey, ...remainderConfig } = config
const serviceConfig = {
...COUNTLY_SETUP_DEFAULTS,
...config,
app_key: config.appKey
...remainderConfig,
app_key: appKey
}
const { autoTrack, metricsService, storageProvider } = serviceConfig
this.metricsService = metricsService
this.storageProvider = storageProvider ?? null
this.accumulate = new EventAccumulator(metricsService)
this.storageProvider = storageProvider ?? null
this.metricsService.init(serviceConfig)
Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 appKey is missing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea we should. we can handle in a separate PR though.

this.accumulate = new EventAccumulator(metricsService)
this.metricsService.group_features(this.groupedFeatures)

if (autoTrack) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,17 @@ import { expect, sinon } from '../testUtils.js'
import Countly from 'countly-sdk-nodejs'
import { EventAccumulator } from '../../src/EventAccumulator.js'

const sleep = async (ms: number): Promise<unknown> => await new Promise(resolve => setTimeout(resolve, ms))
const sandbox = sinon.createSandbox()
const addEventListenerStub = sandbox.stub()
globalThis.addEventListener = addEventListenerStub

describe('EventAccumulator', () => {
let countlyStub: sinon.SinonStubbedInstance<typeof Countly>
let clock: sinon.SinonFakeTimers

beforeEach(() => {
countlyStub = sandbox.stub(Countly)
clock = sandbox.useFakeTimers()
})

afterEach(() => {
Expand All @@ -28,18 +29,19 @@ describe('EventAccumulator', () => {
}
// add the event twice with a 100ms delay between them, this is needed to test the duration.
accumulator.addEvent(event)
await sleep(100)
await clock.tickAsync(100)
accumulator.addEvent(event)
// flush the event
accumulator.flush('test')
// validate the call count
expect(countlyStub.add_event.callCount).to.be.equal(1)
// check the event data
const { count, dur, key, sum, segmentation } = countlyStub.add_event.getCall(0).args[0]
expect(count).to.be.equal(2)
expect(dur).to.be.greaterThanOrEqual(100)
expect(key).to.be.equal('test')
expect(sum).to.be.equal(2)
expect(segmentation).to.be.deep.equal({})
expect(countlyStub.add_event.callCount).to.be.equal(1)
})

it('should flush events after the flush interval', async () => {
Expand All @@ -52,15 +54,16 @@ describe('EventAccumulator', () => {
}
accumulator.addEvent(event)
// wait for the flush interval to pass
await sleep(150)
await clock.tickAsync(150)
// validate the call count
expect(countlyStub.add_event.callCount).to.be.equal(1)
// check the event data
const { count, dur, key, sum, segmentation } = countlyStub.add_event.getCall(0).args[0]
expect(count).to.be.equal(1)
expect(dur).to.be.greaterThanOrEqual(100)
expect(key).to.be.equal('test')
expect(sum).to.be.equal(1)
expect(segmentation).to.be.deep.equal({})
expect(countlyStub.add_event.callCount).to.be.equal(1)
})

it('should flush events when flush=true', async () => {
Expand All @@ -73,16 +76,17 @@ describe('EventAccumulator', () => {
}
// add the event twice with a 100ms delay between them, this is needed to test the duration.
accumulator.addEvent(event)
await sleep(100)
await clock.tickAsync(100)
accumulator.addEvent(event, true)
// validate the call count
expect(countlyStub.add_event.callCount).to.be.equal(1)
// check the event data
const { count, dur, key, sum, segmentation } = countlyStub.add_event.getCall(0).args[0]
expect(count).to.be.equal(2)
expect(dur).to.be.greaterThanOrEqual(100)
expect(key).to.be.equal('test')
expect(sum).to.be.equal(2)
expect(segmentation).to.be.deep.equal({})
expect(countlyStub.add_event.callCount).to.be.equal(1)
})

it('should accumulate segments', async () => {
Expand All @@ -105,8 +109,10 @@ describe('EventAccumulator', () => {
}
// add the event twice with a 100ms delay between them, this is needed to test the duration.
accumulator.addEvent(event1)
await sleep(100)
await clock.tickAsync(100)
accumulator.addEvent(event2, true)
// validate the call count
expect(countlyStub.add_event.callCount).to.be.equal(1)
// check the event data
const { count, dur, key, sum, segmentation } = countlyStub.add_event.getCall(0).args[0]
expect(count).to.be.equal(2)
Expand All @@ -117,7 +123,6 @@ describe('EventAccumulator', () => {
foo: 'bar',
bar: 'baz'
})
expect(countlyStub.add_event.callCount).to.be.equal(1)
})

it('should accumulate different types of events', async () => {
Expand All @@ -140,10 +145,12 @@ describe('EventAccumulator', () => {
}
accumulator.addEvent(event1)
// adding the second event after 50ms so that the first event is flushed first
await sleep(50)
await clock.tickAsync(50)
accumulator.addEvent(event2)
// wait for the flush interval to pass
await sleep(150)
await clock.tickAsync(150)
// validate the call count
expect(countlyStub.add_event.callCount).to.be.equal(2)
// check the event data
const calls = [['test1', { foo: 'bar' }], ['test2', { bar: 'baz' }]]
calls.forEach(([testKey, segment], idx) => {
Expand All @@ -154,7 +161,6 @@ describe('EventAccumulator', () => {
expect(sum).to.be.equal(1)
expect(segmentation).to.be.deep.equals(segment)
})
expect(countlyStub.add_event.callCount).to.be.equal(2)
})

it('should flush all events from the accumulator', async () => {
Expand Down Expand Up @@ -186,11 +192,13 @@ describe('EventAccumulator', () => {

accumulator.addEvent(event1)
// adding the second event after 50ms so that the first event is flushed first
await sleep(50)
await clock.tickAsync(50)
accumulator.addEvent(event2)
await sleep(50)
await clock.tickAsync(50)
accumulator.addEvent(event3)
accumulator.flushAll()
// validate the call count
expect(countlyStub.add_event.callCount).to.be.equal(2)
// check the event data
const test1Results = countlyStub.add_event.getCall(0).args[0]
expect(test1Results.count).to.be.equal(3)
Expand All @@ -205,7 +213,5 @@ describe('EventAccumulator', () => {
expect(test2Results.key).to.be.equal('test2')
expect(test2Results.sum).to.be.equal(1)
expect(test2Results.segmentation).to.be.deep.equals({ bar: 'baz' })

expect(countlyStub.add_event.callCount).to.be.equal(2)
})
})
1 change: 1 addition & 0 deletions test/node/NodeMetricsProvider.spec.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* eslint-disable @typescript-eslint/no-unused-expressions */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we shouldn't need this one here if we're ignoring this rule in tests? but we probably shouldn't disable it completely in all tests.. I'm good with it either way.. eslint can be painful in test code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expect(telemetry).to.have.property('storageProvider').that.is.not.null

seems to drive eslint crazy, I can look at this later.

import { ensureCall, expect, sinon } from '../testUtils.js'
import CountlyNodeSdk from 'countly-sdk-nodejs'
import { NodeMetricsProvider } from '../../src/NodeMetricsProvider.js'
Expand Down