feat: Accumulate Events #65

whizzzkid · 2023-01-19T07:22:57Z

Closes: #61

In this PR:

implementing accumulating interface
Fixed types to accommodate accumulation of events.
Accumulated events also accumulate segments.
A single type of event can be flushed
All events can be flushed.
Automatic hookup to beforeunload event to prevent event loss.

SgtPooki

I know it's in progress but I looked anyway since you checked mine out

SgtPooki · 2023-01-20T08:09:54Z

src/types/countly.d.ts

+  export interface CountlyEventData {
    key: string
    count: number
    sum: number
-    segmentation: Record<string, string | number>
+    dur: number
+    segmentation: Segments
  }
-  interface CountlyEvent {
+  export interface CountlyEvent {


SgtPooki · 2023-01-20T08:11:06Z

src/MetricsProvider.ts

 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'


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.

SgtPooki · 2023-01-20T08:12:23Z

src/EventAccumulator.ts

+   *
+   * @param {string} key - event key
+   */
+  flush (key: string): void {


should we have a flush_all?

added with tests

SgtPooki · 2023-01-20T08:15:12Z

src/EventAccumulator.ts

+  key: '',
+  count: 1,
+  sum: 1,
+  dur: Date.now(),


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

https://github.com/Countly/countly-sdk-web/blob/1c72a3b5d4c1e66031e3ecc5779adfe030ab21e0/lib/countly.js#L3501

https://support.count.ly/hc/en-us/articles/360037441932-Web-analytics-JavaScript-#timed-events

this gets reset before the event gets flushed. setting default to 0.

src/EventAccumulator.ts

SgtPooki · 2023-01-20T08:25:33Z

src/EventAccumulator.ts

+        timeout: setTimeout(() => {
+          this.flush(key)
+        }, this.flushInterval)
+      }


Suggested change

}

})

SgtPooki · 2023-01-20T08:26:04Z

src/EventAccumulator.ts

+      }
+    } else {
+      // if event is in the store, update the event data.
+      const { eventData } = this.events[key]


Suggested change

const { eventData } = this.events[key]

const { eventData } = this.events.get(key) as eventStore

SgtPooki · 2023-01-20T08:26:27Z

src/EventAccumulator.ts

+      const { eventData } = this.events[key]
+      eventData.count += count
+      eventData.sum += 1
+      eventData.segmentation = { ...eventData.segmentation, ...newEvent.segmentation }


Suggested change

eventData.segmentation = { ...eventData.segmentation, ...newEvent.segmentation }

eventData.segmentation = { ...eventData.segmentation, ...newEvent.segmentation }

this.events.set(key, eventData)

why do I need to set it back? Map.get should get me reference to the object, modifying the object updates the referenced object.

SgtPooki · 2023-01-20T08:27:43Z

src/EventAccumulator.ts

+ */
+export class EventAccumulator implements IEventAccumulator {
+  private readonly metricsService: CountlyWebSdk
+  private readonly events: Record<string, eventStore>


Suggested change

private readonly events: Record<string, eventStore>

private readonly events: Map<string, eventStore>

src/EventAccumulator.ts

whizzzkid

explainers

src/EventAccumulator.ts

whizzzkid · 2023-01-20T09:45:04Z

src/EventAccumulator.ts

+  key: '',
+  count: 1,
+  sum: 1,
+  dur: Date.now(),


this gets reset before the event gets flushed. setting default to 0.

src/EventAccumulator.ts

whizzzkid · 2023-01-20T09:55:00Z

src/EventAccumulator.ts

+    this.metricsService.q.push(['add_event', eventData])
+    clearTimeout(timeout)
+    // eslint-disable-next-line @typescript-eslint/no-dynamic-delete
+    delete this.events[key]


whizzzkid · 2023-01-20T10:03:25Z

src/EventAccumulator.ts

+   *
+   * @param {string} key - event key
+   */
+  flush (key: string): void {


added with tests

* main: chore(release): 1.2.0 [skip ci] feat: use localStorage to cache consent in browsers (#64)

.eslintrc.json

SgtPooki · 2023-01-24T18:29:46Z

.mocharc.json

+{
+  "extensions": ["ts"],
+  "spec": ["test/**/*.spec.*"],
+  "require": ["ts-node/register"],
+  "node-option": [
+    "experimental-specifier-resolution=node",
+    "loader=ts-node/esm"
+  ]
+}


this change for running the tests without build should probably be in a separate PR

yes, but I started when we did not have any testing in place, apologies, if you want I can spend sometime splitting these, but I hope it's fine for this time?

SgtPooki · 2023-01-24T18:32:34Z

.mocharc.json

+  "spec": ["test/**/*.spec.*"],
+  "require": ["ts-node/register"],
+  "node-option": [
+    "experimental-specifier-resolution=node",


do we need this if we're using ts-node/esm loader?

for some reason, it wants both.

SgtPooki · 2023-01-24T19:27:19Z

test/unit/EventAccumulator.spec.ts

+
+    accumulator.addEvent(event1)
+    // adding the second event after 50ms so that the first event is flushed first
+    await sleep(50)


Suggested change

await sleep(50)

clock.tick(50)

SgtPooki · 2023-01-24T19:27:28Z

test/unit/EventAccumulator.spec.ts

+    // adding the second event after 50ms so that the first event is flushed first
+    await sleep(50)
+    accumulator.addEvent(event2)
+    await sleep(50)


Suggested change

await sleep(50)

clock.tick(50)

SgtPooki · 2023-01-24T19:27:55Z

test/unit/EventAccumulator.spec.ts

+    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)


check before getting args

Suggested change

expect(countlyStub.add_event.callCount).to.be.equal(2)

SgtPooki · 2023-01-24T19:28:51Z

test/unit/EventAccumulator.spec.ts

+    }
+
+    accumulator.addEvent(event1)
+    // adding the second event after 50ms so that the first event is flushed first


i'm not sure I understand this. If the event names are different, why does a 50ms delay ensure the first event is flushed first?

because right now events are isolated per key type, i.e. they are flushed per event key and not all of those are flushed together. We can make all events flush at once, but that would break the use case where we only want to flush click events, that would not clear the right timeout and would need to be handled separately for other event types. I can change this to flush everything if you like that better.

SgtPooki · 2023-01-24T19:31:21Z

package.json

    "test": "run-s test:*",
-    "test:node": "aegir test -t node -f 'dist/test/node.spec.js'",
+    "test:node": "aegir test --target node --files 'test/**/*.spec.ts'",


should be a separate PR.

we will need a way to separate node and browser tests because of the countly import. if you're proposing we just find all tests in node/ then we need to update this

Suggested change

"test:node": "aegir test --target node --files 'test/**/*.spec.ts'",

"test:node": "aegir test --target node --files 'test/node/**/*.spec.ts'",

tsconfig can have both node and browser types, just wondering if it still be a problem, moving for now.

* main: chore(release): 1.2.2 [skip ci] fix(types): correct types for exports (#73) chore(release): 1.2.1 [skip ci] fix: expose more tracking methods (#70)

whizzzkid · 2023-01-26T09:04:32Z

@SgtPooki thanks for a thorough review, I fixed most of the issues:

👍🏽 = fixed.

Discussions:

Ready for a re-review.

SgtPooki

thanks for the updates, looks great. A few non-blocking comments

SgtPooki · 2023-01-26T17:50:08Z

src/MetricsProvider.ts

    this.accumulate = new EventAccumulator(metricsService)
-
+    this.storageProvider = storageProvider ?? null
    this.metricsService.init(serviceConfig)


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

SgtPooki · 2023-01-26T17:56:36Z

test/node/NodeMetricsProvider.spec.ts

@@ -1,3 +1,4 @@
+/* eslint-disable @typescript-eslint/no-unused-expressions */


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.

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

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

github-actions · 2023-01-27T00:26:47Z

🎉 This PR is included in version 1.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

whizzzkid added 9 commits January 19, 2023 00:02

feat: editor config, sane!

ee49b2a

Add event accumulator class

d07079d

feat: Update types and hookup accumulator

ccece9b

fix: configs

f1da631

feat: moving types to set rootDir

06ef5a9

adding test folder

525234c

adding relevant types

b8cde72

types migration

882075f

fix: add_event

6a59727

SgtPooki reviewed Jan 20, 2023

View reviewed changes

whizzzkid added 7 commits January 20, 2023 01:50

feat: adding accumulator test.

dd6a6cb

breaking down logic

1802c4e

fix: import order

9e62d8f

fix: sinon stubs

ded115c

feat: Adding impl using map

719519c

adding flushAll method

c459dcb

Hooking up to the beforunload event

d23fcb6

whizzzkid commented Jan 20, 2023

View reviewed changes

lint

db97c66

whizzzkid marked this pull request as ready for review January 20, 2023 10:25

whizzzkid added 5 commits January 21, 2023 00:32

Merge branch 'main' into feat/accumulate-events

a28bc36

* main: chore(release): 1.2.0 [skip ci] feat: use localStorage to cache consent in browsers (#64)

fix: 🔀 merged with upstream

5791d84

fix: 📦 adding missing package after merge

98a7ab2

fix: 🧪 EventAccumulator

90b37ba

fix: 🩹 fix imports

eb5ae38

whizzzkid changed the title ~~[WIP] feat: Accumulate Events~~ feat: Accumulate Events Jan 21, 2023

whizzzkid added 4 commits January 21, 2023 03:30

fix: 🚚 rename to spec.ts

f638eb8

fix: ♻️ cleanup

1238a5f

fix: 📦 package-lock.json

6bd68c6

fix: ✏️ spec naming and scope

7c07a7d

SgtPooki requested changes Jan 24, 2023

View reviewed changes

whizzzkid added 15 commits January 25, 2023 03:24

Merge branch 'main' into feat/accumulate-events

4fe767b

* main: chore(release): 1.2.2 [skip ci] fix(types): correct types for exports (#73) chore(release): 1.2.1 [skip ci] fix: expose more tracking methods (#70)

fix: 🔧 don't disable rules globally

9eea9a5

fix: 🔧 only apply eslint overrides to spec files.

2722ec2

fix: 🚨 fixes lint.

8eb34e9

fix: ♻️ Moving tests to node folder

d34928b

fix: 🏷️ generics

772018d

fix: 👔 instantiating at declaration

2313806

fix: 📝 adding missing documentation

8c4f560

fix: 👔 fixing unload event to handle both browser and node.

c999b06

fix: 🚚 digest -> accumulate

fbaa4f3

fix: generics

dfc72df

fix: ✏️ dupe imports

c2c4277

fix: fixing appKey

a0242f9

feat: ⚡ useFakeTimers instead of await

ab218ab

fix: ♻️ move call order

3549175

whizzzkid requested a review from SgtPooki January 26, 2023 09:04

SgtPooki approved these changes Jan 26, 2023

View reviewed changes

whizzzkid merged commit 2d43252 into main Jan 27, 2023

github-actions bot added the released label Jan 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Accumulate Events #65

feat: Accumulate Events #65

whizzzkid commented Jan 19, 2023 •

edited

Loading

SgtPooki left a comment

SgtPooki Jan 20, 2023

SgtPooki Jan 20, 2023

SgtPooki Jan 20, 2023

whizzzkid Jan 20, 2023

SgtPooki Jan 20, 2023

whizzzkid Jan 20, 2023

SgtPooki Jan 20, 2023

SgtPooki Jan 20, 2023

whizzzkid Jan 26, 2023

SgtPooki Jan 20, 2023

whizzzkid Jan 26, 2023

SgtPooki Jan 20, 2023

whizzzkid left a comment

whizzzkid Jan 20, 2023

whizzzkid Jan 20, 2023

whizzzkid Jan 20, 2023

SgtPooki Jan 24, 2023

whizzzkid Jan 25, 2023

SgtPooki Jan 24, 2023

whizzzkid Jan 25, 2023

SgtPooki Jan 24, 2023

SgtPooki Jan 24, 2023

SgtPooki Jan 24, 2023

SgtPooki Jan 24, 2023

whizzzkid Jan 26, 2023

SgtPooki Jan 24, 2023

whizzzkid Jan 26, 2023

whizzzkid commented Jan 26, 2023

SgtPooki left a comment

SgtPooki Jan 26, 2023

SgtPooki Jan 26, 2023

whizzzkid Jan 27, 2023

github-actions bot commented Jan 27, 2023

	const { eventData } = this.events[key]
	const { eventData } = this.events.get(key) as eventStore

	eventData.segmentation = { ...eventData.segmentation, ...newEvent.segmentation }
	eventData.segmentation = { ...eventData.segmentation, ...newEvent.segmentation }
	this.events.set(key, eventData)

	private readonly events: Record<string, eventStore>
	private readonly events: Map<string, eventStore>

	"test:node": "aegir test --target node --files 'test/*/.spec.ts'",
	"test:node": "aegir test --target node --files 'test/node/*/.spec.ts'",

		@@ -1,3 +1,4 @@
		/* eslint-disable @typescript-eslint/no-unused-expressions */

feat: Accumulate Events #65

feat: Accumulate Events #65

Conversation

whizzzkid commented Jan 19, 2023 • edited Loading

SgtPooki 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whizzzkid 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

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

whizzzkid commented Jan 26, 2023

SgtPooki 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

github-actions bot commented Jan 27, 2023

whizzzkid commented Jan 19, 2023 •

edited

Loading