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: different rate limiting handling #765

Merged
merged 7 commits into from
Sep 13, 2023

Conversation

pauldambra
Copy link
Member

the last attempt didn't play nicely with older clients...

Now

  • capture still returns 200 when rate limiting but includes a list of rate limited products in the body
  • the rate limiter uses the batch key or defaults to events (currently recordings sets a batch key and events doesn't)
  • we rate limit for 1 minute
  • we don't check for rate limiting on every response since there's no need to parse json for every request just in case there's been limiting

@github-actions
Copy link

github-actions bot commented Jul 27, 2023

Size Change: -1.18 kB (0%)

Total Size: 683 kB

Filename Size Change
dist/array.full.js 177 kB -295 B (0%)
dist/array.js 118 kB -296 B (0%)
dist/es.js 118 kB -296 B (0%)
dist/module.js 118 kB -296 B (0%)
ℹ️ View Unchanged
Filename Size
dist/recorder-v2.js 93.6 kB
dist/recorder.js 58.3 kB

compressed-size-action

Copy link
Collaborator

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

Looks good! Small comments but the core looks great

src/rate-limiter.ts Outdated Show resolved Hide resolved
src/rate-limiter.ts Outdated Show resolved Hide resolved
@pauldambra pauldambra marked this pull request as ready for review August 1, 2023 18:22
Copy link
Collaborator

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

👍

@benjackwhite benjackwhite added the bump patch Bump patch version when this PR gets merged label Aug 2, 2023
@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.

@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.

@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.

@pauldambra pauldambra removed the stale label Sep 11, 2023
@pauldambra pauldambra merged commit b6604de into master Sep 13, 2023
11 checks passed
@pauldambra pauldambra deleted the feat/different-rate-limiting-handling branch September 13, 2023 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump patch Bump patch version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants