feat(admin): chargeback-only growth-report (org overview, users, leaderboards, personal-vs-service) - #26
Conversation
Baz dismissed its prior approval because a re-review found new findings.
|
Thanks for the review — all 7 findings addressed in 89d6119:
Added regression tests for the empty-admin-set, scope-count, zero-member-seeding, and units-adverb fixes; full unit suite 149 passing. |
|
Addressed the two new findings (plus three correctness bugs a high-effort review pass surfaced) in a5031d3: Baz findings
Also fixed (from a high-effort review pass):
Regression tests added for all five; full unit suite 154 passing. |
|
Addressed in 9c9211e: factored the new_in/before/pct window math into a shared |
dsblank
left a comment
There was a problem hiding this comment.
Overview
This PR pivots cometx admin growth-report from SDK/platform-direct collection to a chargeback-only report. Net effect (12 files, +3484/−2679):
cometx/utils.py— newfetch_chargeback_report(), now the single source for/api/admin/chargeback/report(used bychargeback-report,migrate-users,growth-report).cometx/cli/admin_growth_users.py(new, 713 lines) — pure parse/derive layer:UserRecord/WorkspaceRecord, adoption + churn + capability time series, leaderboard ranking, personal-vs-service classification.cometx/cli/admin_growth_report.py— rewrittenGrowthReporter: hard-fails withGrowthReportErrorwithout an admin key, assembles four sections (org overview / users / leaderboards / personal-vs-service).cometx/cli/admin_growth_render.py— chart contract change:area/stackedBars→lines/barsLine, window-band overlay removed, horizontal gridlines added; collector chips and product headings dropped.- CLI:
--platforms/--limitremoved;--active-window,--leaderboard-top-n,--exclude-personal,--personal-patternadded.
Verified locally: 154 unit tests pass; black/isort clean on the touched files.
Findings
1. "Workspaces: total vs. active" total line is flat by construction — contradicts the chart next to it
workspace_active_series seeds every bucket's total from all_workspaces (admin_growth_users.py:460,466), so the total is the current workspace count for all history. Confirmed empirically — two workspaces, one whose only member was created in 2026:
first {'key': '2023-10', 'values': {'total': 2, 'active': 0}}
last {'key': '2026-07', 'values': {'total': 2, 'active': 2}}
The docstring argues this keeps the chart consistent with the KPI denominator, but the same section renders "Workspaces added vs. deleted", which does show growth over time. A reader sees "we always had N workspaces" beside "we added workspaces monthly". Suggested fix: derive per-bucket total from workspaces whose earliest member existed by bucket-end, and seed only the workspaces that have no datable members (the ones the reverse-map can't see) — that keeps the final bucket matching the KPI without falsifying history.
2. The growth-rate line in barsLine has no readable scale
drawBarsLine uses a second y-scale (rmax, admin_growth_render.py) but hGrid only labels the left/bar axis, and attachTip was not wired into drawLines/drawBarsLine (it survives only in drawBars/drawGroupedH). So the growth-rate percentage line — and every value in every lines chart — is shape-only: no axis labels on its scale, no hover values, and x labels only at the first and last key. Either add a right-axis label pair for the line scale and reattach attachTip to both new chart kinds, or express the rate on the bar axis.
3. Growth-rate denominator ignores deletions
In _build_unified_section (admin_growth_report.py:501-521), prev_total accumulates added only and never subtracts deleted, while the chart plots deleted bars right beside the line. The first bucket is also always 0.0 by construction. Worth either subtracting deletions or renaming the series to "added / cumulative added".
4. Service-account container-key guessing can silently invert the split
_extract_service_account_names (admin_growth_report.py:134) accepts "users" as a service-account container. If /api/admin/service-accounts ever returns a general user list under that key, every user is classified as a service account — and the report labels it "Source: service accounts from admin API", i.e. authoritative. The mirror risk: if entries carry only email (:150 accepts it) while classify_accounts matches on user.username (admin_growth_users.py:255), nothing matches and everything lands in "Personal", again labeled authoritative with no heuristic fallback. Recommend narrowing the accepted keys (drop "users") and matching membership on username or email.
5. New in <window> (% of base) will usually read 0 for workspaces
_workspace_growth_kpi proxies workspace creation with the earliest member created_at (admin_growth_report.py:439-453). A new workspace staffed by existing users is therefore never "new", and with the default --window 7d this KPI will typically show 0.0% / +0 new, looking broken rather than approximate. The proxy is documented in code but nothing surfaces it in the HTML — add a sub/hint noting it's a membership proxy.
6. Scoped and --exclude-personal runs mix scopes silently
_scope_chargebackfilters users to members of the selected workspaces, but chargeback per-user totals are org-wide, so a scoped run attributes each user's entire activity to the selection._filter_personal_chargebacktrims workspaces but keeps the whole user roster (documented at:390-395), so Users-section KPIs still count users who only existed in dropped workspaces.
Both are defensible approximations, but the header badge just says "Scoped to N selected workspace(s)" — a one-line caveat in the section hints would prevent misreading.
7. Smaller items
- flake8 not clean: 3 new E501s (
admin_growth_report.py:705, 727, 809) against.flake8'smax-line-length = 88. The PR body claims flake8 clean. - Dead JS/payload:
indexOfKeyandkeyInRangeare now unreferenced;accent/bandindrawLinesandmute/bandindrawBarsare unused after the overlay removal; every chart still emitswindow_start/window_end, which nothing draws. - Stale docs in code: the
admin_growth_report.pymodule docstring still describes "cross-platform use-case growth & rates (Opik + EM + MPM)", andadmin.py's subparserhelp=still says "cross-platform use-case growth report". READMEs were updated correctly. --units hourcost:_bucket_keyshas a 100 000-iteration guard that truncates silently, and each of the ~7 series builders is O(buckets × users). Measured: 2000 users × 1501 daily buckets = 0.21 s/series (fine); hourly over the same span is ~25× that per series plus a 36k-point SVG. A warning above some bucket count would be cheap insurance.migrate_users._RequestsClient.getdropsparams— harmless today (params={}), but a latent trap iffetch_chargeback_reportstarts using it;_ApiShim.configis likewise never read sincehostis always passed.- Nit: workspace leaderboard rows pass values through
_num, user rows don't (admin_growth_report.py:969,980) — inconsistent float rendering. report_monthis interpolated unquoted into the query string (utils.py). Pre-existing behavior, low risk, buturlencodewould be tidier.
Quality and coverage
Code quality is genuinely high: the split into a pure-functions module is the right shape, docstrings explain the approximations (proxy attributions, soft-delete-only churn, MPM absence) rather than hiding them, timezone handling is explicit (_dt_to_ms treating naive parse_time_key output as UTC is a nice catch), and _short_api_error scrubbing headers/cookies out of user-facing errors is a good security touch. parse_users/_extract_licensed_users degrade instead of crashing, and _assemble_report_data isolates each section behind its own try/except.
Coverage is strong on the derivation layer (26 tests in test_admin_growth_users.py, 44 in test_admin_growth_report.py, including the admin-key error path, scoping, and the service-account shapes). The gap is rendering: test_admin_growth_render.py has a single test, covering only lines. The new barsLine and groupedBarsH paths aren't exercised through build_html at all — worth adding two analogous tests, plus one asserting personal_vs_service/leaderboards sections reach the body.
Verdict
Sound direction and a clean methodology switch. Requesting changes for #1 (visibly self-contradictory chart), #2 and #4 (address or explicitly defer), and the flake8 lines. #3, #5, #6 are hint-text/labeling fixes that cost little and prevent misreading numbers that will be shown to customers. Squash-merge, as the PR body recommends, is right given the build-then-strip history.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…hten skip test Widen the degrade-never-crash guard: a successfully-fetched but malformed chargeback payload (e.g. a non-dict user entry) now warns and drops only the people section instead of crashing the whole report. Also fix test_include_users_false_skips_people to patch the actual call-site binding (cometx.cli.admin_growth_report.fetch_chargeback_report) so its call-count assertion is meaningful, and add a regression test proving the widened guard end-to-end via build().
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ic regex
Finding 1: _extract_service_account_names now returns None (not an empty
set) for an unrecognized response shape or a recognized-but-nonempty
container that yields zero extracted names, so classify_accounts falls
back to the labeled regex heuristic instead of misreporting source as
admin_api with a fabricated zero split. A genuinely-empty recognized
container (e.g. {"serviceAccounts": []}) still returns set(), since that
is a real zero-service-accounts answer.
Finding 2: _looks_like_service_account no longer runs an unanchored regex
over the concatenated username+email, which let "sa-"/"bot-" match
mid-string in real names like lisa-brown/abbot-jones. Prefix tokens
(svc-/sa-/bot-) are now anchored to a name-segment boundary in the
username; the sagemaker-integration.com domain token is matched against
the email's domain, anchored to the end of the host, so a lookalike
domain like sagemaker-integration.com.evil.com no longer matches.
Adds 6 regression tests across tests/unit/test_admin_growth_users.py and
tests/unit/test_admin_growth_report.py covering both findings.
…-personal) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…import Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ervice-account regex Three pre-merge fixes from the final whole-branch review of the people layer: hoist leaderboard_users above the leaderboards try block so a parse_users failure can no longer leave it unbound (previously masked by a misleading UnboundLocalError warning in the personal-vs-service except), drop the dead section-level hint from _build_personal_vs_service_section (render_section never reads it; the per-chart hint already carries the same source text), and tighten -service-account regex matching with a trailing boundary so jane-service-accountant is no longer mislabeled as a service account. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…READMEs Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…eaderboards - rename top section to "Organization overview (chargeback)"; move the SDK creation timelines to their per-product growth sections; keep an SDK-only fallback titled "Workspaces & projects (accessible)" when chargeback is absent - workspaces added-vs-deleted: new barsLine render primitive (added/deleted bars + overlaid growth-rate line); optional line + no top-right rate label - overview KPI: replace static "Total experiments" with "Workspace growth (window)"; add "User growth (window)" KPI to the Users section - leaderboards: remove the SDK-only "top workspaces by traces" board; label the user board "EM activity" and add per-board org-wide/SDK source hints - org-wide by default with a header scope line; exact org-wide experiment/ project leaderboards and by-workspace table sourced from chargeback - tests updated (182 passing) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…xtraction Rework the entry chain to build the report entirely from the admin chargeback report and hard-error (GrowthReportError, non-zero exit) when it is unavailable — there is no SDK fallback. Delete all SDK/platform-direct code: collectors (_collect_em/_opik/_mpm + helpers), the CreationEvent/UsageMetric data model, continuous_series/cumulative/growth_stats/unified_events/use_cases_by_workspace, the per-product growth/adoption sections, the SDK charts, and _unified_table. Drop the platforms/limit/include_users knobs from GrowthReporter/__init__ and generate_growth_report. Collapse _build_unified_section, _build_leaderboards_ section, _assemble_report_data, and _scope_label to their chargeback-only forms. Re-point --exclude-personal to filter the chargeback workspace list (_filter_personal_chargeback). Delete the SDK unit tests; keep/adjust the chargeback tests; add hard-error, trimmed-signature, and exclude-personal tests. The SDK/platform-direct methodology is preserved on the local growth-report-sdk-full branch. 144 unit tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Remove the drawStacked/drawArea JS chart primitives + their drawChart dispatch branches, the per-product (opik/em/mpm) rendering in build_html/collectCharts, the collector-status chips in render_topbar, and the now-unused .collectors / .product-heading CSS and PLATFORM_ORDER constant. The renderer now draws only bars/lines/barsLine/groupedBarsH and renders unified + people + leaderboards + personal_vs_service. Adds a test asserting no product/collector/stacked/area markup remains. 145 unit tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n-zero without admin key Remove the SDK-only CLI flags from the growth-report subparser, update the description/examples to state an admin API key is required (chargeback-only), drop the removed kwargs from the generate_growth_report call, and catch GrowthReportError to print the message and sys.exit(1). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The chargeback-only READMEs referenced a `growth-report-sdk-full` branch that exists only locally (not pushed), which is confusing for external readers. Remove the mention; the retired-SDK context isn't needed in shipped docs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Empty admin service-accounts set is authoritative: stop coercing set() -> None in _assemble_report_data, so a valid "zero service accounts" response keeps the admin_api source instead of falling back to the regex heuristic (Logical Bug, high). - Scope badge counts post-filter workspaces: _scope_label takes scoped_count (= len(parse_workspaces(scoped))) so the topbar matches the rendered sections after --exclude-personal / missing-workspace filtering (Logical Bug). - workspace_active_series accepts all_workspaces and seeds each bucket's total from it, so zero-member chargeback workspaces are counted and the chart total matches the Active-workspaces KPI (Logical Bug). - Fix "dayly": add _units_adverb() (hour->hourly/day->daily/week->weekly/ month->monthly) and use it in all chart hints (Naming/Typo). - Dedup: _chargeback_licensed_users delegates to the shared _extract_licensed_users; em/opik_user_breakdown_series share a single _user_breakdown_series builder. - WorkspaceRecord.members is now a tuple (immutable, matching frozen=True). - Add regression tests for the empty-admin-set, scope-count, zero-member-seeding, and units-adverb fixes. 149 unit tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Workflow review (verified): - em_score no longer crashes on a null count: _metric_value and the user leaderboard value_fn guard experiment_count/data_logged_mb with `or 0`, so a present-but-null chargeback value can't raise TypeError and silently blank the whole Users + Leaderboards sections. - top_users/bottom_users now exclude deleted and suspended accounts (via _rankable_users), matching the time-series/classify_accounts exclusions, so a deleted/suspended account is never listed as a current top user. - adoption_rate_series omits the em/opik series entirely when that capability has no signal (mirrors capability_series), instead of charting a misleading flat 0%; the chart builds its categories/legend from the present keys. Baz review: - _short_api_error redacts header/cookie/body/CSP tails on the parse-miss fallback, so a verbose SDK/HTTP exception can't leak into the user-facing GrowthReportError. - personal-vs-service heuristic hint now says "admin service-accounts API unavailable" instead of "admin API returned no service accounts", since heuristic source means the fetch failed (a genuine empty response is honored as admin_api). Adds regression tests for all five. 154 unit tests pass; flake8 clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Factor the new_in/before/pct window calculation into a single _window_growth() helper taking a collection of creation timestamps; _workspace_growth_kpi and the Users-section growth KPI now both call it, so a growth-window change lives in one place. Behavior unchanged (154 tests pass). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
9c9211e to
b680c8f
Compare
Baz dismissed its prior approval because a re-review found new findings.
…ack URL scheme Addresses PR #26 review findings: - Workspace-churn growth rate divided by a base that only accumulated additions, never subtracting deletions, so the surviving-total denominator drifted high and the rate read low after any deletion. Track a net surviving total (+added -deleted) and use its value at each bucket's start as the denominator. - fetch_chargeback_report() now requires an https:// base with a non-empty host, rejecting malformed/non-https --host/--source-url/override values before issuing the GET instead of forwarding them verbatim. - migrate_users._RequestsClient.get() adds the same scheme/host guard at the request boundary. Regression tests: churn rate uses net surviving base; chargeback fetch rejects non-https bases and host overrides. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- #1 workspace_active_series: report real per-bucket active totals instead of a flat total repeated across the series. - #2 barsLine/lines: add right-axis scale (rAxis) for the secondary rate series and wire hover tooltips (attachTip) with per-column labels; remove dead indexOfKey/keyInRange JS helpers. - #4 service accounts: drop the 'users' container key and match a service account by username OR email (mirror-failure safe). - #5 workspace-growth KPI: surface the membership-proxy basis in the KPI sub ("+N new (est. from earliest member)"). - #6 scoping: add a caveat that per-user totals stay org-wide when workspaces are scoped. - #7 lint/misc: fix Python E501s + stale docstrings; warn on growth series bucket truncation; pass reportMonth as a URL-encoded query param (and stop migrate_users' client dropping params); make _num rounding consistent; drop 7 dead window_start/window_end chart keys. - tests: render coverage for barsLine, groupedBarsH, and that the leaderboards / personal-vs-service sections reach the rendered body. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@dsblank thanks for the review — all findings are addressed in a1b5a8b. Mapping each to its fix: #1 — #2 — #3 — growth-rate denominator ignored deletions #4 — service-account detection #5 — workspace-growth KPI reads 0 #6 — scoped / exclude-personal silent scoping #7 — flake8 E501 + stale docstrings + misc
Coverage: added render tests for Verification: 173 unit tests pass, |
dsblank
left a comment
There was a problem hiding this comment.
Re-review
Verified state: all 7 earlier Baz threads are marked addressed and I confirmed the fixes are in the tree (shared _extract_licensed_users, _units_adverb, scoped_count badge, all_workspaces seeding, tuple members, _user_breakdown_series, empty-set service accounts, _window_growth, net surviving_total churn base). 173 passed on the full unit suite; flake8 delta vs main is neutral (long lines are pre-existing and live in the CSS/JS blobs).
The new findings below are all in the security-hardening commit (9648b25), which I don't think was re-reviewed after the SSRF threads closed.
1. HTTPS-only requirement breaks on-prem HTTP installs (regression) — cometx/utils.py:404, cometx/cli/migrate_users.py:141
fetch_chargeback_report now hard-rejects anything that isn't https://. This repo's own docs document on-prem servers reached over plain HTTP — MIGRATIONS.md:35 (--url-override=http://comet.X.com/clientlib) and README.md:781 (http://comet.a.com). Confirmed:
>>> fetch_chargeback_report(api) # url_override = http://comet.internal.corp/clientlib
ValueError: Chargeback server URL must be an https:// URL with a host; got 'http://comet.internal.corp/clientlib'.
This is not scoped to the new command: cometx admin chargeback-report and cometx migrate-users both route through the shared helper now, and both worked over HTTP before this PR. Those are existing users who lose a working command.
The SSRF concern that motivated this is real, but scheme-pinning is the wrong lever — it doesn't block https://169.254.169.254, and it does block legitimate internal HTTP. Suggest rejecting only clearly-malformed values (no scheme / no host), and if you want to keep discouraging HTTP, warn rather than raise — or gate it behind an explicit opt-in.
2. Path prefix on the base URL is silently dropped — cometx/utils.py:410
base = "%s://%s" % (parsed.scheme, parsed.netloc) # discards parsed.pathConfirmed: --host https://comet.x.com/prefix → GET https://comet.x.com/api/admin/chargeback/report. Before this PR, admin.py used --host verbatim and migrate_users used f"{server_url}/api/...", both preserving the prefix. Given the docs above reference /clientlib-style paths, deployments behind a path prefix will now silently hit the wrong URL.
Related: migrate_users._fetch_chargeback_report prints Fetching chargeback report from {url} using the old concatenation, so the printed URL can differ from the one actually requested — a confusing debug trail when this bites.
3. Every fetch failure is reported as "requires an admin API key" — cometx/cli/admin_growth_report.py:347
The except Exception around fetch_chargeback_report collapses DNS failures, timeouts, 500s, and now the ValueError from finding #1 into one message asserting the key isn't admin. With #1 in play, an on-prem user gets told to fix their API key when the actual problem is their URL scheme. Worth letting ValueError (config/URL problems) through with its own message, or at minimum not asserting a cause.
4. Hourly buckets scale poorly — cometx/cli/admin_growth_users.py:343
--units hour is a valid choice (admin.py:512) and _bucket_keys permits up to 100k buckets. Measured: 26,281 buckets × 200 users = 0.9s for one series, and eight series are built per report. At ~5k users over 3 years that's on the order of minutes; at the 100k-bucket guard, considerably worse. _iter_buckets rescans the whole user list per bucket, so it's O(buckets × users) eight times over.
Cheap mitigations: hoist the per-bucket rescan (sort users by created_at once and use a sweep pointer), or lower the guard and surface the existing warning before doing the work rather than after.
5. Smaller items
_fetch_service_accountsbypasses the new URL validation (admin_growth_report.py:176) — it builds the URL fromurl_overrideby hand with no scheme check, so the hardening in #1 is inconsistent across the two admin endpoints. Its blanketexcept Exceptionalso converts genuine bugs into a silent heuristic downgrade."csp"as a redaction marker (admin_growth_report.py:216) — a 3-character substring searched across the whole error text. A hostname or message containing those letters truncates the error, potentially to"unexpected error". Anchor it like the others (content-security-policyalready covers the real case).- Truthiness vs
is not None—churn_seriesusesif u.created_at:/if u.deleted_at:while_iter_bucketsand_window_growthuseis not None. Only differs at epoch 0, but the inconsistency is free to remove. - User leaderboard values aren't
_num-normalized (admin_growth_report.py:979,990) —emit_wswraps workspace values in_num, the user charts passvalue_fn(u)raw, soem_scorerenders with decimals where workspace rows render clean integers.
What's good
- The chargeback-only consolidation genuinely simplifies things;
admin_growth_report.pyreads much better than the multi-platform version it replaces. - Degradation discipline is consistent and well-documented — every series returns
None/[]rather than raising, and each section is independently try-wrapped. - Provenance labeling is honest throughout: proxy metrics are labeled as proxies in the chart hints,
admin_apivsheuristicis surfaced to the reader, and the service-account regexes carry the boundary-anchoring rationale inline. - Renderer escaping is sound —
_esceverywhere, and the<→\u003cpayload escape correctly prevents</script>breakout. - Test coverage is solid for the pure derivation layer (83 tests across the four new/changed files).
Recommendation
Requesting changes on #1 (and #2 alongside it) — those are silent regressions to two pre-existing commands on documented deployment topologies, and neither is covered by a test. #3 makes them hard to diagnose. Everything else is polish and could land as follow-ups.
Squash-merge is the right call given the build-then-strip history, as the description notes.
- admin_api_url: accept http+https on-prem bases (reject only malformed scheme/empty host) and preserve the base path prefix (e.g. /clientlib) instead of dropping it (#1, #2) - migrate_users: mirror the http+https check; print the actually-built request URL rather than a naively-concatenated one (#1, #2) - growth-report build(): surface a bad-URL ValueError with its own message instead of the generic "needs admin key" text (#3) - _iter_buckets: replace the per-bucket rescan with a created_at-sorted sweep pointer; behavior-preserving (#4) - _fetch_service_accounts: reuse the validated admin_api_url builder (#5a) - _short_api_error: drop the over-broad "csp" redaction marker (#5b) - churn_series: use `is not None` consistently for created/deleted (#5c) - leaderboard bar values: normalize via _lb_value/_num so fractional em_score renders as a clean int (#5d) Adds coverage for the URL layer (http, path prefix, malformed base/host), the sweep's mid-series deletion drop-out, and _num/_lb_value rounding. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for the re-review, @dsblank — all five addressed in e398819. #1 — on-prem #2 — path prefix dropped / printed URL ≠ requested URL. #3 — #4 — per-bucket rescan. Replaced with a
You flagged that #1/#2 weren't covered — added tests for the URL layer (http on-prem, path-prefix preservation, malformed base/host), plus the sweep's mid-series deletion drop-out (#4) and |
Baz dismissed its prior approval because a re-review found new findings.
Baz flagged that _fetch_chargeback_report now routes source_url through
admin_api_url(), which raises ValueError on a malformed base. source_url
can come from a new-style API key's embedded baseUrl (see
_resolve_server_url), which isn't validated up front -- so a bad baseUrl
made the CLI crash with an uncaught traceback where the pre-refactor
f"{server_url}/api/..." concatenation never raised.
Catch ValueError at the call site and surface it as a clean
"[ERROR] Invalid source server URL" + exit(1), matching the CLI's
existing error-handling style. Adds a regression test driving the real
admin_api_url path.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two regressions introduced by the previous round of review fixes.
A. `except ValueError` around `fetch_chargeback_report` was meant to catch
`admin_api_url`'s validation error, but `response.json()` raises
`json.JSONDecodeError` -- also a `ValueError` -- when the endpoint answers
2xx with a non-JSON body (an SSO/reverse-proxy HTML login page). That was
misreported as a malformed URL ("could not reach the chargeback endpoint:
Expecting value: line 1 column 1"). `admin_api_url` now raises
`InvalidServerURLError` (a `ValueError` subclass, so existing callers are
unaffected) and both handlers match on that type. growth-report routes a
non-JSON body back to the unavailable-endpoint message; migrate-users gets
its own handler naming the likely cause instead of a traceback.
B. The `_iter_buckets` sweep refactor was billed as behavior-preserving but
moved the series start: it filtered suspended users out before computing
`earliest_ms`, where the old code took the min over every dated user. Since
`churn_series` derives its own earliest from all users, the charts on the
same page stopped sharing an x-axis whenever the earliest account was
suspended (7 churn buckets vs 3 active buckets). The bucket range is now
spanned by all dated users again; suspended users remain excluded from
`existing`, so leading buckets read zero rather than being dropped.
Also drops an unused `call` import in test_migrate_users.py.
183 unit tests pass; black/isort clean; no new flake8 findings.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`_resolve_server_url` hard-rejected any non-https `--url`/`--source-url`, which contradicted the two layers below it: `admin_api_url` and `_RequestsClient.get` both accept http(s) with a host precisely because on-prem Comet servers are reached over plain http. It was also asymmetric -- an http base embedded in an API key's `baseUrl` (branch 2) passed through unchecked, so only the explicit flag was strict. Both flags now apply the same rule as those layers: accept http(s) with a host, reject only clearly-malformed values (non-http(s) scheme or empty host), still up front so a typo fails before any request is issued. The error message names what was wrong instead of just demanding https. Replaces test_explicit_url_non_https_exits (which locked in the old rule) with an on-prem http acceptance test plus a parametrized malformed-URL case. 186 unit tests pass; black/isort clean; no new flake8 findings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dsblank
left a comment
There was a problem hiding this comment.
Re-reviewed at 1864e4c. The five findings from my last pass all landed and are covered by tests; the URL-layer gaps I flagged as untested now have them.
That pass turned up three regressions introduced by the previous round of fixes, which I've fixed directly on this branch rather than sending back:
dddacc9— (A) theexcept ValueErrorguardingfetch_chargeback_reportalso caughtjson.JSONDecodeError, so a 2xx response with a non-JSON body (an SSO/reverse-proxy HTML login page) was misreported as a malformed URL.admin_api_urlnow raisesInvalidServerURLError, aValueErrorsubclass so existing callers are unaffected, and both handlers match on that type. (B) the_iter_bucketssweep refactor was billed as behavior-preserving but filtered suspended users out before computingearliest_ms, so these series stopped sharing an x-axis withchurn_serieswhenever the earliest account was suspended (7 churn buckets vs 3 active buckets). The bucket range spans all dated users again.f3c0db7— (C)_resolve_server_urlhard-rejected non-https--url/--source-url, contradictingadmin_api_urland_RequestsClient.get, which both accept on-prem http. It was also asymmetric: an http base embedded in an API key'sbaseUrlalready passed through unchecked. Both flags now apply the same rule.a6e866d— version bump to 3.6.9.
186 unit tests pass, changed files are black/isort clean, and flake8 counts match main exactly on every file touched.
Two non-blocking items left, neither worth holding the merge:
_fetch_service_accounts' switch to the prefix-preserving URL builder has no test — prefix preservation is only covered for the chargeback endpoint, so that URL could silently regress.- Now that on-prem http is accepted, pointing
--urlat an http host sends the destination API key in the clear. Inherent to supporting on-prem http, but a warning when the resolved scheme ishttpwould be cheap.
Squash-merging as recommended in the description, given the build-then-strip history. Noting for the record that I'm approving a branch that includes my own three commits.
Both findings from Baz's last pass. Neither is a regression from this PR --
the first predates the sweep refactor (every version has the same early
return) -- but both are cheap to close.
Workspaces chart silently vanished while its KPI showed numbers: when no user
carries a `created_at`, `_iter_buckets` has no timeline and yields nothing, so
`workspace_active_series` returned `[]` and the caller's `if ws_active_pts:`
gate dropped the chart -- while `workspace_active_stats` happily reported
`{total: 2, active: 1, active_pct: 50.0}` from membership alone. It now emits a
single "as of now" bucket computed the way the KPI is (membership + `is_active`,
ignoring `created_at`), so chart and KPI agree instead of one disappearing.
Same class of bug as the zero-member finding fixed in 89d6119, reached via the
undated-users path.
Duplicated URL validation: the http(s)-plus-host rule existed in three copies
(`admin_api_url`, `_RequestsClient.get`, `_resolve_server_url`) and could drift
into accepting different bases. All three now call one `validate_server_base`
in `cometx.utils`, which takes a `label` so each site keeps naming its own
offending input. A separate dependency-free module was considered and dropped:
`cometx/__init__.py` imports comet_ml transitively, so no `cometx.*` import can
avoid the SDK and the extra module bought nothing.
197 unit tests pass (11 new, including a test that all three call sites accept
and reject the same bases); black/isort clean; flake8 counts match main on
every file touched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three findings from Baz's pass on 08cbb26, all reproduced before fixing. Credential leak (two paths). Operators can point these commands at a base carrying credentials (https://user:pass@comet.internal). `admin_api_url` rebuilds from `parsed.netloc`, so userinfo survived into the constructed URL that migrate-users prints, and `InvalidServerURLError` echoed the raw input via `got %r` -- both put the password on screen and into logs. Added `redact_url_userinfo`, applied to every display path: the validation error, all four migrate-users URL prints (destination, source, fetch line, non-JSON error), and `_short_api_error`, since SDK/HTTP exceptions routinely quote the request URL. The regex is unanchored so a URL quoted mid-message is caught, and its userinfo class excludes /?# and whitespace so an `@` in a path or sentence isn't mistaken for credentials. The URL actually requested keeps its userinfo: stripping it would break a deployment relying on it for proxy/basic auth, and the credentials are bound for that host either way. Redaction is display-only by design. Fallback total diverged from the KPI: the "as of now" bucket added in 08cbb26 seeded `total` from the membership subset, so a workspace whose only members are suspended (absent from both the reverse-map and the seeded set, yet still counted in the KPI's all_workspaces denominator) was missed -- chart total 1 vs KPI total 2. The fallback now delegates to `workspace_active_stats` outright, so the two agree by construction rather than by parallel arithmetic. Baz's other two open threads need no code: it posted "addressed" replies on both (1864e4c, a6e866d) but leaves threads unresolved. 207 unit tests pass (10 new); black/isort clean; flake8 counts match main on every file touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
User description
Summary
Adds
cometx admin growth-report: an org-wide growth & adoption report rendered as a single self-contained HTML page, built entirely from the admin chargeback report.Requires an admin API key. The report is derived only from the admin chargeback endpoint; with a non-admin key the command prints a clear error and exits non-zero (no fallback).
Report sections (all chargeback-derived, org-wide by default)
Scope is org-wide by default; pass
WORKSPACEargs to scope down.--exclude-personal/--personal-patterndrop personal workspaces.Note on scope / history
Earlier iterations of this PR also included an SDK/platform-direct methodology (per-workspace Opik/EM/MPM collection). That approach was consolidated out so this command follows the chargeback methodology only; the later commits strip the SDK data-extraction and rendering. The net diff is the chargeback-only report — recommend squash-merge given the build-then-strip history.
Testing
🤖 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 fetch_chargeback_report_("fetch_chargeback_report"):::added GrowthReporter_("GrowthReporter"):::modified GrowthReporter_assemble_report_data_("GrowthReporter._assemble_report_data"):::added fetch_service_accounts_("_fetch_service_accounts"):::added CHARGEBACK_REPORT_API_("CHARGEBACK_REPORT_API"):::added SERVICE_ACCOUNTS_API_("SERVICE_ACCOUNTS_API"):::added admin_ -- "Routes growth-report subcommand to new chargeback-based generator." --> generate_growth_report_ admin_ -- "Uses fetch_chargeback_report to GET and save chargeback JSON." --> fetch_chargeback_report_ generate_growth_report_ -- "Instantiates GrowthReporter with active-window, leaderboards, and personal filters." --> GrowthReporter_ GrowthReporter_ -- "GrowthReporter.build now fetches chargeback, validating server base and URL." --> fetch_chargeback_report_ GrowthReporter_ -- "_assemble_report_data fetches service accounts for personal-vs-service charts." --> GrowthReporter_assemble_report_data_ GrowthReporter_assemble_report_data_ -- "_assemble_report_data fetches service accounts for personal-vs-service charts." --> fetch_service_accounts_ fetch_chargeback_report_ -- "fetch_chargeback_report GETs /api/admin/chargeback/report with Authorization, reportMonth." --> CHARGEBACK_REPORT_API_ CHARGEBACK_REPORT_API_ -- "Consumes returned chargeback JSON payload for downstream parsing." --> fetch_chargeback_report_ fetch_service_accounts_ -- "_fetch_service_accounts GETs /api/admin/service-accounts using Authorization headers." --> SERVICE_ACCOUNTS_API_ SERVICE_ACCOUNTS_API_ -- "Parses service-accounts JSON into name set; returns None on failures." --> fetch_service_accounts_ classDef added stroke:#15AA7A classDef removed stroke:#CD5270 classDef modified stroke:#EDAC4C linkStyle default stroke:#CBD5E1,font-size:13pxRework
cometx admin growth-reportinto a chargeback-only HTML report by teachingGrowthReporterto assemble organization, users, leaderboards, and personal-vs-service sections whileadmin_growth_renderdraws the new charts and layout. Centralize admin URL and chargeback fetching inutilsandmigrate_users, then wire the CLI, docs, and tests around the new admin-key-only flow.admin_growth_report,admin_growth_users, andadmin_growth_render, including org overview, user adoption/churn, leaderboards, and personal-vs-service account splits.Modified files (10)
Latest Contributors(2)
admin,migrate_users, andutilspreserve path prefixes, allow on-premhttp, and redact credentials in errors.Modified files (6)
Latest Contributors(2)