Skip to content

feat(exception-capture): implement thread-safe fixed-window rate limiting with post-limit sampling#655

Open
mishrak5j wants to merge 5 commits into
PostHog:mainfrom
mishrak5j:feature/exception-rate-limiting
Open

feat(exception-capture): implement thread-safe fixed-window rate limiting with post-limit sampling#655
mishrak5j wants to merge 5 commits into
PostHog:mainfrom
mishrak5j:feature/exception-rate-limiting

Conversation

@mishrak5j

Copy link
Copy Markdown
Contributor

🚀 Description

Implements client-side exception rate limiting to resolve the open TODO
in posthog/exception_capture.py. Introduces a dedicated ExceptionRateLimiter
that clamps exception traffic in local memory before any network transmission.


🛠️ Design Architecture

Dual-stage filtering pipeline:

[ Unhandled Exception Caught ]


Stage 1: ExceptionRateLimiter (Fixed Window + Heartbeat)
├─► Acquires thread lock
├─► Checks window expiry, resets if elapsed
├─► count <= max ──► Allow
└─► count > max ──► Allow every Nth (heartbeat signal, count % post_limit_every == 0)

▼ (passed Stage 1)
Stage 2: Sampling (lock-free)
└─► random.random() > sample_rate ──► Drop

▼ (passed Stage 2)
[ Client Outbound Ingestion Queue ]


⚙️ New Client Configuration Parameters

Parameter Default Description
exception_capture_max_per_window 100 Max exceptions before heartbeat kicks in
exception_capture_window_seconds 60.0 Rolling window duration in seconds
exception_capture_sample_rate 1.0 Fraction of exceptions to forward (0.0–1.0)

🧪 Tests Added

  • test_allows_within_limit_and_handles_heartbeat — verifies first N pass, then 1-in-N heartbeat
  • test_window_resets_counters_cleanly — verifies counter resets after window expiry
  • test_invalid_parameters_raise_value_errors — verifies bad config raises ValueError
  • test_post_every_one_allows_all_events_after_limit — verifies post_limit_every=1 bypasses throttle

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • Added changelog entry via sampo add

⚠️ Breaking Changes

None at runtime — new client config params use getattr with safe defaults.
Existing Client instances without these attributes will behave identically to before.

@mishrak5j mishrak5j requested a review from a team as a code owner June 9, 2026 04:42
Copilot AI review requested due to automatic review settings June 9, 2026 04:42

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Introduces client-side exception capture throttling to prevent flooding PostHog with repeated exception events.

Changes:

  • Added a thread-safe fixed-window ExceptionRateLimiter with post-limit “heartbeat” sampling.
  • Integrated the rate limiter (and an additional sample-rate gate) into exception capture flow.
  • Added unit tests validating rate limiter behavior, window resets, and parameter validation.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
posthog/rate_limiter.py Adds the ExceptionRateLimiter implementation used to throttle exception capture.
posthog/test/test_rate_limiter.py Adds unit coverage for rate limit behavior and configuration validation.
posthog/exception_capture.py Wires the limiter into exception capture and adds probabilistic sampling.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread posthog/exception_capture.py
Comment thread posthog/exception_capture.py Outdated
Comment thread posthog/exception_capture.py
@greptile-apps

greptile-apps Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Comments Outside Diff (1)

  1. posthog/exception_capture.py, line 18-21 (link)

    P2 The TODO this PR was introduced to resolve still remains. Since rate limiting is now implemented, the stale comment should be removed to avoid confusing future readers.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: posthog/exception_capture.py
    Line: 18-21
    
    Comment:
    The TODO this PR was introduced to resolve still remains. Since rate limiting is now implemented, the stale comment should be removed to avoid confusing future readers.
    
    
    
    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!

Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
posthog/exception_capture.py:18-21
The TODO this PR was introduced to resolve still remains. Since rate limiting is now implemented, the stale comment should be removed to avoid confusing future readers.

```suggestion
class ExceptionCapture:
    log = logging.getLogger("posthog")
```

### Issue 2 of 3
posthog/rate_limiter.py:75-83
`_get_state_for_tests` is not called by any test in `test_rate_limiter.py` and reads shared state without holding `_lock`, making it inconsistent under concurrent access. Since the tests drive the limiter entirely through `should_capture()`, this method is purely superfluous — it should be removed.

```suggestion

```

### Issue 3 of 3
posthog/test/test_rate_limiter.py:82-94
**Prefer parameterised tests for validation cases**

The team convention is to use parameterised tests. `test_invalid_parameters_raise_value_errors` groups four independent input/error pairs inside one function body, which means a failure in an early `pytest.raises` block masks the remaining cases. Using `@pytest.mark.parametrize` with `(kwargs, match)` tuples makes each case independently reportable and aligns with the project style.

Reviews (1): Last reviewed commit: "feat: add client-side fixed window excep..." | Re-trigger Greptile

Comment thread posthog/rate_limiter.py Outdated
Comment thread posthog/test/test_rate_limiter.py Outdated
mishrak5j added 4 commits June 9, 2026 00:48
Refactor test for invalid parameters to use parameterization for cleaner code and better coverage.
Removed the internal state retrieval method for testing.
Remove debug log for exception capture rate limit.
@mishrak5j mishrak5j requested a review from Copilot June 9, 2026 05:15

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

Comment thread posthog/rate_limiter.py
Comment on lines +41 to +42
if max_exceptions < 0:
raise ValueError("max_exceptions must be >= 0")
Comment on lines +31 to +43
# Pull configurations dynamically from user-facing Client setups
max_exceptions = getattr(client, "exception_capture_max_per_window", 100)
window_seconds = getattr(client, "exception_capture_window_seconds", 60.0)
post_limit_every = getattr(client, "exception_capture_post_limit_every", 10)

self._sample_rate = getattr(client, "exception_capture_sample_rate", 1.0)

# Initialize the rate limiter engine
self._rate_limiter = ExceptionRateLimiter(
max_exceptions=max_exceptions,
window_seconds=window_seconds,
post_limit_every=post_limit_every,
)
Comment thread posthog/rate_limiter.py
Comment on lines +48 to +50
self._max = int(max_exceptions)
self._window = float(window_seconds)
self._post_every = int(post_limit_every)
Comment thread posthog/rate_limiter.py
Comment on lines +17 to +20
Parameters
- max_exceptions: non-negative int, number of allowed events per window.
- window_seconds: positive float, window length in seconds.
- post_limit_every: positive int, after the limit, allow 1 in ``post_limit_every``.
@marandaneto marandaneto requested review from a team, ablaszkiewicz, cat-ph and hpouillot June 9, 2026 13:45
@marandaneto

Copy link
Copy Markdown
Member

@PostHog/team-error-tracking do we need this? we dont have that in any other SDK i think (for exceptions at least)

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.

3 participants