feat(exception-capture): add client-side token bucket rate limiting#662
Open
hpouillot wants to merge 4 commits into
Open
feat(exception-capture): add client-side token bucket rate limiting#662hpouillot wants to merge 4 commits into
hpouillot wants to merge 4 commits into
Conversation
Port the posthog-js BucketedRateLimiter (packages/core/src/utils/ bucketed-rate-limiter.ts) and apply it to exception autocapture with the same settings as the browser and Node SDKs: one bucket per exception type, bucket size 10, refilling 1 token per 10 seconds. Resolves the long-standing TODO in exception_capture.py. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
posthog-python Compliance ReportDate: 2026-06-12 11:18:34 UTC ✅ All Tests Passed!45/45 tests passed Capture Tests✅ 29/29 tests passed View Details
Feature_Flags Tests✅ 16/16 tests passed View Details
|
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
posthog/test/test_bucketed_rate_limiter.py:223-248
**Integration test placed in wrong test module**
`test_exception_capture_rate_limits_per_exception_type` exercises `ExceptionCapture` end-to-end (including its `sys.excepthook` side-effect and `close()` teardown) — it is an integration test for `ExceptionCapture`, not for `BucketedRateLimiter`. Having it here means anyone reading `test_exception_capture.py` gets an incomplete picture of that class's tested behaviour. It belongs in `test_exception_capture.py` alongside the other `ExceptionCapture` tests.
Reviews (1): Last reviewed commit: "feat(exception-capture): add client-side..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+223
to
+248
| def test_exception_capture_rate_limits_per_exception_type(): | ||
| from posthog.exception_capture import ExceptionCapture | ||
|
|
||
| client = MagicMock() | ||
| capture = ExceptionCapture(client) | ||
| try: | ||
|
|
||
| def exc_info(error): | ||
| try: | ||
| raise error | ||
| except type(error): | ||
| import sys | ||
|
|
||
| return sys.exc_info() | ||
|
|
||
| for _ in range(15): | ||
| capture.capture_exception(exc_info(ValueError("boom"))) | ||
|
|
||
| # bucket size 10 -> 9 captured, the rest rate limited | ||
| assert client.capture_exception.call_count == 9 | ||
|
|
||
| # a different exception type has its own bucket | ||
| capture.capture_exception(exc_info(ZeroDivisionError("zero"))) | ||
| assert client.capture_exception.call_count == 10 | ||
| finally: | ||
| capture.close() |
Contributor
There was a problem hiding this comment.
test_exception_capture_rate_limits_per_exception_type exercises ExceptionCapture end-to-end (including its sys.excepthook side-effect and close() teardown) — it is an integration test for ExceptionCapture, not for BucketedRateLimiter. Having it here means anyone reading test_exception_capture.py gets an incomplete picture of that class's tested behaviour. It belongs in test_exception_capture.py alongside the other ExceptionCapture tests.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/test/test_bucketed_rate_limiter.py
Line: 223-248
Comment:
**Integration test placed in wrong test module**
`test_exception_capture_rate_limits_per_exception_type` exercises `ExceptionCapture` end-to-end (including its `sys.excepthook` side-effect and `close()` teardown) — it is an integration test for `ExceptionCapture`, not for `BucketedRateLimiter`. Having it here means anyone reading `test_exception_capture.py` gets an incomplete picture of that class's tested behaviour. It belongs in `test_exception_capture.py` alongside the other `ExceptionCapture` tests.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Expose exception_autocapture_bucket_size, exception_autocapture_refill_rate and exception_autocapture_refill_interval_seconds on Client and the module-level API, passed through to ExceptionCapture's rate limiter. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bucket size 50 refilling 10 tokens per 10 seconds (was 10/1/10, the browser SDK defaults) since one server process aggregates exceptions across many users' requests. Defaults now live on ExceptionCapture and are referenced by Client and the module-level API. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add enable_exception_autocapture_rate_limiting (default False) on Client, the module-level API and ExceptionCapture. The limiter is only constructed when enabled, so default behavior is unchanged from released versions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
marandaneto
approved these changes
Jun 12, 2026
Contributor
|
Reviews (2): Last reviewed commit: "feat(exception-capture): make rate limit..." | Re-trigger Greptile |
cat-ph
approved these changes
Jun 12, 2026
| def capture_exception(self, exception, metadata=None): | ||
| try: | ||
| if self._rate_limiter is not None: | ||
| exception_type = self._exception_type(exception) |
There was a problem hiding this comment.
is this the correct order with chained exceptions? I think that might consume the top-level one instead, i.e. RuntimeError from ZeroDivisionError consumes RuntimeError instead of ZeroDivisionError
client = MagicMock()
capture = ExceptionCapture(client, rate_limiting_enabled=True, bucket_size=2)
capture.capture_exception(exc_info_for(RuntimeError("wrapped", ZeroDivisionError())))
capture.capture_exception(exc_info_for(RuntimeError("wrapped", KeyError())))
assert client.capture_exception.call_count == 2I think this fails, but we'd want it to pass?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
💡 Motivation and Context
Addresses the long-standing
TODOinposthog/exception_capture.py: exception autocapture has no client-side rate limiting, so a crash loop can flood the ingestion queue.This ports the
BucketedRateLimiterfrom posthog-js (packages/core/src/utils/bucketed-rate-limiter.ts) and applies it to exception autocapture as an opt-in feature:$exception_list[0].type, falling back to"Exception")Skipping exception capture because of client rate limiting.like the other SDKsNew
Clientoptions (also available as module-level settings):enable_exception_autocapture_rate_limitingFalseexception_autocapture_bucket_size50exception_autocapture_refill_rate10exception_autocapture_refill_interval_seconds10Deviations from the JS source, since this SDK runs in threaded server processes:
threading.Lock(theon_bucket_rate_limitedcallback fires outside the lock)One JS quirk is preserved for cross-SDK parity: the call that drains the bucket is itself reported as limited, so a burst lets
bucket_size - 1events through.💚 How did you test it?
posthog/test/test_bucketed_rate_limiter.py: 25 tests porting the posthog-js spec (bucketed-rate-limiter.spec.ts) — consumption, refill math, partial intervals, bucket isolation, callback semantics,stop(), timestamp carry-over — plus Python-specific tests: parameter clamping, a 10-thread concurrency test asserting exactlybucket_size - 1of 200 contended consumes pass,ExceptionCaptureintegration (15 same-type exceptions on a size-10 bucket → 9 captured; a different type still passes), disabled-by-default (100 captures pass untouched, no limiter constructed), enabled defaults, and config pass-through fromClientkwargs to the limiter.posthog/test/test_exception_capture.py(15 tests) still passes.ruff format,ruff check, andmypyare clean.📝 Checklist
If releasing new changes
sampo addto generate a changeset file🤖 Generated with Claude Code