Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe score monitor now gets daily call and lead counts from a date-bounded SQL aggregate through a new database accessor. The change removes the previous call-list and per-lead analytics accessors and adds tests for aggregate mapping and empty results. ChangesDaily Statistics Aggregation
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant ScoreMonitor._get_daily_call_stats
participant get_daily_summary_stats
participant Database
ScoreMonitor._get_daily_call_stats->>get_daily_summary_stats: Request aggregate for the 24-hour date range
get_daily_summary_stats->>Database: Execute date-bounded aggregate query
Database-->>get_daily_summary_stats: Return aggregate row or no rows
get_daily_summary_stats-->>ScoreMonitor._get_daily_call_stats: Return first row or None
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The daily summary’s checked counts retain their previous meaning, with no identified issue requiring a fix before merge. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change addresses a reported pod-memory failure without evidence of broader data access or a new externally reachable path. Its remaining risk is whether the database can execute the daily aggregation reliably at production volume. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit checks the daily count, Comment |
…h of call trackers The daily Slack summary in langfuse_score_monitor loaded every lead_call_tracker row of the last 24h (SELECT lct.*, ~143k rows, 1.3 GB on disk) into Python just to count status/outcome/provider. The pod running it grew to ~5.4 GB and was evicted for node memory pressure. The "summary sent" Redis key is only set on success, so every 10-min run retried on another pod: 6 evictions per night at 23:30-00:30 IST since 22 Sep, Buddy LB 5xx alerts, and no Slack summary since 21 Sep. get_daily_summary_stats_query returns one row (call counts, provider split, per-request_id lead counts) with the same rules as the Python loops. On the 21 Sep window it reproduces the logged stats and runs in ~0.15s vs 2m13s. Remove get_all_lead_call_trackers and the lead_call_tracker get_lead_based_analytics accessor (and their query builders/exports): the summary was their only caller, and an unbounded full-row fetch by date range is the pattern that caused this. The analytics dashboard's own get_lead_based_analytics handler is unrelated and unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
0761e0c to
aef8f97
Compare
Problem
langfuse_score_monitorbuilds the daily Slack summary at 23:30 IST._get_daily_call_statsranSELECT lct.*over the last 24h oflead_call_trackerwith no LIMIT (~143k rows, 1.3 GB on disk, full JSON columns), decoded every row into a model, and only countedstatus/outcome/provider.Fix
A new
get_daily_summary_stats_queryreturns one row with the same counts, using the same rules as the old Python loops:FINISHEDcount;NO_ANSWER/CONFIRM/CANCEL/ADDRESS_UPDATED/BUSYcounts; provider split (TWILIO / EXOTEL / PLIVO)request_id: total, picked (finished > no_answer), confirmed, cancelled, address-updated_get_daily_call_statsnow reads this row. The derived percentages, the stats dict and the Slack message are unchanged, so the summary loses nothing. Memory is now flat no matter how many calls there are.Removed dead code
get_all_lead_call_trackersand thelead_call_trackeraccessorget_lead_based_analyticsare deleted, along with their query builders and theapp/database/accessor/__init__.pyexports. The summary was their only caller (checked with a repo-wide grep; the dynamicimport_modulesites only load intent and UI modules). An unbounded full-row fetch by date range is exactly the pattern that caused this, so it shouldn't be left around for reuse. The analytics dashboard's ownget_lead_based_analyticshandler (analytics/handlers.py) is a different function and is unchanged.Verification
calls_attempteddiffers, by +7, for calls that finished after the snapshot). It runs in ~0.15s vs 2m13s.tests/test_langfuse_daily_summary_stats.pypins the output to the exact dict prod logged on 21 Sep, plus the zero fallback.pytest tests: 3586 passed.pyrefly check: 0 errors. black / isort / autoflake clean.Deploy note
This needs to be live before 23:30 IST tonight, or the same evictions repeat.
🤖 Generated with Claude Code
Summary by CodeRabbit