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

Insights event batching #578

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

rabidpraxis
Copy link
Contributor

This includes an EventsWorker GenServer.

It bundles up sent events into batches and transmits them to the backend if they hit an event threshold (events_batch_size) or a time threshold (events_timeout). It will drop events if the queue is too full (events_max_queue_size).

This implementation differs a little from our Ruby implementation:

  • It attempts to retry batches of events if the server does not respond, or responds with unexpected errors. It drops the batch after n attempts (events_max_batch_retries)
  • It can have multiple concurrently retrying batches, but events will be dropped if the max queue size is it (including events already batched)
  • It has a static timeout value if the backend responds with a 429. This is an attempt to make sure that we don't send events too often (currently defaulted to 60s) if we know the server will not accept them.

This is ready for a review, but I would like to add a little more testing around the Client.

@stympy
Copy link
Member

stympy commented Feb 10, 2025

Looking good!

This replaces the array with a queue so we can get FIFO behavior when
pulling batches. This is to ensure that we attempt to send the oldest
batch first.

With this, I'm also using the same batch dropping logic for throttled
batches. This way, we drop the oldest batch after 3 failures (roughly 3
minutes). When the user upgrades or the throttle is reset, the events we
send right after will be closer to the current time.
@rabidpraxis rabidpraxis marked this pull request as ready for review February 22, 2025 16:01
@rabidpraxis
Copy link
Contributor Author

rabidpraxis commented Feb 22, 2025

Okay the client stuff might need some refactoring after #579 is merged, but the logic and testing around the batching is ready for an actual review.

I updated a few things:

  • We log the dropped event count after an interval (hardcoded 60s)
  • We now drop batches if we are throttling, so when the backend response with a 429 > events_max_batch_retries times then we toss the oldest batch, with the idea to keep the active set of batches close to the current time.

@rabidpraxis rabidpraxis requested a review from sorentwo February 22, 2025 16:05
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