Repository navigation
Tags
Tags
feat(admin): chargeback-only growth-report (org overview, users, lead… …erboards, personal-vs-service) (#26) * refactor(admin): shared fetch_chargeback_report helper in utils * feat(growth-report): collect Opik TRACE_COUNT alongside SPAN_COUNT * feat(growth-report): add multi-series lines chart primitive Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(growth-report): add admin_growth_users parse + adoption/leaderboard derivations * feat(growth-report): people section (active/adoption) + CLI args + render wiring * fix(growth-report): degrade people layer on malformed chargeback; tighten 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(). * feat(growth-report): people active/adoption/capability over-time series Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(growth-report): leaderboards section (workspace + user, top/bottom N) * feat(growth-report): personal-vs-service-account split via admin API (regex fallback) * fix(growth-report): honest service-account degrade + anchored heuristic 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. * feat(growth-report): optional personal-workspace exclusion (--exclude-personal) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * style(growth-report): apply black/isort formatting; drop unused test import Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(growth-report): hoist leaderboard_users; drop dead hint; bound -service-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> * docs(growth-report): document people-layer CLI flags and sections in READMEs Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(growth-report): org-overview section, growth KPIs, cleaner leaderboards - 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> * refactor(growth-report): chargeback-only report.py; remove SDK data extraction 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> * refactor(growth-report): strip SDK rendering from render.py 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> * refactor(growth-report): drop --platforms/--limit/--no-users; exit non-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> * docs(growth-report): chargeback-only methodology; admin key required Rewrite the growth-report sections of README.md and README-ADMIN.md for the chargeback-only report: admin API key required (errors out otherwise), drop the removed --platforms/--limit/--no-users flags and the SDK/per-product framing, document the chargeback sections (org overview, users, leaderboards, personal- vs-service), and note the SDK methodology is preserved on the local growth-report-sdk-full branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(growth-report): chargeback-shape the _sample_report_data render fixture Replace the legacy SDK-shaped fixture (products/collectors/stackedBars/area, per-product growth+adoption sections) with a chargeback-only payload: the four current sections (Organization overview, Users, Leaderboards, Personal-vs- service) and only bars/lines/barsLine/groupedBarsH charts. Update the two render-test assertions that referenced the old "Workspaces & projects" title and drop the stray legacy "products" key from the missing-sections test. 145 unit tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(growth-report): drop reference to the local SDK-preservation branch 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> * fix(growth-report): address PR review findings - 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> * fix(growth-report): correctness fixes from code review (workflow + Baz) 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> * refactor(growth-report): share window-growth math between KPIs 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> * fix(growth-report): correct churn growth-rate base + validate chargeback 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> * fix(growth-report): implement PR #26 review findings (dsblank) - #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> * fix(growth-report): address dsblank re-review (URL builder + sweep) - 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> * fix(migrate-users): catch malformed baseUrl instead of crashing 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> * fix(growth-report): narrow URL error handling; keep series axes aligned 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> * fix(migrate-users): accept on-prem http bases via --url/--source-url `_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> * Version 3.6.9: chargeback-only growth-report Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(growth-report): chart-vs-KPI mismatch; one shared server-URL rule 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> * fix(admin): redact URL credentials; fallback total must follow the KPI 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> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Douglas Blank <doug@comet.com>
PreviousNext