Skip to content

Commit 44a3cc8

Browse files
christian-byrneConnor Byrne
andauthored
fix(telemetry): emit billing.operation.succeeded for all operation types and forward billing events to the desktop host sink (#14310)
## Summary Two coupled billing-telemetry correctness fixes. Both make billing dashboards under-report silently rather than fail loudly, which is why they ship together. Plan doc: [Billing telemetry program dossier](https://app.notion.com/p/3ad6d73d365081fea125e284b6ac9f13) ## Changes - **What**: 1. `billingOperationStore.handleSuccess` now emits `billing.operation.succeeded` for **all three** operation types (`subscription`, `topup`, `cancel`). It previously only emitted it for `cancel`, while `billing.operation.failed` and `billing.operation.timeout` are emitted for all three. Any `succeeded / (succeeded + failed)` ratio grouped by `operation_type` therefore read a **permanent 0%** for `subscription` and `topup` — a metric that can never be non-zero, so an anomaly monitor on it pages immediately and forever. 2. `HostTelemetrySink` now implements `trackBillingEvent`. It did not, and `initHostTelemetry` *replaces* the provider registry rather than extending it, so a cloud build running inside the desktop shell emitted **zero** canonical `billing.*` events to any provider — an entire user segment missing from every billing funnel. - **Breaking**: no. Both changes are additive at the event level. No event was renamed, removed, or had a property removed. ### Event contract after this change `billing.operation.succeeded` is now emitted once per successful billing operation, for every operation type, with the property set below. This matches `billing.operation.failed` / `billing.operation.timeout` exactly, so numerator and denominator of a success-rate ratio are groupable by the same tags: | Property | Type | Presence | | --- | --- | --- | | `operation` | `'operation'` | always | | `stage` | `'succeeded'` | always | | `outcome` | `'success'` | always | | `billing_op_id` | string | always | | `operation_type` | `'subscription' \| 'topup' \| 'cancel'` | always | | `tier` | tier key or `'team'` | when the caller supplied it | | `cycle` | `'monthly' \| 'yearly'` | when the caller supplied it | | `checkout_type` | `'new' \| 'change'` | when the caller supplied it | | `payment_intent_source` | attribution enum | when the caller supplied it | Notes for anyone querying this: - A success-rate monitor should now be `succeeded / (succeeded + failed)` over `billing.operation.*`, grouped by `operation_type`, and it will be non-zero for all three types. - `.timeout` is a separate stage from `.failed`; a ratio that should treat timeouts as failures must include both in the denominator. - The flow-specific success events `billing.subscription_checkout.succeeded` and `billing.topup.succeeded` are unchanged and still fire. A `subscription` success therefore emits both `billing.operation.succeeded` and `billing.subscription_checkout.succeeded`. These are distinct event names — do not sum them into one success count. - The four optional grouping keys are omitted from the payload when the caller passed no metadata (the workspace top-up path currently starts its poller without metadata), so group-bys on them must tolerate a missing tag rather than assume it is present. ## Review Focus **How to review this, and what it is part of.** This is one step in a broader effort to make the billing/subscription funnel observable end to end. The two fixes here are both "the dashboard lies" bugs rather than user-visible bugs, so the thing to check is the *event contract*, not UI behavior. Suggested reading order: 1. `billingOperationStore.ts` — the whole fix is one moved emit. The new `billing.operation.succeeded` call is unconditional at the top of `handleSuccess`'s telemetry block; the old `else` branch that emitted it only for `cancel` is deleted. Confirm the `cancel` payload is unchanged in practice (`cancel` operations are started without metadata, so the four new optional keys serialize away) and that `subscription` / `topup` now additionally emit it. 2. `HostTelemetrySink.ts` — a four-line method delegating to the same `getBillingTelemetryEventName` / `getBillingTelemetryEventPayload` helpers the two cloud providers use, so the host rail receives a byte-identical property set. 3. The three test files — each new test fails against the unfixed source and passes with it (verified by reverting only the source and re-running). **The design question worth a second opinion (fix 2).** There were two candidate fixes: implement `trackBillingEvent` on the sink, or make `initHostTelemetry` compose with the existing registry instead of replacing it. This PR implements the sink method, because that is what the surrounding code's intent points at: the sink already forwards 46 of the 54 `TelemetryProvider` methods through a single `capture()` bridge, and `trackBillingEvent` postdates the sink's creation — it was never a deliberate exclusion, just a method added later and not backfilled. Registry *replacement*, by contrast, looks deliberate: in the desktop shell the host process is the single telemetry egress point, and composing would double-emit every event on two rails with two different identities, affecting all ~40 forwarded events rather than just billing. That is a much larger blast radius and a cross-surface decision, so it is left as a follow-up question rather than decided here: **should a cloud build inside the desktop shell fan out to both the host bridge and the cloud providers?** Worth a reviewer's opinion, not blocking this fix. Also out of scope but noted while in here: `trackAuthFailed`, `trackExecutionOutcome`, `trackOnboardingTour`, `trackShellLayout`, `trackWidgetFavoriteToggled`, `trackWorkspaceInviteSent`, and `trackWorkspaceInviteFailed` are likewise absent from the host sink. Only `trackBillingEvent` is in scope here. **Why these two are worth landing promptly.** Fix 1 unblocks success-rate monitoring: a private, access-controlled infra repository has monitors pending review that are built on exactly the `succeeded / (succeeded + failed)` shape, and they cannot be applied against the current event stream without alerting permanently on an unreachable metric. Fix 2 closes a whole-segment blind spot — desktop-in-cloud users are currently absent from every billing funnel, on every provider. **Overlap with open PRs.** #14216 also edits `handleSuccess` in this file, adding a dual-emit *inside* the `type === 'subscription'` branch. This PR's hunk is an insertion *before* that branch plus the deletion of the trailing `else`. The two are semantically independent and compatible; expect at most a trivial textual conflict depending on merge order. This PR deliberately does **not** touch `useDowngradeToPersonal.ts`, whose three remaining `failure_category: 'unknown'` sites are already claimed by #14229. ## Testing Six new regression tests, all confirmed to fail against the unfixed source: - `billingOperationStore.test.ts` — parameterized over all three operation types, asserting `billing.operation.succeeded` fires with the full grouping-key payload. This is the test that pins the parity invariant. - `HostTelemetrySink.test.ts` — a success event and a failure event are forwarded to the host bridge with the derived event name and payload. - `initHostTelemetry.test.ts` — end-to-end through the registry: after `initHostTelemetry()`, a `trackBillingEvent` call reaches the host bridge. No e2e added: this is provider-dispatch-level code with no UI surface, and exercising it in Playwright would require driving a real billing flow against live payment rails. Per `browser_tests/README.md` this belongs at the unit level. Gates: `pnpm typecheck`, `pnpm lint`, `pnpm format:check`, `pnpm knip` all clean; full unit suite green (1067 files, 14204 passed / 8 skipped). Co-authored-by: Connor Byrne <c.byrne@comfy.org>
1 parent d622d43 commit 44a3cc8

5 files changed

Lines changed: 136 additions & 9 deletions

File tree

src/platform/telemetry/initHostTelemetry.test.ts

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,4 +51,30 @@ describe('initHostTelemetry', () => {
5151
undefined
5252
)
5353
})
54+
55+
it('dispatches canonical billing events through the host registry', () => {
56+
const capture = vi.fn()
57+
window.__comfyDesktop2 = { isRemote: () => false, Telemetry: { capture } }
58+
remoteConfig.value = { enable_telemetry: true }
59+
60+
initHostTelemetry()
61+
useTelemetry()?.trackBillingEvent({
62+
operation: 'subscription_checkout',
63+
stage: 'succeeded',
64+
outcome: 'success',
65+
billing_op_id: 'op-1',
66+
tier: 'pro'
67+
})
68+
69+
expect(capture).toHaveBeenCalledWith(
70+
TelemetryEvents.BILLING_SUBSCRIPTION_CHECKOUT_SUCCEEDED,
71+
{
72+
operation: 'subscription_checkout',
73+
stage: 'succeeded',
74+
outcome: 'success',
75+
billing_op_id: 'op-1',
76+
tier: 'pro'
77+
}
78+
)
79+
})
5480
})

src/platform/telemetry/providers/host/HostTelemetrySink.test.ts

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,54 @@ describe('HostTelemetrySink', () => {
156156
)
157157
})
158158

159+
it('forwards canonical billing events using the derived name and payload', () => {
160+
new HostTelemetrySink().trackBillingEvent({
161+
operation: 'operation',
162+
stage: 'succeeded',
163+
outcome: 'success',
164+
billing_op_id: 'op-1',
165+
operation_type: 'subscription',
166+
tier: 'pro',
167+
cycle: 'monthly',
168+
checkout_type: 'new'
169+
})
170+
171+
expect(state.capture).toHaveBeenCalledExactlyOnceWith(
172+
TelemetryEvents.BILLING_OPERATION_SUCCEEDED,
173+
{
174+
operation: 'operation',
175+
stage: 'succeeded',
176+
outcome: 'success',
177+
billing_op_id: 'op-1',
178+
operation_type: 'subscription',
179+
tier: 'pro',
180+
cycle: 'monthly',
181+
checkout_type: 'new'
182+
}
183+
)
184+
})
185+
186+
it('forwards billing failures with their failure category', () => {
187+
new HostTelemetrySink().trackBillingEvent({
188+
operation: 'topup',
189+
stage: 'failed',
190+
outcome: 'failure',
191+
billing_op_id: 'op-2',
192+
failure_category: 'provider_decline'
193+
})
194+
195+
expect(state.capture).toHaveBeenCalledExactlyOnceWith(
196+
TelemetryEvents.BILLING_TOPUP_FAILED,
197+
{
198+
operation: 'topup',
199+
stage: 'failed',
200+
outcome: 'failure',
201+
billing_op_id: 'op-2',
202+
failure_category: 'provider_decline'
203+
}
204+
)
205+
})
206+
159207
it('does nothing when the host bridge is absent', () => {
160208
delete window.__comfyDesktop2
161209

src/platform/telemetry/providers/host/HostTelemetrySink.ts

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import type {
1313
AddCreditsClickMetadata,
1414
AuthMetadata,
1515
BeginCheckoutMetadata,
16+
BillingTelemetryEvent,
1617
DefaultViewSetMetadata,
1718
EnterLinearMetadata,
1819
ExecutionErrorMetadata,
@@ -50,7 +51,12 @@ import type {
5051
WorkflowImportMetadata,
5152
WorkflowSavedMetadata
5253
} from '../../types'
53-
import { CANCELLATION_STAGE_EVENTS, TelemetryEvents } from '../../types'
54+
import {
55+
CANCELLATION_STAGE_EVENTS,
56+
TelemetryEvents,
57+
getBillingTelemetryEventName,
58+
getBillingTelemetryEventPayload
59+
} from '../../types'
5460
import { normalizeSurveyResponses } from '../../utils/surveyNormalization'
5561

5662
type HostTelemetryProperties = Parameters<
@@ -121,6 +127,13 @@ export class HostTelemetrySink implements TelemetryProvider {
121127
this.capture(TelemetryEvents.BEGIN_CHECKOUT, metadata)
122128
}
123129

130+
trackBillingEvent(event: BillingTelemetryEvent): void {
131+
this.capture(
132+
getBillingTelemetryEventName(event),
133+
getBillingTelemetryEventPayload(event)
134+
)
135+
}
136+
124137
trackMonthlySubscriptionSucceeded(
125138
metadata?: SubscriptionSuccessMetadata
126139
): void {

src/platform/workspace/stores/billingOperationStore.test.ts

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -362,6 +362,42 @@ describe('billingOperationStore', () => {
362362
})
363363
})
364364

365+
// Parity with .failed/.timeout, which fire for all three types: a
366+
// succeeded/(succeeded+failed) ratio reads a permanent 0% for any type
367+
// missing from the numerator.
368+
it.for(['subscription', 'topup', 'cancel'] as const)(
369+
'fires billing.operation.succeeded for a %s operation',
370+
async (type) => {
371+
vi.mocked(workspaceApi.getBillingOpStatus).mockResolvedValue({
372+
id: 'op-1',
373+
status: 'succeeded',
374+
started_at: new Date().toISOString()
375+
})
376+
377+
const store = useBillingOperationStore()
378+
void store.startOperation('op-1', type, {
379+
tier: 'creator',
380+
cycle: 'monthly',
381+
checkoutType: 'new',
382+
paymentIntentSource: 'subscription_required'
383+
})
384+
385+
await vi.advanceTimersByTimeAsync(0)
386+
387+
expect(mockTrackBillingEvent).toHaveBeenCalledWith({
388+
operation: 'operation',
389+
stage: 'succeeded',
390+
outcome: 'success',
391+
billing_op_id: 'op-1',
392+
operation_type: type,
393+
tier: 'creator',
394+
cycle: 'monthly',
395+
checkout_type: 'new',
396+
payment_intent_source: 'subscription_required'
397+
})
398+
}
399+
)
400+
365401
it('shows topup success message for topup operations', async () => {
366402
vi.mocked(workspaceApi.getBillingOpStatus).mockResolvedValue({
367403
id: 'op-1',

src/platform/workspace/stores/billingOperationStore.ts

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -278,6 +278,18 @@ export const useBillingOperationStore = defineStore('billingOperation', () => {
278278
cleanup(opId)
279279

280280
const telemetry = useTelemetry()
281+
telemetry?.trackBillingEvent({
282+
operation: 'operation',
283+
stage: 'succeeded',
284+
outcome: 'success',
285+
billing_op_id: opId,
286+
operation_type: operation.type,
287+
tier: operation.tier,
288+
cycle: operation.cycle,
289+
checkout_type: operation.checkoutType,
290+
payment_intent_source: operation.paymentIntentSource
291+
})
292+
281293
if (operation.type === 'subscription') {
282294
telemetry?.trackBillingEvent({
283295
operation: 'subscription_checkout',
@@ -308,14 +320,6 @@ export const useBillingOperationStore = defineStore('billingOperation', () => {
308320
outcome: 'success',
309321
billing_op_id: opId
310322
})
311-
} else {
312-
telemetry?.trackBillingEvent({
313-
operation: 'operation',
314-
stage: 'succeeded',
315-
outcome: 'success',
316-
billing_op_id: opId,
317-
operation_type: 'cancel'
318-
})
319323
}
320324

321325
const billingContext = useBillingContext()

0 commit comments

Comments
 (0)