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: Added client side capture rate limiting #1051

Merged
merged 74 commits into from
Apr 18, 2024
Merged

Conversation

benjackwhite
Copy link
Collaborator

@benjackwhite benjackwhite commented Mar 4, 2024

Changes

Needs PostHog/posthog#21388

We want to protect ourselves and our users from abnormal client behaviour. This can be anything from an accidental loop in the code to someone running some strange scripts in the browser to a bug.

  • Adds the client side check for capture
  • Logs a message
  • Make the configs configurable
  • Persist to storage (which handles page load unload loops)
  • Adds a $$ event when the rate limit kicks in

Checklist

  • Tests for new code (see advice on the tests we use)
  • Accounted for the impact of any changes across different browsers
  • Accounted for backwards compatibility of any changes (no breaking changes in posthog-js!)

Copy link

github-actions bot commented Mar 4, 2024

Size Change: +5.58 kB (+1%)

Total Size: 968 kB

Filename Size Change
dist/array.full.js 231 kB +1.4 kB (+1%)
dist/array.js 129 kB +1.4 kB (+1%)
dist/es.js 129 kB +1.4 kB (+1%)
dist/module.js 129 kB +1.4 kB (+1%)
ℹ️ View Unchanged
Filename Size
dist/exception-autocapture.js 12.2 kB
dist/recorder-v2.js 108 kB
dist/recorder.js 108 kB
dist/surveys-module-previews.js 62 kB
dist/surveys.js 58.3 kB

compressed-size-action

@benjackwhite benjackwhite marked this pull request as ready for review March 4, 2024 16:58
@pauldambra
Copy link
Member

is it missing a commit? ratelimiter has no "bucket"

@benjackwhite benjackwhite requested a review from pauldambra March 5, 2024 08:32
@benjackwhite benjackwhite added the bump minor Bump minor version when this PR gets merged label Mar 5, 2024
src/__tests__/posthog-core.js Outdated Show resolved Hide resolved
src/rate-limiter.ts Show resolved Hide resolved
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.

ran it locally
captured 100k events in a loop
only ingested 291
saw logs in the console

🚀

only "improvement" I can think of is a heavily debounced internal event or custom rrweb event in recording maybe that we throttled to help with support but that's very much polishing and not blocking

Base automatically changed from feat/request-refactor to main March 8, 2024 12:03
# Conflicts:
#	src/__tests__/decide.js
#	src/__tests__/featureflags.js
#	src/__tests__/posthog-core.identify.js
#	src/__tests__/rate-limiter.test.ts
#	src/decide.ts
#	src/posthog-core.ts
#	src/posthog-featureflags.ts
#	src/rate-limiter.ts
#	src/types.ts
src/rate-limiter.ts Outdated Show resolved Hide resolved
@posthog-bot
Copy link
Collaborator

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.

@benjackwhite
Copy link
Collaborator Author

This is a bit stuck in limbo atm due to the inability to filter out these internal events from usage reports... Wondering about changing the logic instead on pipeline filter out the event (maybe make it a specific one called $$ingestion_warning or something).

@xvello ideally I'd leave this more to pipeline to decide on the best approach / point in time to do that filtering. Either I can keep this PR on hold until something like that is ready, or merge without the rate limit event. Thoughts?

@posthog-bot
Copy link
Collaborator

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.

# Conflicts:
#	src/constants.ts
#	src/posthog-core.ts
#	src/posthog-persistence.ts
#	src/types.ts
# Conflicts:
#	src/__tests__/posthog-core.js
#	src/posthog-core.ts
#	src/types.ts
# Conflicts:
#	src/posthog-core.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump minor Bump minor version when this PR gets merged waiting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants