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

chore(blobby): penalize non-batched traffic for overflow detection #21607

Merged
merged 4 commits into from
May 6, 2024

Conversation

xvello
Copy link
Contributor

@xvello xvello commented Apr 17, 2024

Problem

Sessions from very old posthog-js versions can still slow down a partition without triggering overflow, because they emit one message per event, instead of batching them together.

Batching was implemented in PostHog/posthog-js#694, released as v1.68.2 in 2023. Default batching settings are a linger of 2 seconds, or a size of 900kib

Changes

  • OverflowManager has a new minPerCall property. Calls to observe consume at least this quantity
  • The value is set to 1MB/session/batch on blobby (after messages in the same Kakfa batch are aggregated). This will make a session trigger overflow if it sends more than 5 batches per functional second after a 200 burst budget. Might not be too high? We'll see and increase if needed.

Does this work well for both Cloud and self-hosted?

How did you test this code?

@xvello xvello requested review from a team and pauldambra April 17, 2024 11:39
@bretthoerner
Copy link
Contributor

FAIL tests/main/ingestion-queues/session-recording/session-recordings-consumer.test.ts (8.895 s)
  ● ingester with consumeOverflow=false › when ingester.stop is called in teardown › overflow detection › should not trigger overflow if under threshold

Seems likely related?

@@ -597,7 +598,9 @@ describe.each([[true], [false]])('ingester with consumeOverflow=%p', (consumeOve
describe(
'overflow detection',
consumeOverflow
? () => {} // Skip these tests when running with consumeOverflow (it's disabled)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

new eslint rule

Copy link
Contributor

@bretthoerner bretthoerner left a comment

Choose a reason for hiding this comment

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

Good idea!

I don't have a good fell for whether 1MB each is low or high, it sounds like you think it may not be high enough? If so that seems like a safer bet to start. 👍

Copy link
Member

@pauldambra pauldambra left a comment

Choose a reason for hiding this comment

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

👍

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@posthog-bot
Copy link
Contributor

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.

@posthog-bot posthog-bot closed this May 2, 2024
@bretthoerner
Copy link
Contributor

@xvello I assume you saw this, but just in case. Or maybe it's left unmerged on purpose?

@xvello xvello reopened this May 2, 2024
@xvello
Copy link
Contributor Author

xvello commented May 2, 2024

Should be merged indeed, was not sure about the value to set though.

@posthog-bot posthog-bot removed the stale label May 3, 2024
@xvello xvello merged commit c2a5d61 into master May 6, 2024
101 checks passed
@xvello xvello deleted the xvello/overflow-minperbatch branch May 6, 2024 12:54
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.

4 participants