-
Notifications
You must be signed in to change notification settings - Fork 0
Setup tweet cap alert #47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feat/configurable-whitelist
Are you sure you want to change the base?
Conversation
| - _Bad_: `types.rs`, `impls.rs` | ||
| - _Good_: `user.rs` containing `struct User` and `impl User`. | ||
|
|
||
| ## 2. Code Structure & Patterns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should not be checked in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review: Feat - Setup Tweet Cap Alert
This PR implements tracking of Twitter API usage and alerting when a threshold is reached. This is crucial for managing API costs/limits.
1. Suggestion: Alert Frequency
In src/services/alert_service.rs, the alert logic triggers whenever the current total exceeds the threshold:
if current_total >= self.config.tweet_sync.alert_threshold {
// Sends alert...
}Observation: Once the threshold is reached, every subsequent sync (e.g., every 10 minutes) will trigger another alert until the billing period resets.
Recommendation: Since the interval is 10 minutes, this spam might be acceptable for visibility. However, if you want to reduce noise in the future, you could modify the logic to only alert when the threshold is just crossed, or add a cooldown.
// Optional: Only alert if we crossed the line just now
if current_total >= threshold && previous_total < threshold {
self.send_twitter_limit_alert(...).await?;
}2. Period Calculation & Edge Cases
The billing period calculation in TweetPullUsageRepository::calculate_period_for_date is clever in handling short months, but "reset day" logic can be tricky.
- Observation: If
reset_dayis 31:- Feb 28th (non-leap) -> Period "Jan" (Start Jan 31).
- March 1st -> Period "Feb" (Start Feb 28/29 effective).
- This effectively shifts the "month label" by one. As long as this is consistent with your billing provider (e.g., X API billing cycle), it is fine. Just verify that the resulting "Period String" (e.g., "2023-01") matches the billing invoice you want to correlate it with.
3. Usage Tracking Resilience
The increment_usage method uses an atomic DB update (SET tweet_count = ... + EXCLUDED.tweet_count), which is excellent for concurrency.
However, consider the crash scenario in TweetSynchronizerService:
- Tweets are pulled and saved.
track_and_alert_usageis called.- If Step 2 fails (DB connection blip), the error is logged but the sync function continues (
tracing::error!). - Since the tweets were saved,
last_idwill be updated on the next run, so these tweets won't be pulled again. - Result: These tweets are never counted in the usage stats.
Recommendation: While perfect accuracy isn't required for an alert system, be aware that this is "at most once" counting behavior in failure modes. If stricter accounting is needed, usage increment should happen in the same transaction as saving the tweets (or relevant_tweets insertion). Given this is for "alerts" and not "billing audits", the current approach is likely acceptable.
4. Minor: Error Handling
In AlertService::send_webhook_alert:
Err(crate::AppError::Server(format!("Webhook alert failed with status {}: {}", status, body)))This is a good use of the existing AppError. Ensure that AppError::Server is logged appropriately upstream.
Summary
The logic is solid. The alert frequency (every 10 minutes once threshold is met) is noted as acceptable for current operations.
|
Note: I don't think alert spamming is critical when it's only every 10 minutes. It should kinda make noise, and every 10 minutes is not so bad. |
Summary
When we pull tweets we track the usage and give alert if certain threshold has passed, so we know we are running out of quota.
Changes