feat: add webhook rate limiting and stats tracking#143
feat: add webhook rate limiting and stats tracking#143gauthierdmn wants to merge 1 commit intomainfrom
Conversation
Add in-memory sliding window rate limiter to protect webhook endpoints from replay attacks and retry storms. Includes per-platform statistics tracking with disk persistence and a /stats endpoint for observability. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![nominalbot[bot]](https://avatars.githubusercontent.com/in/2961348?s=60&v=4){kind=small}
There was a problem hiding this comment.
The new webhook rate limiter and stats components are well-designed and implemented, providing valuable control and observability. A few minor improvements related to type annotations, persistence strategy, performance considerations, and statistical accuracy are suggested.
| _Handler = Callable[[web.Request], Awaitable[web.StreamResponse]] | ||
|
|
||
|
|
||
| def build_rate_limit_middleware( |
There was a problem hiding this comment.
The return type of build_rate_limit_middleware can be made more specific than Any. The @web.middleware decorator returns a web.MiddlewareFactory.
| def build_rate_limit_middleware( | |
| def build_rate_limit_middleware( | |
| limiter: WebhookRateLimiter, | |
| ) -> web.MiddlewareFactory: |
|
|
||
| logger: logging.Logger = logging.getLogger(__name__) | ||
|
|
||
| STATS_FILE: Path = Path(tempfile.gettempdir()) / "webhook_stats.pkl" |
There was a problem hiding this comment.
Using tempfile.gettempdir() for storing webhook_stats.pkl might not guarantee persistence across server restarts, as temporary directories can be cleared by the operating system. If the goal is truly to survive restarts, consider a more permanent and configurable storage location, such as a subdirectory within the application's data directory or a path specified in the application configuration.
| if error: | ||
| self._errors[platform] += 1 | ||
|
|
||
| self._save() |
There was a problem hiding this comment.
Calling self._save() on every record_request can introduce a performance bottleneck, especially for high-volume webhook traffic, due to repeated disk I/O and pickling operations. Consider implementing a periodic save mechanism (e.g., every N requests, or every X seconds) or saving only on graceful application shutdown to reduce overhead.
| "total_errors": self._errors[platform], | ||
| "error_rate": self._errors[platform] / count, | ||
| "avg_response_time_ms": round(avg_time * 1000, 2), | ||
| "last_updated": str(datetime.now()), |
There was a problem hiding this comment.
The last_updated field currently reports the datetime.now() when the summary is requested. To provide more accurate information about when the statistics were actually last updated, consider storing a _last_updated_timestamp attribute within the class and updating it whenever record_request is called or when _save is successfully completed. This would reflect the freshness of the data rather than the query time.
| }, | ||
| file, | ||
| ) | ||
| except OSError: |
There was a problem hiding this comment.
It's good practice to catch specific exceptions. While OSError covers file system errors, pickle.PicklingError should also be caught explicitly to handle issues during the serialization process itself for robustness.
| except OSError: | |
| except (OSError, pickle.PicklingError): | |
| logger.debug("Failed to save webhook stats") |
1 participant
Add in-memory sliding window rate limiter to protect webhook endpoints from replay attacks and retry storms. Includes per-platform statistics tracking with disk persistence and a /stats endpoint for observability.