feat: Rate limit logs #204
Merged
Conversation
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
Contributor
Reviewer's GuideImplements per-message, per-level log rate limiting in Trigger, storing last emission times to avoid sending identical logs too frequently, and adds tests to validate the new behavior and preserve existing batching semantics. Sequence diagram for Trigger.log with rate limitingsequenceDiagram
actor Connector
participant Trigger
Connector->>Trigger: log(message, level, args, kwargs)
activate Trigger
Trigger->>Trigger: compute key = level:message
Trigger->>Trigger: last_log_time = _last_log_time.get(key, 0)
Trigger->>Trigger: rate_limit = LOGS_RATE_LIMIT.get(level, 0)
Trigger->>Trigger: now = time.time()
alt within rate limit
Trigger-->>Connector: return (skip sending log)
else not rate limited
Trigger->>Trigger: _last_log_time[key] = now
Trigger->>Trigger: append log dict to _logs
Trigger-->>Connector: return
end
deactivate Trigger
Class diagram for updated Trigger log rate limitingclassDiagram
class Trigger {
<<ModuleItem>>
+ClassVar dict~str,int~ LOGS_MAX_BATCH_SIZE
+ClassVar int LOGS_MAX_DELTA
+ClassVar dict~str,int~ LOGS_RATE_LIMIT
-bool _critical_log_sent
-dict~str,float~ _last_log_time
-list~dict~ _logs
+log(message str, level LogLevelStr, args, kwargs) None
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Darkheir
force-pushed
the
feat/logs_rate_limit
branch
from
08b8a78 to
4e5e324
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The
_last_log_timedict grows a new entry for every distinctlevel:messagecombination and is never cleaned up, which could lead to unbounded memory growth over long‑running triggers; consider adding a pruning strategy (e.g., periodic cleanup of old keys) or a size‑bounded cache.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_last_log_time` dict grows a new entry for every distinct `level:message` combination and is never cleaned up, which could lead to unbounded memory growth over long‑running triggers; consider adding a pruning strategy (e.g., periodic cleanup of old keys) or a size‑bounded cache.
## Individual Comments
### Comment 1
<location> `sekoia_automation/trigger.py:79` </location>
<code_context>
self._secrets: dict[str, Any] = {}
self._stop_event = Event()
self._critical_log_sent = False
+ self._last_log_time: dict[str, float] = {}
self._logs: list[dict] = []
</code_context>
<issue_to_address>
**suggestion (performance):** Consider bounding or periodically cleaning `_last_log_time` to avoid unbounded growth over long‑running triggers.
Since `_last_log_time` keeps an entry per `level:message` and is never pruned, a long‑running trigger with many distinct messages can leak memory. Please add some eviction (e.g., max size, age-based pruning, or clearing after successful batch sends) to keep this dict bounded over time.
Suggested implementation:
```python
# Time to wait for stop event to be received
_STOP_EVENT_WAIT = 120
def _prune_last_log_time(self) -> None:
"""Prune `_last_log_time` to avoid unbounded growth.
Removes old entries based on age and bounds the dict size.
"""
if not getattr(self, "_last_log_time", None):
return
now = time.time()
# Age-based pruning
max_age = getattr(self, "LOGS_LAST_LOG_TIME_MAX_AGE", 0) or 0
if max_age > 0:
keys_to_delete = [
key
for key, ts in self._last_log_time.items()
if now - ts > max_age
]
for key in keys_to_delete:
self._last_log_time.pop(key, None)
# Size-based pruning
max_size = getattr(self, "LOGS_LAST_LOG_TIME_MAX_SIZE", 0) or 0
if max_size <= 0 or len(self._last_log_time) <= max_size:
return
# Remove oldest entries (smallest timestamp) until within bounds
surplus = len(self._last_log_time) - max_size
for key, _ in sorted(
self._last_log_time.items(), key=lambda kv: kv[1]
)[:surplus]:
self._last_log_time.pop(key, None)
```
```python
self._secrets: dict[str, Any] = {}
self._stop_event = Event()
self._critical_log_sent = False
# rate-limit state: last log time per "level:message"
self._last_log_time: dict[str, float] = {}
```
```python
import time
import requests
LOGS_MAX_BATCH_SIZE = 50
LOGS_MAX_DELTA = 5 # seconds
# Bound `_last_log_time` growth
LOGS_LAST_LOG_TIME_MAX_SIZE = 1000
LOGS_LAST_LOG_TIME_MAX_AGE = 3600 # seconds
```
To fully implement the eviction and make it effective, you should also:
1. Call `_prune_last_log_time()` in the method that updates `self._last_log_time` (i.e., wherever you do `self._last_log_time[key] = time.time()` for log rate limiting), directly after updating the timestamp:
- Example pattern to adjust:
```python
last = self._last_log_time.get(key)
# ... rate limit logic ...
self._last_log_time[key] = now
```
should become:
```python
last = self._last_log_time.get(key)
# ... rate limit logic ...
self._last_log_time[key] = now
self._prune_last_log_time()
```
2. Optionally, if you have a method that clears `_logs` after a successful batch send, you can also call `_prune_last_log_time()` there to keep the in-memory state small over time, especially for long-running triggers.
You will need to locate the exact logging/rate-limiting method(s) in this file and apply the above pattern to integrate `_prune_last_log_time()` at the right points.
</issue_to_address>
### Comment 2
<location> `tests/test_trigger.py:317-326` </location>
<code_context>
assert log_request["message"] == "test message"
+def test_trigger_log_rate_limit(mocked_trigger_logs):
+ trigger = DummyTrigger()
+ trigger.LOGS_MAX_BATCH_SIZE = 0
+
+ assert mocked_trigger_logs.call_count == 0
+
+ trigger.log("test message", "info")
+ trigger.log("test message", "info")
+ assert mocked_trigger_logs.call_count == 1
+ assert len(mocked_trigger_logs.last_request.json()["logs"]) == 1
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test that verifies the rate limit window elapsing allows the same log to be sent again.
This test only verifies that consecutive identical logs are rate-limited. To also validate the time-based behavior, please add a test that controls the rate limit window (e.g., by temporarily setting `LOGS_RATE_LIMIT["info"]` to a small value or monkeypatching `time.time`) and asserts that: (1) the first call is sent, (2) an immediate second call is suppressed, and (3) after `rate_limit` seconds (simulated), a third call with the same message/level is sent again.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+317
to
+326
| def test_trigger_log_rate_limit(mocked_trigger_logs): | ||
| trigger = DummyTrigger() | ||
| trigger.LOGS_MAX_BATCH_SIZE = 0 | ||
|
|
||
| assert mocked_trigger_logs.call_count == 0 | ||
|
|
||
| trigger.log("test message", "info") | ||
| trigger.log("test message", "info") | ||
| assert mocked_trigger_logs.call_count == 1 | ||
| assert len(mocked_trigger_logs.last_request.json()["logs"]) == 1 |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add a test that verifies the rate limit window elapsing allows the same log to be sent again.
This test only verifies that consecutive identical logs are rate-limited. To also validate the time-based behavior, please add a test that controls the rate limit window (e.g., by temporarily setting LOGS_RATE_LIMIT["info"] to a small value or monkeypatching time.time) and asserts that: (1) the first call is sent, (2) an immediate second call is suppressed, and (3) after rate_limit seconds (simulated), a third call with the same message/level is sent again.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #204 +/- ##
==========================================
+ Coverage 91.55% 91.58% +0.02%
==========================================
Files 49 49
Lines 3187 3197 +10
==========================================
+ Hits 2918 2928 +10
Misses 269 269 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Darkheir
force-pushed
the
feat/logs_rate_limit
branch
from
4e5e324 to
6a4b636
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Darkheir
force-pushed
the
feat/logs_rate_limit
branch
from
6a4b636 to
d96e57c
Compare
otetard
previously approved these changes
Dec 15, 2025
Signed-off-by: Darkheir <[email protected]>
Darkheir
force-pushed
the
feat/logs_rate_limit
branch
from
d96e57c to
600751d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR introduces rate limiting for logs sent by connectors and triggers.
It rate limits sending the same log (same message and same level) to the API.
Summary by Sourcery
Introduce rate limiting for trigger logs to avoid repeatedly sending identical log entries to the API.
Enhancements:
Tests: