Conversation
# Conflicts: # hyx/events.py # hyx/ratelimit/api.py # hyx/ratelimit/managers.py # hyx/retry/api.py
# Conflicts: # hyx/telemetry/otel.py # hyx/telemetry/prometheus.py # hyx/telemetry/statsd.py # tests/test_otel.py # tests/test_prometheus.py # tests/test_statsd.py
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
roma-glushko
added
the
type:documentation
roma-glushko
changed the title
There was a problem hiding this comment.
Pull request overview
This PR adds event system support to rate limiter components, enabling telemetry integration for rate limiting operations. It follows issue #68 to bring rate limiters in line with other Hyx components like retry, circuit breaker, timeout, and bulkhead.
Changes:
- Introduced
RateLimiterListenerevent system for rate limiters withon_rate_limitedevent - Added telemetry integrations for StatsD, Prometheus, and OpenTelemetry
- Updated rate limiter managers and API to support event dispatching and component naming
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
hyx/ratelimit/events.py |
New file defining RateLimiterListener base class and global listener registry |
hyx/ratelimit/managers.py |
Added name property and event_dispatcher support to RateLimiter classes |
hyx/ratelimit/api.py |
Updated tokenbucket to support listeners, event_manager, and naming |
hyx/telemetry/statsd.py |
Added RateLimiterListener for StatsD metrics and updated register_listeners |
hyx/telemetry/prometheus.py |
Added RateLimiterListener for Prometheus metrics and updated register_listeners |
hyx/telemetry/otel.py |
Added RateLimiterListener for OpenTelemetry metrics and updated register_listeners |
tests/test_statsd.py |
Added test for rate limiter StatsD metrics |
tests/test_prometheus.py |
Added test for rate limiter Prometheus metrics |
tests/test_otel.py |
Added test for rate limiter OpenTelemetry metrics |
tests/test_ratelimiter/test_api.py |
Updated tests to use explicit limiter names for clarity |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @property | ||
| def name(self) -> str | None: | ||
| return self._name |
There was a problem hiding this comment.
The name property returns str | None, which is inconsistent with other components in the codebase. For example, TimeoutManager's name property returns str (non-optional) with a default of empty string. This inconsistency could cause issues if code expects all component names to be non-None strings. Consider either making the return type consistent with other components or ensuring that None values are handled appropriately throughout the codebase.
| if self._limiter._name is None: | ||
| self._limiter._name = get_default_name(func) |
There was a problem hiding this comment.
Directly assigning to the private attribute _name on self._limiter violates encapsulation and is problematic because the RateLimiter base class defines _name but doesn't provide a setter. This creates tight coupling between the tokenbucket class and the internal implementation details of the limiter. Consider either adding a setter property to RateLimiter or restructuring to pass the name through the constructor as intended.
| if self._limiter._name is None: | |
| self._limiter._name = get_default_name(func) | |
| if self._name is None: | |
| self._name = get_default_name(func) |
| self._client = _get_client(client) | ||
|
|
||
| async def on_rate_limited(self, limiter: "RateLimiter") -> None: | ||
| self._client.incr(f"ratelimiter.{limiter.name}.rejected") |
There was a problem hiding this comment.
If limiter.name is None, the string formatting will produce a metric name like "ratelimiter.None.rejected". While this follows the existing pattern used by other components in the codebase (e.g., TimeoutListener, BulkheadListener), it could still produce confusing metric names. Consider documenting this behavior or ensuring name is always populated with a default value.
| self._client.incr(f"ratelimiter.{limiter.name}.rejected") | |
| # Use a clear default when limiter.name is not set to avoid 'ratelimiter.None.*' metrics. | |
| limiter_name = limiter.name or "unnamed" | |
| self._client.incr(f"ratelimiter.{limiter_name}.rejected") |
| ) | ||
|
|
||
| async def on_rate_limited(self, limiter: "RateLimiter") -> None: | ||
| self._rate_limited_counter.labels(component=limiter.name).inc() |
There was a problem hiding this comment.
If limiter.name is None, the label will be set to None. While this follows the existing pattern used by other components in the codebase (e.g., CircuitBreakerListener), it could still potentially cause issues with Prometheus. Consider documenting this behavior or ensuring name is always populated with a default value like an empty string.
| self._rate_limited_counter.labels(component=limiter.name).inc() | |
| component_name = limiter.name or "" | |
| self._rate_limited_counter.labels(component=component_name).inc() |
hyx/ratelimit/api.py
Outdated
| event_dispatcher: EventDispatcher[TokenBucketLimiter, RateLimiterListener] = EventDispatcher( | ||
| listeners, | ||
| _RATELIMITER_LISTENERS, | ||
| event_manager=event_manager, | ||
| ) | ||
|
|
||
| def __init__(self, max_executions: float, per_time_secs: float, bucket_size: float | None = None) -> None: | ||
| self._limiter = TokenBucketLimiter( | ||
| max_executions=max_executions, | ||
| per_time_secs=per_time_secs, | ||
| bucket_size=bucket_size, | ||
| name=name, | ||
| event_dispatcher=event_dispatcher.as_listener, | ||
| ) | ||
|
|
||
| event_dispatcher.set_component(self._limiter) |
There was a problem hiding this comment.
The tokenbucket class manually constructs the EventDispatcher and wires it to the TokenBucketLimiter, instead of using the create_manager helper function. This approach is inconsistent with other components in the codebase like retry, timeout, and bulkhead, which all use create_manager. Consider refactoring to use create_manager for consistency and to reduce boilerplate code.
No description provided.