Discord: Automate Stat Messages#1012
Conversation
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds daily statistics aggregation across database tables, Discord message formatting and delivery, a protected monitored cron endpoint, webhook configuration, periodic invoker wiring, and service tests. ChangesDaily statistics pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CronInvoker
participant daily_stats_cron_job
participant collect_daily_stats
participant Database
participant Discord
CronInvoker->>daily_stats_cron_job: GET /cron/daily-stats
daily_stats_cron_job->>collect_daily_stats: optional window_hours
collect_daily_stats->>Database: execute time-windowed statistics queries
Database-->>collect_daily_stats: categorized statistics
daily_stats_cron_job->>Discord: post formatted message chunks
Discord-->>daily_stats_cron_job: HTTP response
Possibly related PRs
Suggested reviewers: 🚥 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. Comment |
github-actions
Bot
changed the title
OpenAPI changes ⚪ No API surface changesNote This PR does not modify the API contract.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Prajna1999
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
backend/app/tests/services/test_stats.py (1)
134-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilence Ruff unused-argument warnings in mock helpers.
Ruff flags unused
url,timeout(andjsoninflaky_post) in the mock callables. Use*_args, **_kwargsto match therequests.postsignature without naming unused parameters.♻️ Proposed fix
- def fake_post(url, json, timeout): + def fake_post(*_args, **kwargs): - posted.append(json["content"]) + posted.append(kwargs["json"]["content"])- def flaky_post(url, json, timeout): + def flaky_post(*_args, **_kwargs): calls["n"] += 1 raise requests.ConnectionError("nope")Also applies to: 161-161
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/tests/services/test_stats.py` at line 134, Update the mock helpers fake_post and flaky_post in the stats tests to accept unused positional and keyword arguments via *_args and **_kwargs instead of naming unused request parameters, while preserving each helper’s existing behavior.Source: Linters/SAST tools
backend/app/crud/stats.py (1)
76-88: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSQL identifier interpolation via f-string is a fragile pattern.
_org_count_sqlinterpolatestabledirectly into the SQL string. All current callers pass hardcoded literals, so there is no immediate injection risk, but this pattern will silently allow SQL injection if a future caller passes dynamic input. Consider adding a whitelist assertion or a comment documenting the constraint.🛡️ Proposed guard
def _org_count_sql(table: str) -> TextClause: + # table must be a hardcoded literal — never user input + assert table.isidentifier(), f"Invalid table name: {table}" return text( f"""🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/crud/stats.py` around lines 76 - 88, Protect the table identifier interpolated by `_org_count_sql` from future dynamic input. Add an explicit whitelist validation for the supported table names before constructing the SQL, or document and enforce that callers may only provide trusted constants; raise an appropriate error for unsupported values.backend/app/api/routes/cron.py (1)
151-163: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse consistent logging format.
Line 151 and 162 use f-strings while line 156 uses %-style interpolation. Prefer %-style for logging to enable lazy evaluation, or at minimum be consistent within the same function.
♻️ Suggested consistency fix (f-string → %-style)
def daily_stats_cron_job(session: SessionDep, hours: int | None = None) -> dict[str, Any]: - logger.info(f"[daily_stats_cron_job] Cron job invoked | hours={hours}") + logger.info("[daily_stats_cron_job] Cron job invoked | hours=%s", hours) try: result = collect_daily_stats(session=session, window_hours=hours) post_daily_stats_to_discord(format_daily_stats_message(result)) logger.info( "[daily_stats_cron_job] Completed | window: %s", result["window"], ) return result except Exception as e: logger.error( - f"[daily_stats_cron_job] Error executing cron job: {e}", + "[daily_stats_cron_job] Error executing cron job: %s", + e, exc_info=True, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/api/routes/cron.py` around lines 151 - 163, Use consistent lazy %-style logging in the daily_stats_cron_job function: replace the f-string messages in the invocation and exception logger calls with format strings and separate arguments, matching the existing completion log.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/api/routes/cron.py`:
- Line 150: Update the return annotation of daily_stats_cron_job from dict to
dict[str, Any], matching collect_daily_stats and the project’s requirement for
parameterized, specific return types; ensure Any is imported if needed.
In `@backend/app/crud/stats.py`:
- Line 76: Replace the Any annotations in _org_count_sql and _rows with
sqlalchemy.TextClause, importing TextClause as needed; keep the existing text()
SQL construction unchanged.
In `@backend/app/services/stats.py`:
- Around line 22-25: In the window calculation within the stats function,
replace the truthiness check on window_hours with an explicit None check so a
value of 0 produces a zero-duration window while only omitted values use
DAILY_WINDOW.
- Around line 120-131: Update _chunk_message to split any individual section
that exceeds _DISCORD_CHUNK_LIMIT into smaller newline-based chunks before or
during normal paragraph chunking, ensuring every returned chunk stays within the
limit. Preserve section content and ordering so post_daily_stats_to_discord can
send the complete digest without oversized Discord messages.
In `@scripts/python/invoke-cron.py`:
- Line 21: The ENDPOINTS list in invoke-cron.py incorrectly includes
/api/v1/cron/daily-stats in the shared five-minute loop, causing repeated daily
digests. Remove it from the shared list and invoke it through a separate daily
scheduler or guard it with a daily-only condition consistent with the configured
0 0 * * * schedule.
---
Nitpick comments:
In `@backend/app/api/routes/cron.py`:
- Around line 151-163: Use consistent lazy %-style logging in the
daily_stats_cron_job function: replace the f-string messages in the invocation
and exception logger calls with format strings and separate arguments, matching
the existing completion log.
In `@backend/app/crud/stats.py`:
- Around line 76-88: Protect the table identifier interpolated by
`_org_count_sql` from future dynamic input. Add an explicit whitelist validation
for the supported table names before constructing the SQL, or document and
enforce that callers may only provide trusted constants; raise an appropriate
error for unsupported values.
In `@backend/app/tests/services/test_stats.py`:
- Line 134: Update the mock helpers fake_post and flaky_post in the stats tests
to accept unused positional and keyword arguments via *_args and **_kwargs
instead of naming unused request parameters, while preserving each helper’s
existing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1695d50d-6e21-4a4d-848b-95ea5e43a0ae
📒 Files selected for processing (6)
backend/app/api/routes/cron.pybackend/app/core/config.pybackend/app/crud/stats.pybackend/app/services/stats.pybackend/app/tests/services/test_stats.pyscripts/python/invoke-cron.py
| monitor_slug="daily-stats-cron-job", | ||
| monitor_config=DAILY_STATS_CRON_MONITOR_CONFIG, | ||
| ) | ||
| def daily_stats_cron_job(session: SessionDep, hours: int | None = None) -> dict: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a parameterized return type.
-> dict is unparameterized (equivalent to dict[Any, Any]). Per coding guidelines, type hints must be specific; use dict[str, Any] to match the return type of collect_daily_stats.
As per coding guidelines: "Use type hints on every function parameter and return value; -> Any is not acceptable unless the type can be narrowed."
🔧 Proposed fix
-def daily_stats_cron_job(session: SessionDep, hours: int | None = None) -> dict:
+def daily_stats_cron_job(session: SessionDep, hours: int | None = None) -> dict[str, Any]:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def daily_stats_cron_job(session: SessionDep, hours: int | None = None) -> dict: | |
| def daily_stats_cron_job(session: SessionDep, hours: int | None = None) -> dict[str, Any]: |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/api/routes/cron.py` at line 150, Update the return annotation of
daily_stats_cron_job from dict to dict[str, Any], matching collect_daily_stats
and the project’s requirement for parameterized, specific return types; ensure
Any is imported if needed.
Source: Coding guidelines
| ) | ||
|
|
||
|
|
||
| def _org_count_sql(table: str) -> Any: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Narrow Any type hints to TextClause.
_org_count_sql returns -> Any and _rows accepts stmt: Any, but both can be narrowed to sqlalchemy.TextClause since text() always returns that type. As per coding guidelines, -> Any is not acceptable unless the type cannot be narrowed.
♻️ Proposed fix
-from sqlalchemy import text
+from sqlalchemy import TextClause, text
-def _org_count_sql(table: str) -> Any:
+def _org_count_sql(table: str) -> TextClause:
-def _rows(session: Session, stmt: Any, params: dict[str, Any]) -> list[dict[str, Any]]:
+def _rows(session: Session, stmt: TextClause, params: dict[str, Any]) -> list[dict[str, Any]]:Also applies to: 97-97
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/crud/stats.py` at line 76, Replace the Any annotations in
_org_count_sql and _rows with sqlalchemy.TextClause, importing TextClause as
needed; keep the existing text() SQL construction unchanged.
Source: Coding guidelines
| end_at = now() | ||
| start_at = end_at - ( | ||
| timedelta(hours=window_hours) if window_hours else DAILY_WINDOW | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
window_hours=0 silently falls back to the 7-day default.
if window_hours else DAILY_WINDOW treats 0 as falsy. Passing hours=0 would return a 7-day window instead of an empty one. Use an explicit None check.
🐛 Proposed fix
- start_at = end_at - (
- timedelta(hours=window_hours) if window_hours else DAILY_WINDOW
- )
+ start_at = end_at - (
+ timedelta(hours=window_hours) if window_hours is not None else DAILY_WINDOW
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| end_at = now() | |
| start_at = end_at - ( | |
| timedelta(hours=window_hours) if window_hours else DAILY_WINDOW | |
| ) | |
| end_at = now() | |
| start_at = end_at - ( | |
| timedelta(hours=window_hours) if window_hours is not None else DAILY_WINDOW | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/services/stats.py` around lines 22 - 25, In the window
calculation within the stats function, replace the truthiness check on
window_hours with an explicit None check so a value of 0 produces a
zero-duration window while only omitted values use DAILY_WINDOW.
| ) | ||
|
|
||
|
|
||
| def _org_count_sql(table: str) -> Any: |
There was a problem hiding this comment.
I think we can avoid the any type here. This is good suggestion given by coderabbit https://github.com/ProjectTech4DevAI/kaapi-backend/pull/1012/changes#r3558059640. Please handle it.
| def section_counts(result: dict[str, Any]) -> dict[str, int]: | ||
| return { | ||
| section: len(rows) | ||
| for section, rows in result["stats"].items() | ||
| if isinstance(rows, list) | ||
| } |
There was a problem hiding this comment.
this function is not used anywhere, if this not needed please remove this dead code.
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| DAILY_WINDOW = timedelta(hours=168) |
There was a problem hiding this comment.
this timedelta(hours=168) is 7 days, not daily. as I can see this cron(here) run daily but posts a rolling 7-day window every run → ~85% of each day's report overlaps yesterday's. Intended? If yes, rename to DEFAULT_STATS_WINDOW / WEEKLY_WINDOW so the daily name doesn't mislead. If the intent was true daily deltas, then I think window should be 24h.
Ayush8923
added
reviewed
and removed
enhancement
2 participants
Issue
Closes #825
Summary
Automates a daily platform stats digest to a Discord channel via webhook. Adds /cron/daily-stats that runs a set of per-organization SQL rollups (LLM call counts, total tokens by model, modality mix, job types, evaluation runs, STT/TTS results, assessments), formats them as compact plain-text tables, chunks past Discord's 2000-char limit, and posts fire-and-forget so a webhook outage never breaks the cron.
Query/format/post logic lives in
app/services/stats.py;the route is a thin orchestrator. Window defaults to 7 days and accepts a ?hours=N override for backfills.Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Summary by CodeRabbit