feat(admin): CSV export for growth-report (Glue/Athena/QuickSight) - #27
Open
LeoRoccoBreedt wants to merge 15 commits into
Open
feat(admin): CSV export for growth-report (Glue/Athena/QuickSight)#27LeoRoccoBreedt wants to merge 15 commits into
LeoRoccoBreedt wants to merge 15 commits into
Conversation
Pure functions that turn parsed UserRecord/WorkspaceRecord objects into Glue-ready CSV rows for three flat tables (users, workspaces, org KPIs). Built from the parsed records rather than the HTML report's display- formatted report_data so numeric columns type correctly in a Glue crawler.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Wires the CLI up to the csv_dir/no_html/chargeback kwargs generate_growth_report already supports. Adds argparse validation for --no-html requiring --csv-dir, and a distinct error for an unreadable/malformed --chargeback-report file (separate from the "requires an admin API key" message). Also strengthens test_html_still_written_when_csv_dir_absent to assert byte-identical HTML output with/without --csv-dir under a frozen clock, rather than just file existence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Regenerate the customer-facing sample CSVs to match the final, customer-agreed schema (report_date instead of report_month, no workspace column in growth_users.csv, plus em_last_used_at / opik_last_used_at), remove the superseded Option B/C samples, rewrite docs/csv-samples/README.md around the single agreed schema, and add a CSV export subsection to README-ADMIN.md documenting the --csv-dir / --no-html / --chargeback-report flags. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two pre-merge review fixes for the growth-report CSV export. 1. CSV write failures exited 0. The generic `except Exception` handler in the growth-report dispatch printed the error and returned, so an unwritable --csv-dir (or one pointing at a file) reported success while shipping nothing -- the worst failure shape for the monthly scheduled run this feature is built for. Now calls sys.exit(1), matching the sibling GrowthReportError handler, and preserving --debug tracebacks. 2. CSVs were written without an explicit encoding, falling back to the platform locale. Under LANG=C (typical for cron/systemd) a single non-ASCII username raised UnicodeEncodeError mid-write, leaving a truncated CSV for Glue to crawl. Now opens with encoding="utf-8", matching admin_growth_render.py and utils.py. Tests: a regression test driving admin() to a non-zero SystemExit with --csv-dir pointing at a file; a UTF-8 round-trip through DictReader; and a subprocess test that runs the write in a genuinely ASCII locale (the bug cannot be reproduced in-process -- open() resolves its default encoding in C, and PEP 538 coercion turns LANG=C back into UTF-8). A third reported finding (total_users disagreeing with the users table) is deliberately NOT addressed here: the prescribed fix does not achieve its stated goal. See the task report for the analysis. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… count `total_users` in growth_org_kpis.csv excludes suspended accounts (it is the licensing/adoption denominator behind active_users_pct), while growth_users.csv carries one row per non-deleted user, suspended included. The two therefore differ for any org with suspended or deleted accounts. This is intentional, so document it rather than changing the metric -- the users table exposes is_suspended, so a dashboard can reproduce either definition from the row data. Adds a caveat to the CSV export section of README-ADMIN.md (matching the existing "no workspace column" caveat) and a two-sentence note to the customer-facing docs/csv-samples/README.md. Also corrects the sample KPI values, which were hand-authored on the naive assumption and contradicted the behavior now documented: with 6 user rows of which 1 is suspended, total_users is 5 (not 6) and active_users_pct is 80.0 (not 66.7). new_users_in_window_pct was likewise wrong at 16.7 -- the real `_window_growth` divides new-in-window by accounts pre-dating the window (1/5 = 20.0), not by the total. The samples now demonstrate the documented rule instead of teaching against it. Regenerated the CSVs and rebuilt the zip. No behavior change: total_users, active_users_60d, active_users_pct and adoption_stats are untouched. Full suite 235 passed, unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
docs/ holds design specs, plans, customer correspondence and customer-deliverable CSV samples — working artifacts, not product code. This is a public repo, so they stay local. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
LeoRoccoBreedt
force-pushed
the
feat/growth-report-csv-export
branch
from
September 3, 2026 19:45
c980a0a to
6532eb2
Compare
The C-locale regression test passed the non-ASCII payload through
`python -c`, so the subprocess had to decode its own command line using
the ASCII locale the test deliberately sets. On Linux CI that fails
before any of our code runs ("Unable to decode the command from the
command line"); macOS happened to tolerate it.
Write the script to a UTF-8 file with a coding declaration and pass the
path instead: argv stays pure ASCII while the source is still read as
UTF-8. Verified still red/green -- reverting the `encoding="utf-8"` fix
in _write_csv reproduces the UnicodeEncodeError the test guards.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`comet_ml.exceptions.NotFound.__str__` returns None when the 404 body is not JSON -- an HTML error page from a proxy or ingress, for instance. The admin error handler then died with "TypeError: __str__ returned non-string (type NoneType)", replacing the real HTTP error with a traceback from the handler itself. An operator hitting a wrong or unavailable endpoint saw no usable diagnosis. Route the handler through `_exception_text`, which falls back to the exception class name plus the response status code when `str(exc)` is empty or raises. A 404 on the chargeback endpoint now prints "ERROR: NotFound (HTTP 404)". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Running the helper script by path sets sys.path[0] to the script's own directory (tmp_path) rather than the CWD, so `import cometx` failed on CI with ModuleNotFoundError. It passed locally only because an editable install put the package on sys.path anyway -- exactly the difference between a dev checkout and a clean CI checkout. Pass the repo root explicitly via PYTHONPATH, derived from the imported package rather than hardcoded. Verified against a simulated CI setup (subprocess run from a different cwd with no editable install visible), and still red/green: reverting the encoding="utf-8" fix reproduces the UnicodeEncodeError. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Running against a real deployment surfaced a confusing gap: the total_users KPI read 434 while growth_users.csv had 408 rows, with nothing in the data explaining the difference. The two exclude different populations -- total_users excludes suspended accounts, the users table excludes deleted ones -- so a dashboard showed two tiles disagreeing by 27 with no way to reconcile them. Add a `deleted_at` column (appended last, always empty since deleted users remain filtered out, so no existing sum changes meaning) and a `deleted_users` KPI. The two files now reconcile explicitly: total_users - deleted_users + suspended = users-table row count verified as 434 - 27 + 1 = 408 against the real data. No change to adoption_stats or total_users semantics -- that remains a separate decision. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
LeoRoccoBreedt
marked this pull request as ready for review
September 4, 2026 13:24
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
Four fixes from the automated review, two of which were real bugs I reproduced before changing anything. 1. Reconciliation broke on deleted+suspended users (logic bug). The documented identity `total_users - deleted_users + suspended` assumed the two exclusions were disjoint. An account carrying BOTH flags is absent from total_users (suspended) AND counted in deleted_users (deleted), so subtracting removed it twice: with 1 live, 1 suspended, 1 deleted and 1 deleted+suspended, the formula predicted 1 row where there were 2. Publish the row count directly as a `users_in_table` KPI instead of asking consumers to derive it, and document that the arithmetic is wrong. Regression test pins the four-way case and asserts the old identity genuinely fails on it. 2. `service_account_source` put a string in the numeric `metric_value` column, which makes a Glue crawler type the whole column as `string` and forces a cast on every SUM/AVG in QuickSight. Add a `metric_text` column for label-unit payloads; `metric_value` is now strictly numeric or empty. 3. `--chargeback-report` opened the snapshot with the platform default encoding, so a file with non-ASCII usernames failed to load under LANG=C -- the same bug already fixed on the CSV write path. 4. Docs claimed `--chargeback-report` avoided the API entirely. It skips the chargeback request only; /api/admin/service-accounts is still queried (verified by instrumenting the call). Corrected both READMEs rather than dropping the call, which is what makes is_service_account authoritative. Samples and zip regenerated against the new schema. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Python's default float repr switches to exponent form outside roughly 1e-5 .. 1e16, so a workspace with a very small data_mb rendered as "1e-05". Athena's CSV SerDe does not parse that as a double -- the value silently becomes NULL in the dashboard, which is the worst failure shape for this pipeline: no error, just a wrong number. The low bound is reachable in practice (a near-empty workspace logging a few bytes), and the test cluster has 500 empty workspaces. Format floats explicitly with %f, trimming padding zeros, and map nan/inf to an empty field rather than emitting text that would force the whole column to type as string. Ints pass through untouched -- arbitrary precision, never exponential. Real chargeback magnitudes (20545.68, 88834.25, 71200.75) still round-trip exactly. Tests updated to compare rendered values numerically rather than by Python type, since floats are now decimal strings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
baz-reviewer
Bot
dismissed
their stale review
September 4, 2026 15:32
Baz dismissed its prior approval because a re-review found new findings.
Comment on lines
+94
to
+100
| if isinstance(value, float): | ||
| # 'f' never uses exponent form; normalize -0.0 and trim the trailing | ||
| # zeros it pads with, leaving at least one decimal place. | ||
| if value != value or value in (float("inf"), float("-inf")): | ||
| return "" # NaN/inf have no honest CSV representation | ||
| text = "%.6f" % value | ||
| text = text.rstrip("0") |
There was a problem hiding this comment.
Numeric export silently loses precision
_num_or_empty uses "%.6f", so valid metric values are quantized and exact parsed fact-table values are lost — should we use a plain-decimal representation that round-trips finite floats without exponent notation?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
cometx/cli/admin_growth_csv.py around lines 94-100, update `_num_or_empty` so finite
floats are rendered in plain decimal notation without the six-place quantization caused
by `"%.6f"`. Use a representation derived from the float’s round-trippable value
(expanding exponent notation, for example via `Decimal` or equivalent), while preserving
the existing empty handling for NaN and infinities and normalizing negative zero.
Contributor
Author
There was a problem hiding this comment.
Correct, and a good catch — fixed in 6647c7f. I introduced %.6f in the previous commit to kill scientific notation and flagged the precision tradeoff to myself without acting on it; you were right that it was not acceptable. Five of six sample values lost data (0.1234567 -> 0.123457), which trades a silent NULL in Athena for a silently wrong number. That is the worse failure.
Now: use repr() whenever it does not produce an exponent — it is the shortest string that round-trips to the identical float — and fall back to exact positional expansion via Decimal only when repr goes exponential. Decimal(float) is lossless, so nothing is rounded.
Every tested value now round-trips exactly and contains no exponent, including the real chargeback magnitudes (20545.68, 88834.25, 71200.75) and the pathological ends (1e-07, 1e20, 1/3, 0.1+0.2). Added a test_no_float_is_quantized regression asserting both properties together, so a future change cannot fix one at the other's expense.
There was a problem hiding this comment.
Commit 6647c7f addressed this comment by replacing %.6f quantization with round-trippable repr output and exact plain-decimal expansion for exponent-form floats, while preserving NaN/infinity handling and negative-zero normalization.
The previous fix used "%.6f", which avoided scientific notation but quantized ordinary values: 0.1234567 became 0.123457. That traded a silent NULL in Athena for a silently wrong number, which is worse. Five of six sample values lost data. Use repr() whenever it does not produce an exponent -- it is the shortest string that round-trips to the identical float -- and fall back to exact positional expansion via Decimal only when repr goes exponential. Decimal(float) is lossless, so nothing is rounded. Every tested value now round-trips exactly AND contains no exponent, including the real chargeback magnitudes and the pathological ends (1e-07, 1e20, 1/3, 0.1+0.2). Reported by Baz review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
User description
Why
A customer runs
cometx admin growth-reportmonthly and feeds the results into a dashboard: CSV → S3 → AWS Glue tables → Athena → QuickSight. Today the command only emits a self-contained HTML page, which that pipeline can't consume. They asked for CSV output of the aggregated data, plus a definite schema so Glue tables can be created against it once and not break on later runs.What this adds
Three new flags on
growth-report:--csv-dir DIRDIR--no-html--csv-dirfor CSV-only runs)--chargeback-report FILEOutput
growth_users.csvgrowth_workspaces.csvgrowth_org_kpis.csvDesign decisions (agreed with the customer)
Full-grain fact tables, not the HTML's derived views. The report's leaderboards and KPIs are top-N slices and sums of these same records. Exporting the full grain lets QuickSight reproduce every chart and answer questions the HTML doesn't, without a new cometx release per requested cut.
No
workspacecolumn on the users table. Chargeback reportsexperiment_count/data_logged_mb/opik_span_countper user, not per (user, workspace). A row per (user, workspace) would repeat each user's totals and makeSUM()over-count. One row per user means a plainSUMis correct with noDISTINCT. Exact per-workspace totals live ingrowth_workspaces.csv. Documented trade-off: per-workspace user breakdowns aren't answerable from this export.Org KPIs are long-format (
metric_name/metric_value) so new metrics arrive as new rows — the Glue schema never changes and existing partitions stay readable.CSV is built from the parsed records, never from
report_data. This is the load-bearing decision:report_dataholds display-formatted strings (_num()inserts thousands separators, rates carry%), which would make a Glue crawler type those columns asstringand silently break aggregation in the dashboard.Validated against a real deployment
Run against a live self-hosted EKS cluster (408 users, 523 workspaces):
%suffixes, or other Glue-hostile formatting in any value.Compatibility
Existing behavior is unchanged when the new flags aren't passed — enforced by a test asserting the HTML is byte-identical (frozen clock, SHA-256) with and without
--csv-dir.Testing
240 unit tests pass (baseline was 207). Coverage includes: one row per multi-workspace user, deleted excluded / suspended flagged, epoch-ms → ISO dates,
None→ empty field (distinct from a real0), no separators or%in any value, header written even with zero rows, non-ASCII round-trip under a C locale, and non-zero exit on write failure.Notes for the reviewer
Two pre-existing issues fixed here, both found while running against the real cluster:
--csv-dirprinted an error but reported success — the worst shape for a scheduled monthly job, which would report success while shipping nothing. Now exits non-zero.__str__.comet_ml.exceptions.NotFound.__str__returnsNonewhen the 404 body isn't JSON (an HTML page from a proxy/ingress), sostr(exc)raisedTypeErrorand replaced the real HTTP error with a traceback from the handler itself. A 404 now printsERROR: NotFound (HTTP 404).Two documented quirks a reviewer will notice in the data:
total_userswon't equal the users-table row count. The KPI excludes suspended users; the table excludes deleted ones. On the real deployment: 434 vs 408. Adeleted_usersKPI makes this reconcile explicitly (434 − 27 + 1 = 408). This is pre-existing behavior —adoption_statsis untouched by this PR and the HTML report has shown 434 all along. Changing the metric's semantics is deliberately deferred as a separate decision.Implementation notes:
admin_growth_csv.pydeliberately does not import fromadmin_growth_report.py(that module imports fromadmin_growth_users.py; importing back would be circular). It takes already-parsed records as arguments.report_dateis the UTC run date, injectable as a parameter for tests but intentionally not exposed as a CLI flag.deleted_atis emitted as a column but is always empty, since deleted users remain filtered out. It exists so the column is typed for a Glue crawler and the schema needn't change if that policy is revisited.🤖 Generated with Claude Code
Generated description
Below is a concise technical summary of the changes proposed in this PR:
graph LR admin_("admin"):::modified generate_growth_report_("generate_growth_report"):::modified GrowthReporter_build_("GrowthReporter.build"):::modified ADMIN_CHARGEBACK_API_("ADMIN_CHARGEBACK_API"):::modified GrowthReporter_assemble_report_data_("GrowthReporter._assemble_report_data"):::modified ADMIN_SERVICE_ACCOUNTS_API_("ADMIN_SERVICE_ACCOUNTS_API"):::modified collect_org_kpis_("collect_org_kpis"):::modified write_growth_csvs_("write_growth_csvs"):::added write_csv_("_write_csv"):::added admin_ -- "Passes CSV output, HTML suppression, and preloaded chargeback options." --> generate_growth_report_ generate_growth_report_ -- "Supplies optional chargeback payload and enables CSV record reuse." --> GrowthReporter_build_ GrowthReporter_build_ -- "Skips chargeback API requests when a local payload is supplied." --> ADMIN_CHARGEBACK_API_ GrowthReporter_build_ -- "Assembly now captures parsed users, workspaces, and organization KPI records." --> GrowthReporter_assemble_report_data_ GrowthReporter_assemble_report_data_ -- "Stores fetched service-account names for consistent CSV classification." --> ADMIN_SERVICE_ACCOUNTS_API_ GrowthReporter_assemble_report_data_ -- "Converts parsed usage, growth, workspace, and account data into KPI rows." --> collect_org_kpis_ generate_growth_report_ -- "Writes three Glue-ready CSV fact tables when CSV output is requested." --> write_growth_csvs_ write_growth_csvs_ -- "Serializes headers and rows into UTF-8 newline-terminated CSV files." --> write_csv_ classDef added stroke:#15AA7A classDef removed stroke:#CD5270 classDef modified stroke:#EDAC4C linkStyle default stroke:#CBD5E1,font-size:13pxAdd a Glue-ready export pipeline to
growth-reportthat writes typed user, workspace, and organization KPI fact tables from parsed chargeback records, while supporting local snapshots and CSV-only execution. Preserve existing HTML behavior and improve operational reliability through deterministic schemas, UTF-8-safe output, non-zero export failures, and resilient exception reporting.--csv-dir,--no-html, and--chargeback-reportworkflows, reuse parsed report state instead of display-formatted data, and document the resulting operational and data-model conventions for scheduled dashboard pipelines.Modified files (4)
Latest Contributors(2)
growth_users.csv,growth_workspaces.csv, and long-formatgrowth_org_kpis.csvfiles from parsed records, preserving user-level grain, workspace totals, numeric formatting, dates, missing values, service-account provenance, and reconciliation KPIs for S3, Glue, Athena, and QuickSight.Modified files (5)
Latest Contributors(2)
__str__implementation is broken, preserving byte-identical HTML behavior, and excluding local design and sample documentation from version control.Modified files (3)
Latest Contributors(2)
Customize your next review