feat(billing): expose Desktop compute usage breakdown - #76889
Conversation
|
Hey @adboio! 👋 It looks like your git author email on this PR isn't your
You can fix it for this repo with: git config user.email "you@posthog.com"Or set it globally with |
d726dbb to
19a33a0
Compare
3b184fd to
810e182
Compare
🤖 CI report✅ Bundle size — no changeUncompressed size of every built Total: 65.67 MiB · no change No file changed by more than 1000 B. Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report ✅ Eager graph — within budgetHow much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy
🟢 Largest files eagerly shipped from
|
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 789 B | src/scenes/ChunkLoadErrorBoundary.tsx |
| 762 B | src/index.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 285.5 KiB | ../node_modules/.pnpm/posthog-js@1.410.1/node_modules/posthog-js/dist/rrweb.js |
| 267.7 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 238.4 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 231.5 KiB | ../node_modules/.pnpm/posthog-js@1.410.1/node_modules/posthog-js/dist/module.js |
| 154.3 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 105.0 KiB | src/lib/api.ts |
| 95.2 KiB | ../packages/quill/packages/quill/dist/index.js |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Toolbar bundle — eager 2.20 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.20 MiB · 17 files | no change | ████░░░░░░ 38.4% of 5.72 MiB |
| Deferred (lazy) | 2.08 MiB · 33 files | no change | n/a — loads on demand |
Loader dist/toolbar.js |
1.1 KiB | no change | █░░░░░░░░░ 5.8% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 723.0 KiB | dist/toolbar/toolbar-app-CLRN4BN3.css |
| 552.3 KiB | dist/toolbar/chunk-chunk-LINCPQWW.js |
| 484.6 KiB | dist/toolbar/chunk-chunk-5HONEO7J.js |
| 133.6 KiB | dist/toolbar/chunk-chunk-PSGMVXK7.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-T5KY5WYR.js |
| 71.0 KiB | dist/toolbar/toolbar-app-BIMPVI2A.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-27JL52RE.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-QQAXO5G5.js |
| 20.9 KiB | dist/toolbar/chunk-chunk-B5MOUYTE.js |
| 12.2 KiB | dist/toolbar/chunk-chunk-PIK3PADE.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — 🔺 +5.7 KiB (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1394.73 MiB · 🔺 +5.7 KiB (+0.0%)
ℹ️ MCP UI apps size — 32 app(s), 17074.3 KB JS
Built size of each MCP UI app (main.js + styles.css).
| App | JS | CSS |
|---|---|---|
| debug | 599.6 KB | 187.7 KB |
| action | 457.8 KB | 187.7 KB |
| action-list | 564.4 KB | 187.7 KB |
| cohort | 456.8 KB | 187.7 KB |
| cohort-list | 563.4 KB | 187.7 KB |
| email-template | 456.6 KB | 187.7 KB |
| error-details | 472.4 KB | 187.7 KB |
| error-issue | 457.5 KB | 187.7 KB |
| error-issue-list | 564.3 KB | 187.7 KB |
| experiment | 561.5 KB | 187.7 KB |
| experiment-list | 565.2 KB | 187.7 KB |
| experiment-results | 563.2 KB | 187.7 KB |
| feature-flag | 567.2 KB | 187.7 KB |
| feature-flag-list | 570.9 KB | 187.7 KB |
| feature-flag-testing | 461.0 KB | 187.7 KB |
| insight-actors | 562.2 KB | 187.7 KB |
| invite-email-preview | 456.0 KB | 187.7 KB |
| llm-costs | 559.5 KB | 187.7 KB |
| session-recording | 458.6 KB | 187.7 KB |
| session-summary | 463.9 KB | 187.7 KB |
| survey | 458.4 KB | 187.7 KB |
| survey-global-stats | 562.2 KB | 187.7 KB |
| survey-list | 565.1 KB | 187.7 KB |
| survey-stats | 562.2 KB | 187.7 KB |
| trace-span | 457.2 KB | 187.7 KB |
| trace-span-list | 564.3 KB | 187.7 KB |
| workflow | 457.1 KB | 187.7 KB |
| workflow-list | 563.7 KB | 187.7 KB |
| loops-review | 461.4 KB | 187.7 KB |
| query-results | 749.3 KB | 187.7 KB |
| render-ui | 830.0 KB | 187.7 KB |
| visual-review-snapshots | 461.6 KB | 187.7 KB |
✅ Django migration risk — no migrations to analyze
No Django migrations need risk analysis.
🦔 ReviewHog reviewed this pull requestFound 2 must fix, 2 should fix, 2 consider. Published 6 findings (view the review). |
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
There was a problem hiding this comment.
ReviewHog Report
Api
Issues: 2 issues
Files (7)
products/tasks/backend/logic/services/sandbox_pricing.pyproducts/tasks/backend/facade/compute_rates.pyproducts/tasks/backend/presentation/views/compute_rate_cards.pyproducts/tasks/backend/routes.pyproducts/tasks/frontend/generated/api.schemas.tsproducts/tasks/frontend/generated/api.tsservices/mcp/src/api/generated.ts
What were the main changes
- Adds get_compute_rate_cards_for_period to canonical sandbox_pricing service, selecting all rate versions overlapping a billing period
- Adds a Tasks facade (compute_rates.py) that validates/serializes canonical published compute rates, returning invalid_configuration on bad config
- Adds new sandbox_compute_rate_cards DRF ViewSet and registers it under /api/projects/{id}/sandbox_compute_rate_cards/
- Regenerates OpenAPI-derived Tasks frontend and MCP client types for the new endpoint
Business logic
Issues: 4 issues
Files (4)
services/llm-gateway/src/llm_gateway/services/quota_resolver.pyservices/llm-gateway/src/llm_gateway/services/compute_rate_resolver.pyservices/llm-gateway/src/llm_gateway/api/usage.pyservices/llm-gateway/src/llm_gateway/main.py
What were the main changes
- Extends QuotaResourceStatus with posthog_code_usage breakdown (token/compute credits, CPU/memory quantities, microUSD subtotals) derived from limited component resources
- Adds independent ComputeRateResolver with its own quota-cache-style freshness window that fetches published rates from the new Tasks endpoint; failures fail open and never affect combined/token usage
- Wires ComputeRateResolver into app.state in main.py alongside QuotaResolver
- Assembles combined ai_credits.breakdown (component integers, USD decimal strings, rate_cards, rate_card_error) on GET /v1/usage/posthog_code without altering existing ai_credits fields
| components["token_used_usd"] = str(Decimal(token_credits) / 100) if token_credits is not None else None | ||
| components["compute_used_usd"] = str(Decimal(compute_credits) / 100) if compute_credits is not None else None |
There was a problem hiding this comment.
USD decimal-string breakdown fields have inconsistent decimal precision
Why we think it's a valid issue
- Checked: The flagged code isn't in the on-disk file (working tree sits at the base commit); pulled the real PR diff. The
token_used_usd/compute_used_usdassembly lives in_posthog_code_usage()inservices/llm-gateway/src/llm_gateway/services/quota_resolver.py(diff hunk at the new linescomponents["token_used_usd"] = str(Decimal(token_credits) / 100) ...). Traced the sibling float path:_credits_to_usdat quota_resolver.py:83-85 usesround(value * _USD_PER_CREDIT, 2), so the codebase's own convention for these dollar amounts is 2 decimals. - Found: Ran the exact expression for representative credit values —
str(Decimal(c)/100)yields0→"0",5→"0.05",100→"1",150→"1.5",67→"0.67",1234→"12.34". Decimal division keeps only the digits the exact quotient needs, so trailing/whole-dollar values lose the cents. The PR's own tests bake this in:test_quota_resolver.pyassertstoken_used_usd == "0"andcompute_used_usd == "0"for zero credits, whiletest_usage.pyasserts"12.34"/"0.67"elsewhere — inconsistent widths across the same field. - Impact: This is a brand-new contract field the PR explicitly ships as a decimal-string "display value" (the reason to add it alongside the existing float
used_usdis a ready-to-render exact amount). A Desktop client rendering it directly shows$0,$1,$1.5instead of$0.00,$1.00,$1.50. Real credit values (0, whole-dollar multiples of 100, single-cent amounts) hit this on common inputs, not edge cases. Since credits are integer cents, quantizing to 2 places is always exact and lossless, matching the sibling convention right next door. Concrete trigger + concrete consequence, directly on the changed lines — clears the bar as a correctness/contract defect rather than style; keeping the reviewer's should_fix.
Issue description
token_used_usd and compute_used_usd are built with str(Decimal(token_credits) / 100) / str(Decimal(compute_credits) / 100), with no rounding/quantization applied. Decimal division only keeps as many fractional digits as the exact quotient needs, so the emitted string's precision varies by value: 0 credits -> "0", 100 credits -> "1", 150 credits -> "1.5", 5 credits -> "0.05", 12345 credits -> "123.45" (verified: str(Decimal(100)/100) == "1", str(Decimal(150)/100) == "1.5"). The whole point of shipping a decimal string here (versus the existing float used_usd/limit_usd) is presumably to give clients an exact, ready-to-render dollar amount, but a client that renders it directly will show "$0", "$1", "$1.5" instead of a consistent "$0.00", "$1.00", "$1.50".
Suggested fix
Quantize to 2 decimal places before stringifying, e.g. str((Decimal(token_credits) / 100).quantize(Decimal("0.01"))), so the format is consistent regardless of value — matching the 2-decimal convention _credits_to_usd already uses for the sibling used_usd/limit_usd fields (round(value * _USD_PER_CREDIT, 2)).
Prompt to fix with AI (copy-paste)
## Context
@services/llm-gateway/src/llm_gateway/services/quota_resolver.py#L119-120
<issue_description>
`token_used_usd` and `compute_used_usd` are built with `str(Decimal(token_credits) / 100)` / `str(Decimal(compute_credits) / 100)`, with no rounding/quantization applied. Decimal division only keeps as many fractional digits as the exact quotient needs, so the emitted string's precision varies by value: 0 credits -> "0", 100 credits -> "1", 150 credits -> "1.5", 5 credits -> "0.05", 12345 credits -> "123.45" (verified: `str(Decimal(100)/100)` == "1", `str(Decimal(150)/100)` == "1.5"). The whole point of shipping a decimal *string* here (versus the existing float `used_usd`/`limit_usd`) is presumably to give clients an exact, ready-to-render dollar amount, but a client that renders it directly will show "$0", "$1", "$1.5" instead of a consistent "$0.00", "$1.00", "$1.50".
</issue_description>
<issue_validation>
- **Checked:** The flagged code isn't in the on-disk file (working tree sits at the base commit); pulled the real PR diff. The `token_used_usd`/`compute_used_usd` assembly lives in `_posthog_code_usage()` in `services/llm-gateway/src/llm_gateway/services/quota_resolver.py` (diff hunk at the new lines `components["token_used_usd"] = str(Decimal(token_credits) / 100) ...`). Traced the sibling float path: `_credits_to_usd` at quota_resolver.py:83-85 uses `round(value * _USD_PER_CREDIT, 2)`, so the codebase's own convention for these dollar amounts is 2 decimals.
- **Found:** Ran the exact expression for representative credit values — `str(Decimal(c)/100)` yields `0→"0"`, `5→"0.05"`, `100→"1"`, `150→"1.5"`, `67→"0.67"`, `1234→"12.34"`. Decimal division keeps only the digits the exact quotient needs, so trailing/whole-dollar values lose the cents. The PR's own tests bake this in: `test_quota_resolver.py` asserts `token_used_usd == "0"` and `compute_used_usd == "0"` for zero credits, while `test_usage.py` asserts `"12.34"`/`"0.67"` elsewhere — inconsistent widths across the same field.
- **Impact:** This is a brand-new contract field the PR explicitly ships as a decimal-string "display value" (the reason to add it alongside the existing float `used_usd` is a ready-to-render exact amount). A Desktop client rendering it directly shows `$0`, `$1`, `$1.5` instead of `$0.00`, `$1.00`, `$1.50`. Real credit values (0, whole-dollar multiples of 100, single-cent amounts) hit this on common inputs, not edge cases. Since credits are integer cents, quantizing to 2 places is always exact and lossless, matching the sibling convention right next door. Concrete trigger + concrete consequence, directly on the changed lines — clears the bar as a correctness/contract defect rather than style; keeping the reviewer's should_fix.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Quantize to 2 decimal places before stringifying, e.g. `str((Decimal(token_credits) / 100).quantize(Decimal("0.01")))`, so the format is consistent regardless of value — matching the 2-decimal convention `_credits_to_usd` already uses for the sibling `used_usd`/`limit_usd` fields (`round(value * _USD_PER_CREDIT, 2)`).
</potential_solution>
There was a problem hiding this comment.
Removed the component USD string fields in the simplification pass. The response now exposes integer credits as the authoritative component amounts, so this formatting inconsistency no longer exists.
| def _optional_integer(value: object) -> int | None: | ||
| if isinstance(value, bool) or not isinstance(value, int): | ||
| return None | ||
| return value |
There was a problem hiding this comment.
_optional_integer can silently collapse a real (non-int) usage value into 'unknown'
Why we think it's a valid issue
- Checked: The upstream serializer for the
quota_limitsresponse the gateway reads (ee/api/quota_limits.py, same on the PR base and head branches), plus how_posthog_code_usageconsumesresource.get("usage")and the file's own sibling number helper. - Found:
QuotaResourceLimitSerializer.usage = serializers.FloatField(allow_null=True)(ee/api/quota_limits.py:30), andlimited = DictField(child=QuotaResourceLimitSerializer())(:61-62) — so every entry the gateway reads out ofdata["limited"]is serialized throughFloatField. DRF'sFloatField.to_representationreturnsfloat(value); I verified the round-trip:json.dumps(float(1234))→1234.0on the wire →resp.json()parses it back as Pythonfloat→isinstance(x, int)isFalse. So_optional_integer(quota_resolver.py:99-101,not isinstance(value, int) → None) drops it. - Found: This isn't limited to the fractional compute fields the reviewer speculated about — it hits all component fields, including
token_credits/compute_credits, because they pass through the sameFloatField. That means_posthog_code_usagegetsNonefor every component andif all(value is None ...): return None(quota_resolver.py:114) collapses the entire breakdown toNone. - Found: The same file already proves floats are the norm from this endpoint: the existing
used_usd/limit_usdpath reads the identicalresource.get("usage")via_optional_number(:88-89), which deliberately acceptsintorfloat. The new_optional_integerbreaks that established float-tolerance. - Found: No test catches this —
test_quota_resolver.pymocks the HTTP response with native Python ints ("usage": 1234), never exercising the real DRFFloatField; the PR notes the Tasks/integration path couldn't be run locally. - Impact: In production the
ai_credits.breakdownwould silently populate asNone/"never synced" for real, synced usage — the exact ambiguity the PR's stated contract forbids (a reported value, including zero, must stay visible). Concrete trigger (upstream FloatField serialization, confirmed) + concrete consequence (whole breakdown dropped), so it clears the bar. - Priority: Bumped should_fix → must_fix: the reviewer framed it as a speculative "if it's ever a float" affecting only computed compute quantities, but the response serializer guarantees floats for all component
usagevalues, so this renders the entire new breakdown non-functional rather than occasionally dropping one field.
Issue description
_optional_integer returns None for anything that isn't a plain Python int (bools excluded), and _posthog_code_usage (line 110) uses it to read cpu_millicore_seconds, memory_mib_seconds, cpu_cost_microusd, and memory_cost_microusd straight from resource.get("usage"). Those are all computed quantities — CPU/memory integrated over time, and cost = rate × quantity — which are plausible candidates for the upstream API to serialize as JSON floats (e.g. 12.5) rather than whole integers. If it ever does, a real, synced usage number is silently turned into None, which the breakdown then reports as 'never synced' — exactly the ambiguity the PR's own stated contract says must be avoided (missing stays unknown, but a reported value, including zero, must remain visible). There's also no logging when this conversion drops a non-null value, so a contract mismatch here would be invisible in production.
Suggested fix
Confirm (and ideally document at the _POSTHOG_CODE_COMPONENT_RESOURCES definition) that Django's quota_limits response guarantees whole-integer JSON for every one of these fields. If that's not guaranteed, widen _optional_integer to accept whole-number floats (e.g. int(value) when isinstance(value, float) and value.is_integer()) rather than dropping them to None.
Prompt to fix with AI (copy-paste)
## Context
@services/llm-gateway/src/llm_gateway/services/quota_resolver.py#L99-102
@services/llm-gateway/src/llm_gateway/services/quota_resolver.py#L110
<issue_description>
`_optional_integer` returns `None` for anything that isn't a plain Python `int` (bools excluded), and `_posthog_code_usage` (line 110) uses it to read `cpu_millicore_seconds`, `memory_mib_seconds`, `cpu_cost_microusd`, and `memory_cost_microusd` straight from `resource.get("usage")`. Those are all computed quantities — CPU/memory integrated over time, and cost = rate × quantity — which are plausible candidates for the upstream API to serialize as JSON floats (e.g. `12.5`) rather than whole integers. If it ever does, a real, synced usage number is silently turned into `None`, which the breakdown then reports as 'never synced' — exactly the ambiguity the PR's own stated contract says must be avoided (missing stays unknown, but a *reported* value, including zero, must remain visible). There's also no logging when this conversion drops a non-null value, so a contract mismatch here would be invisible in production.
</issue_description>
<issue_validation>
- **Checked:** The upstream serializer for the `quota_limits` response the gateway reads (`ee/api/quota_limits.py`, same on the PR base and head branches), plus how `_posthog_code_usage` consumes `resource.get("usage")` and the file's own sibling number helper.
- **Found:** `QuotaResourceLimitSerializer.usage = serializers.FloatField(allow_null=True)` (`ee/api/quota_limits.py:30`), and `limited = DictField(child=QuotaResourceLimitSerializer())` (`:61-62`) — so *every* entry the gateway reads out of `data["limited"]` is serialized through `FloatField`. DRF's `FloatField.to_representation` returns `float(value)`; I verified the round-trip: `json.dumps(float(1234))` → `1234.0` on the wire → `resp.json()` parses it back as Python `float` → `isinstance(x, int)` is `False`. So `_optional_integer` (`quota_resolver.py:99-101`, `not isinstance(value, int) → None`) drops it.
- **Found:** This isn't limited to the fractional compute fields the reviewer speculated about — it hits *all* component fields, including `token_credits`/`compute_credits`, because they pass through the same `FloatField`. That means `_posthog_code_usage` gets `None` for every component and `if all(value is None ...): return None` (`quota_resolver.py:114`) collapses the entire breakdown to `None`.
- **Found:** The same file already proves floats are the norm from this endpoint: the existing `used_usd`/`limit_usd` path reads the identical `resource.get("usage")` via `_optional_number` (`:88-89`), which deliberately accepts `int` *or* `float`. The new `_optional_integer` breaks that established float-tolerance.
- **Found:** No test catches this — `test_quota_resolver.py` mocks the HTTP response with native Python ints (`"usage": 1234`), never exercising the real DRF `FloatField`; the PR notes the Tasks/integration path couldn't be run locally.
- **Impact:** In production the `ai_credits.breakdown` would silently populate as `None`/"never synced" for real, synced usage — the exact ambiguity the PR's stated contract forbids (a reported value, including zero, must stay visible). Concrete trigger (upstream FloatField serialization, confirmed) + concrete consequence (whole breakdown dropped), so it clears the bar.
- **Priority:** Bumped should_fix → must_fix: the reviewer framed it as a speculative "if it's ever a float" affecting only computed compute quantities, but the response serializer guarantees floats for all component `usage` values, so this renders the entire new breakdown non-functional rather than occasionally dropping one field.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Confirm (and ideally document at the `_POSTHOG_CODE_COMPONENT_RESOURCES` definition) that Django's `quota_limits` response guarantees whole-integer JSON for every one of these fields. If that's not guaranteed, widen `_optional_integer` to accept whole-number floats (e.g. `int(value)` when `isinstance(value, float) and value.is_integer()`) rather than dropping them to `None`.
</potential_solution>
There was a problem hiding this comment.
Fixed. The existing DRF serializer emits integral usage values as JSON floats, so the gateway now accepts finite whole-number floats and converts them back to integers. The real wire-shape test now uses 33.0/24.0/0.0 and verifies the breakdown remains populated.
| async def get_rates(self, team_id: int, auth_header: str) -> ComputeRateStatus: | ||
| cached = await self._get_cached(team_id) | ||
| if cached is not None: | ||
| return cached | ||
| status = await self._fetch(team_id, auth_header) | ||
| await self._set_cached(team_id, status) | ||
| return status | ||
|
|
||
| async def _fetch(self, team_id: int, auth_header: str) -> ComputeRateStatus: | ||
| base_url = get_settings().posthog_api_base_url | ||
| if not base_url: | ||
| return ComputeRateStatus() | ||
| response = await self._http.get( | ||
| f"{base_url.rstrip('/')}/api/projects/{team_id}/sandbox_compute_rate_cards/", | ||
| headers={"Authorization": auth_header}, | ||
| timeout=2.0, | ||
| ) | ||
| response.raise_for_status() | ||
| return _parse_status(response.json()) |
There was a problem hiding this comment.
ComputeRateResolver never caches failures, unlike its sibling QuotaResolver, enabling request-path amplification against a struggling/disabled upstream
Why we think it's a valid issue
- Checked:
compute_rate_resolver.pyfull flow (get_rates/_fetch/_set_cached), the siblingQuotaResolver.get_resource_statuserror handling, the caller gating inusage.py, and the actualsandbox_compute_rate_cardsendpoint (ComputeRateCardsViewSet). - Found (core is real):
get_rates(compute_rate_resolver.py:50-56) calls_fetchwith no try/except, and_fetchends inresponse.raise_for_status()(:67), so any 5xx/network/timeout propagates past_set_cached; the only catch isresolve_compute_rates(:37-41) which returns a fail-openComputeRateStatus()but never writes the cache. The siblingQuotaResolverdeliberately caches its transient fail-open for_FAIL_OPEN_CACHE_TTL_SECONDS(quota_resolver.py:185-196). So during a transient outage/slowness of the rate endpoint, the gateway re-hits it on every poll with no backoff, where the sibling would hit once per 60s — a genuine, directly-related reliability inconsistency on a frequently-polled Desktop path. - Found (severity is overstated): The reviewer's headline "any team without the product gets a 404 forever, uncached, on every call" does not hold. (1) The fetch is gated:
usage.py:107-108only callsresolve_compute_rateswhenproduct == POSTHOG_CODE_PRODUCT and breakdown is not None, so only teams with synced posthog_code component usage ever trigger it. (2)ComputeRateCardsViewSet.listhas no product-feature gate — it returns 200 withrate_cards(or an empty list /invalid_configuration) for anyproject:readcaller, so there is no permanent-404 state. (3) Success andinvalid_configurationresponses are parsed and cached by_parse_status/_set_cached; only raised exceptions escape caching. (4) The sibling itself intentionally does NOT cache 4xx (quota_resolver.py:177-184, to avoid poisoning the shared per-team cache with caller-credential-specific failures), so the reviewer's suggestion to cache a 404 would be inconsistent with that pattern — the real gap is only the transient-error (5xx/timeout) caching. - Impact: Confirmed reliability gap: during a transient outage of a lightweight config endpoint, or a rollout window where Django lacks the new route, the resolver amplifies load for active teams instead of backing off like its sibling. It fails open safely (rate cards simply absent), the blast radius is bounded to active-breakdown teams, and the endpoint is cheap — so it clears the bar as a robustness fix but not as a blocking one.
- Priority: Lowered must_fix → should_fix: the must_fix rationale rested on permanent steady-state amplification (404-forever, every team), which is inaccurate; what remains is bounded, safe-failing, transient-only amplification worth fixing to match the sibling's fail-open caching.
Issue description
In get_rates, status = await self._fetch(team_id, auth_header) is not wrapped in try/except, so any exception raised inside _fetch (network error, or response.raise_for_status() firing on any 4xx/5xx) propagates straight out of get_rates before _set_cached runs. The only catch site is the broad except Exception in resolve_compute_rates (lines 37-41), which fails open per-request but never writes to the cache. Compare this with the sibling QuotaResolver added in the same PR (quota_resolver.py), whose docstring explicitly caches a fail-open result for _FAIL_OPEN_CACHE_TTL_SECONDS "so a struggling Django isn't hit on every subsequent request." Because _fetch calls response.raise_for_status() unconditionally with no special-casing (unlike PlanResolver._fetch_plan, which explicitly handles 404), this isn't just an outage edge case: any team without the Tasks/sandbox-compute product enabled will get a 404 from sandbox_compute_rate_cards on literally every call to GET /v1/usage/posthog_code forever, since that failure is never cached. And during any real slowdown of that endpoint, every concurrent request pays the full round trip (up to the 2s timeout) with no backoff, amplifying load on the very endpoint that's already struggling — a self-inflicted thundering herd on a hot, frequently-polled path (Desktop's usage/quota check).
Suggested fix
Wrap the fetch in try/except inside get_rates (mirroring QuotaResolver.get_resource_status), and on failure cache a short-TTL empty ComputeRateStatus() (e.g. a _FAIL_OPEN_CACHE_TTL_SECONDS-style constant) instead of letting every subsequent request retry the call. Also consider special-casing a 404 response (team without sandbox compute enabled) the way PlanResolver._fetch_plan does for the seats endpoint, so a permanently-disabled feature doesn't retry indefinitely.
Prompt to fix with AI (copy-paste)
## Context
@services/llm-gateway/src/llm_gateway/services/compute_rate_resolver.py#L50-68
<issue_description>
In `get_rates`, `status = await self._fetch(team_id, auth_header)` is not wrapped in try/except, so any exception raised inside `_fetch` (network error, or `response.raise_for_status()` firing on any 4xx/5xx) propagates straight out of `get_rates` before `_set_cached` runs. The only catch site is the broad `except Exception` in `resolve_compute_rates` (lines 37-41), which fails open per-request but never writes to the cache. Compare this with the sibling `QuotaResolver` added in the same PR (`quota_resolver.py`), whose docstring explicitly caches a fail-open result for `_FAIL_OPEN_CACHE_TTL_SECONDS` "so a struggling Django isn't hit on every subsequent request." Because `_fetch` calls `response.raise_for_status()` unconditionally with no special-casing (unlike `PlanResolver._fetch_plan`, which explicitly handles 404), this isn't just an outage edge case: any team without the Tasks/sandbox-compute product enabled will get a 404 from `sandbox_compute_rate_cards` on literally every call to `GET /v1/usage/posthog_code` forever, since that failure is never cached. And during any real slowdown of that endpoint, every concurrent request pays the full round trip (up to the 2s timeout) with no backoff, amplifying load on the very endpoint that's already struggling — a self-inflicted thundering herd on a hot, frequently-polled path (Desktop's usage/quota check).
</issue_description>
<issue_validation>
- **Checked:** `compute_rate_resolver.py` full flow (`get_rates`/`_fetch`/`_set_cached`), the sibling `QuotaResolver.get_resource_status` error handling, the caller gating in `usage.py`, and the actual `sandbox_compute_rate_cards` endpoint (`ComputeRateCardsViewSet`).
- **Found (core is real):** `get_rates` (`compute_rate_resolver.py:50-56`) calls `_fetch` with no try/except, and `_fetch` ends in `response.raise_for_status()` (`:67`), so any 5xx/network/timeout propagates past `_set_cached`; the only catch is `resolve_compute_rates` (`:37-41`) which returns a fail-open `ComputeRateStatus()` but never writes the cache. The sibling `QuotaResolver` deliberately caches its transient fail-open for `_FAIL_OPEN_CACHE_TTL_SECONDS` (`quota_resolver.py:185-196`). So during a transient outage/slowness of the rate endpoint, the gateway re-hits it on every poll with no backoff, where the sibling would hit once per 60s — a genuine, directly-related reliability inconsistency on a frequently-polled Desktop path.
- **Found (severity is overstated):** The reviewer's headline "any team without the product gets a 404 forever, uncached, on every call" does not hold. (1) The fetch is gated: `usage.py:107-108` only calls `resolve_compute_rates` when `product == POSTHOG_CODE_PRODUCT and breakdown is not None`, so only teams with synced posthog_code component usage ever trigger it. (2) `ComputeRateCardsViewSet.list` has no product-feature gate — it returns 200 with `rate_cards` (or an empty list / `invalid_configuration`) for any `project:read` caller, so there is no permanent-404 state. (3) Success and `invalid_configuration` responses are parsed and cached by `_parse_status`/`_set_cached`; only raised exceptions escape caching. (4) The sibling itself intentionally does NOT cache 4xx (`quota_resolver.py:177-184`, to avoid poisoning the shared per-team cache with caller-credential-specific failures), so the reviewer's suggestion to cache a 404 would be inconsistent with that pattern — the real gap is only the transient-error (5xx/timeout) caching.
- **Impact:** Confirmed reliability gap: during a transient outage of a lightweight config endpoint, or a rollout window where Django lacks the new route, the resolver amplifies load for active teams instead of backing off like its sibling. It fails open safely (rate cards simply absent), the blast radius is bounded to active-breakdown teams, and the endpoint is cheap — so it clears the bar as a robustness fix but not as a blocking one.
- **Priority:** Lowered must_fix → should_fix: the must_fix rationale rested on permanent steady-state amplification (404-forever, every team), which is inaccurate; what remains is bounded, safe-failing, transient-only amplification worth fixing to match the sibling's fail-open caching.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Wrap the fetch in try/except inside `get_rates` (mirroring `QuotaResolver.get_resource_status`), and on failure cache a short-TTL empty `ComputeRateStatus()` (e.g. a `_FAIL_OPEN_CACHE_TTL_SECONDS`-style constant) instead of letting every subsequent request retry the call. Also consider special-casing a 404 response (team without sandbox compute enabled) the way `PlanResolver._fetch_plan` does for the seats endpoint, so a permanently-disabled feature doesn't retry indefinitely.
</potential_solution>
There was a problem hiding this comment.
Removed the compute-rate resolver and its cache entirely. Published-rate delivery is deferred to the later UI/explanation work, keeping this PR on the existing quota transport path.
| @extend_schema(tags=["tasks"]) | ||
| class ComputeRateCardsViewSet(TeamAndOrgViewSetMixin, viewsets.ViewSet): | ||
| scope_object = "project" | ||
| required_scopes = ["project:read"] | ||
| http_method_names = ["get", "head", "options"] | ||
|
|
||
| @extend_schema( | ||
| summary="Get published cloud-compute rates", | ||
| responses={200: ComputeRateCardsResponseSerializer}, | ||
| ) | ||
| def list(self, request: Request, *args: Any, **kwargs: Any) -> Response: | ||
| rates = compute_rates.get_published_compute_rates((self.team.organization.usage or {}).get("period")) | ||
| if rates.error: | ||
| logger.error("posthog_code_compute_rate_card_invalid") | ||
| return Response(ComputeRateCardsResponseSerializer({"rate_cards": rates.rate_cards, "error": rates.error}).data) |
There was a problem hiding this comment.
New list action will be schema'd and code-gen'd as an array, but it returns a single envelope object
Why we think it's a valid issue
- Checked: the view's
listmethod, the two regenerated frontend artifacts, and the sibling module cited as precedent. - Found (server returns one object):
compute_rate_cards.py:66returnsResponse(ComputeRateCardsResponseSerializer({"rate_cards": ..., "error": ...}).data)— serializer instantiated withoutmany=True, so the HTTP body is a single envelope{rate_cards, error}. - Found (generated client types it as an array):
products/tasks/frontend/generated/api.ts:392-393generatessandboxComputeRateCardsList(...): Promise<ComputeRateCardsResponseApi[]>andapiMutator<ComputeRateCardsResponseApi[]>(...), whileapi.schemas.ts:713definesComputeRateCardsResponseApias a single-objectinterface. So the operation return type isEnvelope[]for an endpoint that actually returns oneEnvelope— a concrete mismatch, exactly drf-spectacular's_is_list_view()array-wrapping on alistaction (view registered as the only route,routes.py:44). - Found (established fix in the same directory):
channels_api.py:210-246already defines_ActivityPageEnvelopeSchema(_is_list_view -> False)with a docstring naming this exact heuristic bug and assigns it viaschema = ...;ComputeRateCardsViewSetomits any such override. - Impact: contract break — a TS consumer trusting the generated type and calling
.map()/.length/[0]on the response gets a runtime TypeError or silentundefined, since the real body is a bare object, not an array. This is the endpoint's entire purpose (generated-client consumption), the wrong contract is already committed in this PR's artifacts, and the fix is trivial and precedented. Meets the 'contract break / broken callers' keep bar.
Issue description
ComputeRateCardsViewSet.list() returns Response(ComputeRateCardsResponseSerializer({...}).data) — a single JSON object {rate_cards, error} (the serializer is instantiated without many=True). drf-spectacular's AutoSchema._is_list_view() heuristic treats any DRF list action as returning a collection and wraps the declared responses={200: ComputeRateCardsResponseSerializer} schema in an array for that operation, regardless of the fact the view body only ever returns one object. This is exactly the bug already fixed elsewhere in this same codebase — including in the sibling module of this very file, products/tasks/backend/presentation/views/channels_api.py (_ActivityPageEnvelopeSchema, comment: "Stops drf-spectacular's list-view heuristic from wrapping the list response in an array... list returns a single page envelope... not a bare collection"), plus posthog/api/event_filter_config.py (SingletonSchema) and products/access_control/backend/presentation/views.py. ComputeRateCardsViewSet doesn't apply this override. The consequence is verified in this same PR chunk's own regenerated clients: products/tasks/frontend/generated/api.schemas.ts correctly defines ComputeRateCardsResponseApi as a single-object interface, but products/tasks/frontend/generated/api.ts generates sandboxComputeRateCardsList(): Promise<ComputeRateCardsResponseApi[]> and apiMutator<ComputeRateCardsResponseApi[]>(...) — an array type for an endpoint that actually returns a bare object. Any TypeScript consumer (Desktop frontend or generated MCP client code) written against this generated type and calling .map(), .length, or [0] on the response will crash or silently read undefined at runtime, because the real HTTP body is {"rate_cards": [...], "error": ...}, not [{...}].
Suggested fix
Add the same _is_list_view override used elsewhere in this product and assign it as the view's schema, e.g.:
class _ComputeRateCardsEnvelopeSchema(AutoSchema):
def _is_list_view(self, serializer: Any = None) -> bool:
return False
class ComputeRateCardsViewSet(TeamAndOrgViewSetMixin, viewsets.ViewSet):
schema = _ComputeRateCardsEnvelopeSchema()
...Since this ViewSet only exposes a list route (no retrieve), the simpler SingletonSchema pattern from event_filter_config.py applies directly — no operationId collision to guard against (per products/review_hog/DECISIONS.md's note that singleton-only viewsets never hit that collision). Then rerun hogli build:openapi and regenerate the MCP client so ComputeRateCardsResponseApi/ComputeRateCardsResponse are referenced as single objects, not arrays, in api.ts and services/mcp/src/api/generated.ts.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/backend/presentation/views/compute_rate_cards.py#L52-66
<issue_description>
`ComputeRateCardsViewSet.list()` returns `Response(ComputeRateCardsResponseSerializer({...}).data)` — a single JSON object `{rate_cards, error}` (the serializer is instantiated without `many=True`). drf-spectacular's `AutoSchema._is_list_view()` heuristic treats any DRF `list` action as returning a collection and wraps the declared `responses={200: ComputeRateCardsResponseSerializer}` schema in an array for that operation, regardless of the fact the view body only ever returns one object. This is exactly the bug already fixed elsewhere in this same codebase — including in the sibling module of this very file, `products/tasks/backend/presentation/views/channels_api.py` (`_ActivityPageEnvelopeSchema`, comment: "Stops drf-spectacular's list-view heuristic from wrapping the `list` response in an array... `list` returns a single page envelope... not a bare collection"), plus `posthog/api/event_filter_config.py` (`SingletonSchema`) and `products/access_control/backend/presentation/views.py`. `ComputeRateCardsViewSet` doesn't apply this override. The consequence is verified in this same PR chunk's own regenerated clients: `products/tasks/frontend/generated/api.schemas.ts` correctly defines `ComputeRateCardsResponseApi` as a single-object interface, but `products/tasks/frontend/generated/api.ts` generates `sandboxComputeRateCardsList(): Promise<ComputeRateCardsResponseApi[]>` and `apiMutator<ComputeRateCardsResponseApi[]>(...)` — an array type for an endpoint that actually returns a bare object. Any TypeScript consumer (Desktop frontend or generated MCP client code) written against this generated type and calling `.map()`, `.length`, or `[0]` on the response will crash or silently read `undefined` at runtime, because the real HTTP body is `{"rate_cards": [...], "error": ...}`, not `[{...}]`.
</issue_description>
<issue_validation>
- **Checked:** the view's `list` method, the two regenerated frontend artifacts, and the sibling module cited as precedent.
- **Found (server returns one object):** `compute_rate_cards.py:66` returns `Response(ComputeRateCardsResponseSerializer({"rate_cards": ..., "error": ...}).data)` — serializer instantiated without `many=True`, so the HTTP body is a single envelope `{rate_cards, error}`.
- **Found (generated client types it as an array):** `products/tasks/frontend/generated/api.ts:392-393` generates `sandboxComputeRateCardsList(...): Promise<ComputeRateCardsResponseApi[]>` and `apiMutator<ComputeRateCardsResponseApi[]>(...)`, while `api.schemas.ts:713` defines `ComputeRateCardsResponseApi` as a single-object `interface`. So the operation return type is `Envelope[]` for an endpoint that actually returns one `Envelope` — a concrete mismatch, exactly drf-spectacular's `_is_list_view()` array-wrapping on a `list` action (view registered as the only route, `routes.py:44`).
- **Found (established fix in the same directory):** `channels_api.py:210-246` already defines `_ActivityPageEnvelopeSchema(_is_list_view -> False)` with a docstring naming this exact heuristic bug and assigns it via `schema = ...`; `ComputeRateCardsViewSet` omits any such override.
- **Impact:** contract break — a TS consumer trusting the generated type and calling `.map()`/`.length`/`[0]` on the response gets a runtime TypeError or silent `undefined`, since the real body is a bare object, not an array. This is the endpoint's entire purpose (generated-client consumption), the wrong contract is already committed in this PR's artifacts, and the fix is trivial and precedented. Meets the 'contract break / broken callers' keep bar.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Add the same `_is_list_view` override used elsewhere in this product and assign it as the view's schema, e.g.:
```python
class _ComputeRateCardsEnvelopeSchema(AutoSchema):
def _is_list_view(self, serializer: Any = None) -> bool:
return False
class ComputeRateCardsViewSet(TeamAndOrgViewSetMixin, viewsets.ViewSet):
schema = _ComputeRateCardsEnvelopeSchema()
...
Since this ViewSet only exposes a list route (no retrieve), the simpler SingletonSchema pattern from event_filter_config.py applies directly — no operationId collision to guard against (per products/review_hog/DECISIONS.md's note that singleton-only viewsets never hit that collision). Then rerun hogli build:openapi and regenerate the MCP client so ComputeRateCardsResponseApi/ComputeRateCardsResponse are referenced as single objects, not arrays, in api.ts and services/mcp/src/api/generated.ts.
</potential_solution>
</details>
There was a problem hiding this comment.
Removed the new rate-card endpoint and its generated clients, so the list-action schema mismatch no longer applies.
| def _billing_period(period: object) -> tuple[datetime | None, datetime | None]: | ||
| if not isinstance(period, list) or len(period) != 2: | ||
| return None, None | ||
| try: | ||
| return dateutil.parser.isoparse(period[0]), dateutil.parser.isoparse(period[1]) | ||
| except (TypeError, ValueError): | ||
| return None, None |
There was a problem hiding this comment.
Missing/malformed billing period silently returns every rate card instead of the period-scoped set the contract promises
Why we think it's a valid issue
- Checked:
get_compute_rate_cards_for_periodinsandbox_pricing.py, theCOMPUTE_RATE_CARDSconfig value,_billing_period's fall-through, the view's call site, andOrganization.current_billing_period. - Found (behavior is real):
sandbox_pricing.pyget_compute_rate_cards_for_periodrunsif start is None or end is None: return cards— returns the full validated set unfiltered._billing_periodreturns(None, None)on missing/malformedusage["period"], and the view (compute_rate_cards.py:63) feeds the rawusage.get("period")straight in, so a missing/unsynced period does silently yield every configured card whileerrorstaysnull, contradicting the serializer help_text ('rate cards overlapping the organization's synchronized billing period'). - Impact (why minor, not should_fix): The consequence is low-severity and largely latent.
COMPUTE_RATE_CARDS: tuple[...] = ()today, soif not rate_cards: return ()fires first and the endpoint returns an empty list regardless of period — the widening is fully dormant until multiple rate-card versions are configured. Even then it requires the compound case (period missing AND ≥2 versions), and the returned cards are all valid, timestamp-labeled published rates — an over-inclusive informational list, not wrong prices, a crash, data loss, or a security/tenant issue (the PR states these rate cards are informational; microUSD subtotals are authoritative). Thestart/end is None → return allpath is also a deliberate branch, so this is a debatable product fallback, not an accidental defect. Thecurrent_billing_period'reimplementation drift' angle is a DRY/code-quality nit, not a bug that bites. - Priority: Down-ranked to
consider— the contract-vs-implementation mismatch is genuine and reproducible (so not dismissed), but its impact is low and currently dormant, which does not justifyshould_fix.
Issue description
_billing_period() (lines 50-56) returns (None, None) whenever organization.usage["period"] is missing, isn't a 2-element list, or fails dateutil.parser.isoparse. That result flows straight into get_compute_rate_cards_for_period(start, end, rate_cards) in products/tasks/backend/logic/services/sandbox_pricing.py (L86-98), which explicitly treats start is None or end is None as "return every validated rate card, unfiltered" (L94-95) rather than empty or erroring. But ComputeRateCardsResponseSerializer.rate_cards's own help_text (compute_rate_cards.py) documents the contract as "rate cards overlapping the organization's synchronized billing period" — so whenever billing-period data is absent or malformed, the endpoint silently widens its response to the entire configured rate-card history (every past and future version) instead of the documented period-scoped subset, which is a real behavior/contract mismatch an API consumer has no way to detect (no error is surfaced; error stays null). This is also a parallel, subtly different reimplementation of the already-existing Organization.current_billing_period property (posthog/models/organization.py:548-566), which parses the same usage["period"] value with a looser len(period) < 2 check (vs. this file's strict != 2) and its own try/except — two independent implementations of "parse the org's billing period" can drift out of sync with each other over time.
Suggested fix
Don't fall through to "no filter" when the period can't be resolved — either return () for rate_cards in that case, or add a distinct error value (e.g. unavailable) so callers can tell "we don't know the period yet" apart from "pricing is inactive". Better still, avoid re-deriving the period from raw organization.usage["period"] altogether: have the view pass self.team.organization.current_billing_period (already implemented, tested, and used elsewhere) into get_published_compute_rates instead of the raw JSON dict, so both the generic quota API and this new endpoint always agree on what "the current billing period" means.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/backend/facade/compute_rates.py#L50-56
@products/tasks/backend/facade/compute_rates.py#L29-34
<issue_description>
`_billing_period()` (lines 50-56) returns `(None, None)` whenever `organization.usage["period"]` is missing, isn't a 2-element list, or fails `dateutil.parser.isoparse`. That result flows straight into `get_compute_rate_cards_for_period(start, end, rate_cards)` in `products/tasks/backend/logic/services/sandbox_pricing.py` (L86-98), which explicitly treats `start is None or end is None` as "return every validated rate card, unfiltered" (L94-95) rather than empty or erroring. But `ComputeRateCardsResponseSerializer.rate_cards`'s own help_text (compute_rate_cards.py) documents the contract as "rate cards overlapping the organization's synchronized billing period" — so whenever billing-period data is absent or malformed, the endpoint silently widens its response to the entire configured rate-card history (every past and future version) instead of the documented period-scoped subset, which is a real behavior/contract mismatch an API consumer has no way to detect (no error is surfaced; `error` stays `null`). This is also a parallel, subtly different reimplementation of the already-existing `Organization.current_billing_period` property (`posthog/models/organization.py:548-566`), which parses the same `usage["period"]` value with a looser `len(period) < 2` check (vs. this file's strict `!= 2`) and its own try/except — two independent implementations of "parse the org's billing period" can drift out of sync with each other over time.
</issue_description>
<issue_validation>
- **Checked:** `get_compute_rate_cards_for_period` in `sandbox_pricing.py`, the `COMPUTE_RATE_CARDS` config value, `_billing_period`'s fall-through, the view's call site, and `Organization.current_billing_period`.
- **Found (behavior is real):** `sandbox_pricing.py` `get_compute_rate_cards_for_period` runs `if start is None or end is None: return cards` — returns the full validated set unfiltered. `_billing_period` returns `(None, None)` on missing/malformed `usage["period"]`, and the view (`compute_rate_cards.py:63`) feeds the raw `usage.get("period")` straight in, so a missing/unsynced period does silently yield every configured card while `error` stays `null`, contradicting the serializer help_text ('rate cards overlapping the organization's synchronized billing period').
- **Impact (why minor, not should_fix):** The consequence is low-severity and largely latent. `COMPUTE_RATE_CARDS: tuple[...] = ()` today, so `if not rate_cards: return ()` fires first and the endpoint returns an empty list regardless of period — the widening is fully dormant until multiple rate-card versions are configured. Even then it requires the compound case (period missing AND ≥2 versions), and the returned cards are all valid, timestamp-labeled published rates — an over-inclusive informational list, not wrong prices, a crash, data loss, or a security/tenant issue (the PR states these rate cards are informational; microUSD subtotals are authoritative). The `start/end is None → return all` path is also a deliberate branch, so this is a debatable product fallback, not an accidental defect. The `current_billing_period` 'reimplementation drift' angle is a DRY/code-quality nit, not a bug that bites.
- **Priority:** Down-ranked to `consider` — the contract-vs-implementation mismatch is genuine and reproducible (so not dismissed), but its impact is low and currently dormant, which does not justify `should_fix`.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Don't fall through to "no filter" when the period can't be resolved — either return `()` for `rate_cards` in that case, or add a distinct error value (e.g. `unavailable`) so callers can tell "we don't know the period yet" apart from "pricing is inactive". Better still, avoid re-deriving the period from raw `organization.usage["period"]` altogether: have the view pass `self.team.organization.current_billing_period` (already implemented, tested, and used elsewhere) into `get_published_compute_rates` instead of the raw JSON dict, so both the generic quota API and this new endpoint always agree on what "the current billing period" means.
</potential_solution>
There was a problem hiding this comment.
There is one more malformed-period case that does not reach the (None, None) fallback. isoparse accepts an offset-free value such as 2026-08-01T00:00:00 and returns a naive datetime. The rate-card lookup later compares it with timezone-aware boundaries, raises TypeError, and the endpoint returns a 500.
Could we reject offset-free periods, and periods where the end is before the start, before looking up rate cards? An endpoint test for those responses would cover it.
There was a problem hiding this comment.
Removed the period-scoped rate-card endpoint and facade in the simplification pass. This PR no longer parses billing periods or returns rate cards.
There was a problem hiding this comment.
Good catch. I removed the rate-card endpoint and its billing-period parser while narrowing this PR to component usage transport, so malformed or naive periods are no longer handled here. Published-rate delivery will be designed separately with the UI/explanation work.
| breakdown = quota_status.posthog_code_usage | ||
| if product == POSTHOG_CODE_PRODUCT and breakdown is not None: | ||
| compute_rates = await resolve_compute_rates(request, user.team_id) | ||
| breakdown = { | ||
| **breakdown, | ||
| "rate_cards": compute_rates.rate_cards, | ||
| "rate_card_error": compute_rates.error, | ||
| } |
There was a problem hiding this comment.
Compute-rate lookup is fully serialized instead of overlapped with plan/quota resolution, adding latency to every usage check
Why we think it's a valid issue
- Checked: The call ordering in
usage.py(resolve_plan_and_quotaat:81,resolve_compute_ratesat:108), the gating condition, the cited overlap precedent independencies.py, andComputeRateResolver's caching. - Found (claim is real):
resolve_compute_ratesis awaited strictly afterresolve_plan_and_quotareturns (usage.py:81→:108), so on a cache miss it is a third sequential Django round-trip. The overlap precedent is genuine:resolve_plan_and_quota(dependencies.py:181-196) documents "Both calls are independent Django roundtrips on cache miss, so ... we overlap them" and usesasyncio.gather. The rate fetch keys only onteam_id, known before that call. - Found (impact is narrower than stated): It is not added to "every" call. The fetch is gated behind
product == POSTHOG_CODE_PRODUCT and breakdown is not None(usage.py:107), so it is skipped entirely for teams without synced posthog_code component usage; andget_ratesserves from Redis on cache hit (compute_rate_resolver.py:51-53), so the full ~2s round-trip only hits on cache miss for breakdown-present teams. The await is properly async (non-blocking on the event loop), and the upstream is a lightweight config read — so this is bounded tail latency, not an at-scale N+1/quadratic/blocking cliff. - Found (the fix carries a real tradeoff): Overlapping requires firing
resolve_compute_ratesspeculatively beforequota_statusreveals whetherbreakdown is not None. For the (likely large) fraction of posthog_code polls where breakdown is absent, that converts today's zero rate-fetches into one speculative upstream call per poll on a hot, frequently-polled path — extra load the current gating deliberately avoids, and which compounds the sibling reliability gap (3-2-1: this resolver never caches failures). So the change is a latency-vs-upstream-load trade, not a clean win. - Priority: Lowered should_fix → consider: the observation is correct and grounded in an in-repo pattern, but the benefit is conditional (cache-miss + breakdown-present only) and bounded, while the suggested overlap adds speculative hot-path calls for breakdown-absent teams — a debatable trade the author may reasonably decline, so it belongs on record at lowest priority rather than as a recommended fix.
Issue description
resolve_compute_rates(request, user.team_id) is only awaited after resolve_plan_and_quota has fully completed, adding a third, strictly sequential upstream HTTP round trip (up to a 2s timeout on cache miss) to the tail latency of every GET /v1/usage/posthog_code call. This codebase already has an established pattern for exactly this situation: resolve_plan_and_quota (dependencies.py) explicitly documents "Both calls are independent Django roundtrips on cache miss, so ... we overlap them" and uses asyncio.gather to run the plan and quota fetches concurrently. The new compute-rate fetch only depends on team_id and the product name — both known before resolve_plan_and_quota is even called — so it doesn't need to wait for quota_status to resolve before starting; only the decision to splice its result into breakdown depends on breakdown is not None.
Suggested fix
Kick off resolve_compute_rates(request, user.team_id) concurrently with resolve_plan_and_quota (e.g. via asyncio.gather) when product == POSTHOG_CODE_PRODUCT, and only merge the result into breakdown once quota_status comes back and breakdown is not None. This removes a full serialized round trip from a hot, frequently-polled endpoint, consistent with the concurrency pattern already used one call up the stack.
Prompt to fix with AI (copy-paste)
## Context
@services/llm-gateway/src/llm_gateway/api/usage.py#L106-113
<issue_description>
`resolve_compute_rates(request, user.team_id)` is only awaited after `resolve_plan_and_quota` has fully completed, adding a third, strictly sequential upstream HTTP round trip (up to a 2s timeout on cache miss) to the tail latency of every `GET /v1/usage/posthog_code` call. This codebase already has an established pattern for exactly this situation: `resolve_plan_and_quota` (dependencies.py) explicitly documents "Both calls are independent Django roundtrips on cache miss, so ... we overlap them" and uses `asyncio.gather` to run the plan and quota fetches concurrently. The new compute-rate fetch only depends on `team_id` and the product name — both known before `resolve_plan_and_quota` is even called — so it doesn't need to wait for `quota_status` to resolve before starting; only the decision to splice its result into `breakdown` depends on `breakdown is not None`.
</issue_description>
<issue_validation>
- **Checked:** The call ordering in `usage.py` (`resolve_plan_and_quota` at `:81`, `resolve_compute_rates` at `:108`), the gating condition, the cited overlap precedent in `dependencies.py`, and `ComputeRateResolver`'s caching.
- **Found (claim is real):** `resolve_compute_rates` is awaited strictly after `resolve_plan_and_quota` returns (`usage.py:81` → `:108`), so on a cache miss it is a third sequential Django round-trip. The overlap precedent is genuine: `resolve_plan_and_quota` (`dependencies.py:181-196`) documents "Both calls are independent Django roundtrips on cache miss, so ... we overlap them" and uses `asyncio.gather`. The rate fetch keys only on `team_id`, known before that call.
- **Found (impact is narrower than stated):** It is not added to "every" call. The fetch is gated behind `product == POSTHOG_CODE_PRODUCT and breakdown is not None` (`usage.py:107`), so it is skipped entirely for teams without synced posthog_code component usage; and `get_rates` serves from Redis on cache hit (`compute_rate_resolver.py:51-53`), so the full ~2s round-trip only hits on cache miss for breakdown-present teams. The await is properly async (non-blocking on the event loop), and the upstream is a lightweight config read — so this is bounded tail latency, not an at-scale N+1/quadratic/blocking cliff.
- **Found (the fix carries a real tradeoff):** Overlapping requires firing `resolve_compute_rates` speculatively *before* `quota_status` reveals whether `breakdown is not None`. For the (likely large) fraction of posthog_code polls where breakdown is absent, that converts today's zero rate-fetches into one speculative upstream call per poll on a hot, frequently-polled path — extra load the current gating deliberately avoids, and which compounds the sibling reliability gap (3-2-1: this resolver never caches failures). So the change is a latency-vs-upstream-load trade, not a clean win.
- **Priority:** Lowered should_fix → consider: the observation is correct and grounded in an in-repo pattern, but the benefit is conditional (cache-miss + breakdown-present only) and bounded, while the suggested overlap adds speculative hot-path calls for breakdown-absent teams — a debatable trade the author may reasonably decline, so it belongs on record at lowest priority rather than as a recommended fix.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Kick off `resolve_compute_rates(request, user.team_id)` concurrently with `resolve_plan_and_quota` (e.g. via `asyncio.gather`) when `product == POSTHOG_CODE_PRODUCT`, and only merge the result into `breakdown` once `quota_status` comes back and `breakdown is not None`. This removes a full serialized round trip from a hot, frequently-polled endpoint, consistent with the concurrency pattern already used one call up the stack.
</potential_solution>
There was a problem hiding this comment.
Removed compute-rate lookup from the request path entirely. The endpoint now performs only the existing plan/quota resolution and returns the component breakdown already carried by that response.
5682f7b to
138442a
Compare
138442a to
9ad8f38
Compare
9ad8f38 to
21761de
Compare
187dadb to
d1f2dbd
Compare
|
😎 Stack merged successfully - details. |
78fedcb to
ebf5db9
Compare
PostHog QA Frontend ReportPASS · 3/3 · 8m · commit
Coverage
Setup
PostHog QA Frontend Report |
|
Reviews (1): Last reviewed commit: "fix(billing): type Desktop usage fixture..." | Re-trigger Greptile |
PR overviewThis pull request adds a Desktop compute usage breakdown to billing and updates gateway quota-resolution behavior that supports usage reporting. One issue has been addressed, but an authenticated caller can still vary whitespace in the Bearer header to bypass the five-minute quota cache. This can repeatedly invoke the Django quota endpoint and increase Redis key cardinality, creating a limited resource-exhaustion risk. Open issues (1)
Fixed/addressed: 1 · PR risk: 5/10 |
| limited["sandbox_compute_memory_mib_seconds"], {"limited": False, "usage": None, "limit": None} | ||
| ) | ||
| for field in ("posthog_code_token_credits", "sandbox_compute_credits", "sandbox_compute_cpu_millicore_seconds"): | ||
| self.assertIs(type(limited[field]["usage"]), int, field) |
There was a problem hiding this comment.
usage is a DRF FloatField, so the endpoint returns whole numbers as 33.0, not 33. This int check will fail. Can we remove the loop at the end of the test?
There was a problem hiding this comment.
yep good catch, fixed
Add current-period token and cloud-compute components to the organization quota response and carry them through the LLM gateway usage API. Serialize applicable canonical compute rate cards without duplicating pricing constants. Generated-By: PostHog Code Task-Id: 94a09572-3a3a-4435-ae95-12fe3067ec15
Build the Desktop display breakdown in the gateway from the existing synchronized quota resources. Keep Django focused on generic resource usage and canonical published rate-card serialization. Generated-By: PostHog Code Task-Id: 94a09572-3a3a-4435-ae95-12fe3067ec15
Restore the generic quota API unchanged and publish canonical cloud-compute rates from a product-owned Tasks endpoint. Resolve and cache those rates independently in the LLM gateway usage path. Generated-By: PostHog Code Task-Id: 94a09572-3a3a-4435-ae95-12fe3067ec15
Route published compute-rate access through the Tasks facade, tighten gateway test types, and commit generated OpenAPI clients for the new endpoint. Generated-By: PostHog Code Task-Id: 94a09572-3a3a-4435-ae95-12fe3067ec15
Generated-By: PostHog Code Task-Id: 0a766035-227e-447a-995d-5cd6bcae7875
The gateway's Desktop breakdown parser reads the informational component resources out of this response, but the endpoint only emitted QuotaResource values, so the breakdown was always null. Components are merged in as JSON integers (the parser discards floats) with null-when-unknown preserved. Generated-By: PostHog Code Task-Id: d3cdac58-b431-4fbf-b16d-3b68aa48aa02
Remove the premature compute-rate endpoint and gateway resolver. Keep this change focused on transporting synchronized token, compute, CPU, and memory usage, while accepting integral JSON floats from the existing quota serializer. Generated-By: PostHog Code Task-Id: 90c0a633-a8f7-4ff2-b06a-870d5e755109
Generated-By: PostHog Code Task-Id: 90c0a633-a8f7-4ff2-b06a-870d5e755109
Generated-By: PostHog Code Task-Id: 90c0a633-a8f7-4ff2-b06a-870d5e755109
Generated-By: PostHog Code Task-Id: 90c0a633-a8f7-4ff2-b06a-870d5e755109
Prevent a caller from reading another credential's cached quota and Desktop usage breakdown. Cache keys contain only a SHA-256 credential fingerprint; the team-scoped last-known billing fallback remains unchanged. Generated-By: PostHog Code Task-Id: 90c0a633-a8f7-4ff2-b06a-870d5e755109
5af6a99 to
7f5f5b1
Compare
| def _redis_key(resource_key: str, team_id: int) -> str: | ||
| return f"quota:{resource_key}:team:{team_id}" | ||
| def _redis_key(resource_key: str, team_id: int, auth_header: str) -> str: | ||
| credential_fingerprint = hashlib.sha256(auth_header.encode()).hexdigest() |
There was a problem hiding this comment.
Low: Quota cache bypass through header variations
Both gateway and Django authentication accept multiple spaces after Bearer, but hashing the raw header gives each equivalent spelling a different cache key. An authenticated caller can vary that whitespace to bypass the five-minute cache on every request, repeatedly hit the Django quota endpoint, and grow Redis key cardinality; fingerprint the normalized credential instead.
| credential_fingerprint = hashlib.sha256(auth_header.encode()).hexdigest() | |
| credential_fingerprint = hashlib.sha256(auth_header.split(None, 1)[-1].encode()).hexdigest() |


Problem
PostHog Desktop needs the current billing-period token and cloud-compute breakdown behind its existing combined credit spend and limit.
Why: The combined credit counter remains authoritative for billing and quota enforcement, while component credits and resource quantities let Desktop explain that usage without creating separate limits or products.
Changes
limited.posthog_code_creditsas the authoritative combined usage, limit, and exhaustion state.Published compute rates are deliberately deferred to the UI/explanation work. This PR does not add a second endpoint, resolver, cache, product, limit, or Stripe metric.
How did you test this code?
Automatic notifications
Docs update
No user-facing docs change. The optional response contract is documented in the API schema and tests.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Codex narrowed the change to the existing quota and usage APIs after reviewing the original rate-card plumbing as premature for this step.
Created with PostHog Code