Skip to content

Adds batching support to AnalyticsNode - #640

Merged
chrisradek merged 14 commits into
masterfrom
node-flush
Oct 31, 2022
Merged

Adds batching support to AnalyticsNode#640
chrisradek merged 14 commits into
masterfrom
node-flush

Conversation

@chrisradek

@chrisradek chrisradek commented Oct 25, 2022

Copy link
Copy Markdown
Contributor

This PR adds batching support to AnalyticsNode.

Plugin behavior

This plugin supports batching events and sending batches when 1 of 3 conditions is met:

  1. Number of events in batch matches the maximum allowed number of events (configurable, default 15)
  2. Adding an event to a batch would exceed the max payload size in bytes (not configurable, ~500 KB)
  3. The amount of time since the batch was created exceeds the max wait time (configurable, default 1000 ms)

The plugin methods (track et. al.) resolve once the associated event has been sent to Segment or retries have been exhausted. This differs from the batching implementation in AnalyticsBrowser which instead resolves as soon as an event is enqueued. I decided to wait to resolve until the event was done being sent because it significantly simplifies the EventQueue's ability to keep track of in-flight events, which in turn makes it easier to support graceful shutdowns.

Implementation details

The plugin is broken into 2 main components:

  • ContextBatch
  • Publisher

The ContextBatch ensures that events are batched properly. It won't allow events to be added if they exceed the event count or byte count.

The Publisher does the heavy lifting. It is responsible for receiving events, creating batches, and sending those batches to Segment.

@changeset-bot

changeset-bot Bot commented Oct 25, 2022

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: d7ae5f1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR


describe('drained emitted event', () => {
test('emits a drained event if only one event is dispatched', async () => {
it('emits a drained event if only one event is dispatched', async () => {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I changed test to it just because my IDE thought test() had to return a promise, so I had tons of red squiggles. Might be a transient type merge occurring, but it matches what we use in out other packages and fixed my issue 😁

})

test('http delivery errors are accessed through the emitter', (done) => {
it.skip('http delivery errors are accessed through the emitter', (done) => {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I skipped this for now - it looked like any error in the previous plugin would end up being treated as an http delivery error, so I wanted to confirm if the intent was for this just to apply to errors thrown by fetch and not non-200 status codes. @silesky

@silesky silesky Oct 28, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

http delivery was meant to be any non ok response along with any thrown errors -- sort of the way axios work, but the issue that maybe you're running into is that a thrown fetch error might not have a Response (e.g., a network error).

So, maybe the "response" keys should be optional, but we also add the request as well. (packages/node/src/app/emitted-errors.ts)

The justification is that even though most emitted errors will be http delivery, we may add other error types like validation in the future, so it's a good future-proof pattern to have, and it also lets you add type-safe specific metadata based on the code (which is the discriminant key). See: packages/node/src/app/emitted-errors.ts

edit: Error emitting doesn't seem to work atm.

@chrisradek
chrisradek marked this pull request as ready for review October 27, 2022 05:43
@silesky

silesky commented Oct 27, 2022

Copy link
Copy Markdown
Contributor

Perf testing

 New SDK vs old SDK ->
 new: 7054.5, old: 6639.55, difference: 414.9499999999998 (Requests per second) 
 Diff performance: 6.25 %

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants