← all branches

feat/posthog-org-360

needs attention
b1b145a · incrementalpre-PRreviewed 2026-08-04 18:03 UTC0H · 4M · 5L · 12I
The branch
Purpose
Add an Org-360 PostHog insight that joins warehouse business tables with PostHog engagement events to give a single-row-per-org view of health, growth, MRR, and delivery (BAT-295).
Goal
Ship a codified DataVisualizationNode insight on the platform_health dashboard that analysts can use as the feature-vector source for org health and persona clustering.
Sub-goals
  • SG-1: Correct argMax tie-break for same-period ledger corrections
  • SG-2: Guard credit columns to NULL when ledger not active
  • SG-3: Comprehensive delivery rate denominator (all attempted RPUs, not just error-of-data)
  • SG-4: Fix epoch-zero LastActive filter (isNotNull → > 0)
The changes (whole branch)
What
Round-2 review fixes to org_360 SQL: (1) argMax now uses (billing_period_start, created_at) tuple for deterministic same-period tie-break; (2) cred_incluidos/cred_consumidos guarded to NULL when ledger inactive; (3) pct_entregados denominator switched from delivered+failing to rpus_intentados_30d (all RPUs with any collect job); (4) ultima_actividad uses > 0 instead of isNotNull; (5) comment refinements and LIMIT comment added.
Why
Prior review caught that the old argMax (single key) was non-deterministic on same-period corrections, stale credit values were shown for inactive orgs, and the old delivery rate denominator only counted data-error failures (not auth/timeout/CFE failures), underreporting failure scope.
Areas
infra/posthog/insights.tf+1293
Blast
1 file, +129/-3 lines. IaC-only. Analytics dashboard change — no application code, no migrations, no API changes. Terraform apply required to deploy.
requires-terraform-apply analytics-only no-app-code
typecheck· IaC-only change (Terraform/HogQL), no TypeScripttests· HogQL SQL insights are not unit-testable locallyci· GitHub auth unavailable in runner contextcoderabbit· No .coderabbit.yaml in repo

Findings · 16

correctness5

low

pct_entregados has no upper-bound guard (unlike ratio_activos)

infra/posthog/insights.tf

Currently delivered ≤ intentados by construction so the ratio is in [0,1]. But unlike ratio_activos there is no least(..., 1.0) guard. If a future classification change breaks the subset invariant the ratio silently exceeds 1 without any structural catch. Low risk now, but inconsistent with the defensive approach used for ratio_activos.

low

mrr_creditos_mxn uses raw credits_consumed; cred_consumidos uses round() — asymmetry

infra/posthog/insights.tf

The MRR overage calculation uses l.credits_consumed unrounded while cred_consumidos displays round(l.credits_consumed, 0). Intentional (precision for finance, rounded for display), but the diff introduces the display rounding while leaving the calculation raw — a future reader may unify them incorrectly. A brief comment on the MRR formula line noting 'precise value intentional' would prevent accidental rounding.

info

argMax with tuple (billing_period_start, created_at) is valid ClickHouse syntax

infra/posthog/insights.tf

Tuple comparison is lexicographic in ClickHouse 21.x+: correctly resolves same-period ties by preferring the latest created_at. Semantics match the stated intent.

info

DateTime > 0 epoch filter is correct for ClickHouse LEFT JOIN fill value

infra/posthog/insights.tf

ClickHouse stores DateTime as UInt32 Unix timestamp; LEFT JOIN miss fills with 0 (1970 epoch), not NULL. Comparing > 0 correctly filters this sentinel. isNotNull would not work because the filled value is a valid non-null zero.

info

NULL guards on cred_incluidos/cred_consumidos are correct and consistent

infra/posthog/insights.tf

Both credit columns now return NULL when ledger is not active, consistent with mrr_creditos_mxn = 0. Eliminates stale credit values from prior periods shown alongside zero MRR.

security1

info

No injection vectors — static Terraform HogQL string, no dynamic interpolation

infra/posthog/insights.tf

All table references and column names are hardcoded literals. No user-controlled inputs. Fully namespaced warehouse tables.

tests1

info

No automated tests for HogQL — IaC SQL not unit-testable locally

infra/posthog/insights.tf

PostHog HogQL insights cannot be unit-tested outside the PostHog environment. No test coverage is expected or possible here. Validation happens by running the insight in PostHog.

improvement9

medium

Repeated ledger_status guard — divergence risk across 3 columns

infra/posthog/insights.tf

l.ledger_status = 'active' gates mrr_creditos_mxn, cred_incluidos, and cred_consumidos independently via three separate IF expressions. If the rule changes (e.g. 'trialing' should also show credits), all three need sync updates. Move the flag into the ledger subquery as a computed column (e.g. status = 'active' AS is_active) so the three outer expressions share one definition.

medium

toString(j.error) for JSON extraction is fragile

infra/posthog/insights.tf

JSONExtractString(toString(j.error), 'code') stringifies then re-parses. If j.error is already String, toString is a no-op but misleading. If it is a JSON/Object column, toString may produce non-standard output (e.g. single-quoted keys). Use the direct extractor matching the column's actual type. Document the column type to prevent silent breakage on schema change.

medium

rpus_intentados semantics: name implies 'run attempts' but measures 'distinct RPUs with any job'

infra/posthog/insights.tf

count() at the outer level counts distinct (org_id, rpu) pairs — RPUs with at least one job, not job executions. The comment says 'INCLUYE reintentos' for jobs but rpus_intentados counts RPUs not jobs. pct_entregados = delivered_RPUs / attempted_RPUs (fraction of RPUs that achieved delivery), which is correct but different from 'fraction of runs that succeeded'. Consider rpus_con_actividad_30d or clarify with a longer comment.

medium

ratio_activos cap at 1.0 silently hides join mismatches

infra/posthog/insights.tf

least(..., 1.0) clamps ratios above 100% rather than surfacing them. A ratio > 1.0 signals a real data-quality issue (PostHog tracking removed members, or org_id/public_id join mismatch). Either surface the raw ratio alongside the capped value or add a boolean flag activos_gt_miembros to make anomalies visible.

low

Plan fallback coalesce(l.plan_id, o.plan_id) can show historical plan alongside zero MRR

infra/posthog/insights.tf

An org with no active ledger still shows its org-level plan and subscription_status, which can make a churned org look active at a glance (plan != null, but MRR = 0, credits = NULL). Add a comment or a derived 'es_activo' boolean that combines ledger_status = 'active' with mrr > 0 to make the state obvious in one column.

low

Events JOIN on public_id — no comment confirming PostHog instrumentation uses public_id

infra/posthog/insights.tf

All other JOINs use o.id (internal UUID PK). The PostHog events JOIN uses o.public_id via $group_0. Correct if the identify call sends public_id, but silently drops events if any path sends the internal id. A one-line comment ('PostHog group identify sends public_id') prevents a future engineer from 'fixing' the join.

low

LIMIT 500 comment added but should note when to revisit

infra/posthog/insights.tf

The new comment '~40 orgs activas, techo holgado' is good context. Consider adding a threshold at which to revisit (e.g. 'revisit at 200+ orgs') so the limit doesn't silently truncate as the customer base grows.

info

Missing: last successful CFE collection date per org (churn signal)

infra/posthog/insights.tf

The 360 view has engagement and delivery health but no column for 'days since last successful collect'. An org could have zero failing RPUs because it has no subscriptions or because it churned. max(created_at) WHERE status='completed' per org would surface dormant accounts.

info

MRR gap: orgs on flat-rate plans with no credit_ledger row show MRR=0

infra/posthog/insights.tf

If legacy flat-rate customers have no ledger row, ledger_status is NULL and mrr_creditos_mxn = 0 even though they are paying. Acknowledged in existing code comment ('NO es el MRR total real') but worth tracking as a known gap.

History · 3 commits

  1. b1b145aneeds attentionincremental0H · 4M · 5L2026-08-04 18:03current
  2. 3e64ea7needs attentionincremental1H · 5M · 4L2026-08-04 03:19
  3. 1581114needs attentionfull4H · 5M · 5L2026-08-04 00:07