chore: cap static analytics detail exports at top-n rows (#4924) - #4932
Conversation
There was a problem hiding this comment.
Pull request overview
Caps the size of selected static analytics JSON exports to reduce page-load payload and monthly regeneration diffs, while keeping uncapped exports that the template renders in full.
Changes:
- Added an optional
max_rowsparameter toexport_df_as_json()to export only the top-N rows (with a console log when truncation happens). - Introduced
MAX_TABLE_EXPORT_ROWS = 100and applied it topageviews.json,outbound_links.json, andfilter_selected.json. - Annotated the README output listing to document which exports are capped.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| analytics/analytics_package/analytics/static_site/export.py | Adds a configurable top-N row cap and applies it to the three sliced table exports. |
| analytics/analytics_package/analytics/static_site/README.md | Documents the three capped export files in the output tree. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
cc02e97 to
07a2555
Compare
note the export-side row cap at the template slice sites, so raising a slice above max_table_export_rows can't silently truncate a table. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
NoopDog
left a comment
There was a problem hiding this comment.
Reviewed at high effort. Verified the load-bearing claims rather than taking them on trust:
- Sortedness holds by construction. All three producers pass
sort_resultsintoget_one_period_change_df, which does a stable descendingsort_values(report_elements.py:282), and all nine committed JSONs undergh-pages/*/data/are in fact sorted descending on their metric — sohead(100)keeps the genuine top rows. - The cap runs after filtering, not before.
exclude_pagesand the suspicious-path filter execute infetch.py:452-465ahead of export, and both are order-preserving boolean masks. - No displayed aggregate is skewed.
pageviews.json/outbound_links.json/filter_selected.jsonare read only bytemplate/index.htmland its deployed copies, and only forslice(0,20)/slice(0,20)/slice(0,30)plus an emptiness check — no totals, counts or "Show all" toggles. The toggles at lines 750 and 970 belong to event detail and access requests, which correctly stay uncapped. - Signature change is backward compatible (
max_rowskeyword-with-default, no callers ofexport_df_as_jsonoutsideexport.py), and the empty/Nonepaths still emit[].
One low-severity note, not blocking: export.py:42 — the cap silently depends on the caller having sorted the frame, and nothing enforces or detects it. If a follow-up under the same "fewer diff lines per regen" motivation adds a deterministic secondary sort (e.g. by page for stable diffs), or a future caller builds a frame without sort_results, df.head(100) would keep 100 arbitrary rows while the log line still reads Capped ... at top 100 of 1810 rows. Sorting inside the function on the metric column, or asserting it is monotonically non-increasing before slicing, would make the contract enforced rather than documented.
Appreciate the description correcting the earlier "unit-tested" claim — the absence of a Python test harness is worth its own ticket.
Closes #4924
What changed
export_df_as_json()gains an optionalmax_rows; when the frame exceeds it, the top rows are kept and the truncation is logged (Capped pageviews.json at top 100 of 1810 rows) so the cap is never silent.MAX_TABLE_EXPORT_ROWS = 100is applied to the three exports the template renders as a fixed slice:pageviews.jsonslice(0, 20)outbound_links.jsonslice(0, 20)filter_selected.jsonslice(0, 30)Left uncapped on purpose: event detail tables, access requests and search queries — the template renders every row of those (behind a "Show all N rows" toggle, or with no slice at all for search), so capping them would silently drop visible data. The constant's comment records both halves of that rule, and the README's output listing now annotates the three capped files.
Measured on a 1,810-row frame: 120KB → 7KB (~95% smaller), which is also thousands of fewer diff lines per monthly regen.
Why top-N is safe here
The cap keeps whichever rows come first, so it depends on the frames being sorted. That holds by construction, not by luck: all three producers pass explicit
sort_resultsintoget_one_period_change_df, which does a stable descendingsort_valueson the current-period metric (report_elements.py—get_page_views_change,get_outbound_links_change,get_index_filter_selected_change), andfetch.py's later exclude-pages / suspicious-path filtering preserves order. The deployed JSONs are fully sorted descending.export_df_as_json's docstring states the expectation for future callers.Nothing consumes the full lists: within the template these three datasets are only ever sliced for their tables plus an emptiness check — no totals, counts or reductions are derived from them — and no other Python, notebook or TypeScript in the repo reads them. So no displayed aggregate can be skewed by the cap.
How verified
Manual local checks only — this PR adds no automated tests. The repo has no Python test harness: no
test_*.pyfiles,pytestis not a declared dependency, and no workflow runs Python tests. (Since #4943 there is ananalyticsCI job, but it runsruff check,ruff format --checkand animport analytics.static_sitesmoke test — no tests. It passes on this branch.) Flagging that explicitly because an earlier version of this description said "unit-tested", which wrongly implied in-repo coverage.What I actually ran: an ad-hoc script in a throwaway virtualenv that imports
export_df_as_jsonand exercises it against a synthetic 1,810-row frame — capped to exactly the top 100 with order preserved and NaN changes still nulled; a below-cap frame exported untouched; nomax_rowsstill exports every row (the event-detail path);Noneand empty frames still emit[]. That script was not committed.Adding a real pytest suite for this module and wiring it into CI is worth doing, but it would introduce a Python test harness this repo doesn't have yet — happy to do that here or as a follow-up ticket, whichever is preferred.
🤖 Generated with Claude Code