fix(slo): trustworthy delivery metric and NUL-safe snapshot writes - #70965
Conversation
The "SLO: 28d Success Rate" insight bucketed starts and completions by calendar day, so an operation that started late in a day and finished after midnight was counted as a failure on the start day and dropped on the next. For batch-heavy operations like subscription_delivery this manufactured hundreds of phantom failures on day boundaries. It also computed the 28-day rolling window over only 28 base rows, so the earliest ~27 points had partial windows and rendered as an artificial ramp on the left edge of the chart — easily misread as a recent decline. Pair starts to completions by correlation_id (the approach the burn rate query on the same dashboard already uses), and compute over 56 days while displaying only the fully-warmed last 28. Generated-By: PostHog Code Task-Id: 900469b2-6987-469a-a26b-3f040acd912f
Insight query results can contain NUL bytes (\x00) from upstream data. When the delivery snapshot is written to SubscriptionDelivery.content_snapshot, Django's JSONField serializes a NUL to a unicode escape that Postgres text and jsonb columns reject, failing the entire delivery write with a DataError. Strip NUL bytes from the serialized insight result. The strip is gated on the escape actually appearing in the orjson output, so clean (common-case) payloads skip the recursive walk. Generated-By: PostHog Code Task-Id: 900469b2-6987-469a-a26b-3f040acd912f
Promote the NUL-byte strip to a shared helper and apply it on the AI-report write path too. The AI report persists LLM-generated markdown, step diagnostics, and the user prompt to content_snapshot — all untrusted NUL sources that bypassed the insight-results serializer, so they could still fail the delivery write with the same DataError. Also drop a redundant sample_rate round-trip in the SLO success-rate query: the weighted_events CTE already exposes weight (1/sample_rate), so carry max(weight) through instead of re-inverting max(sample_rate). Generated-By: PostHog Code Task-Id: 900469b2-6987-469a-a26b-3f040acd912f
A HogQL Map(String, …) column deserializes to a dict whose keys come from event data, so a NUL can land in a key. strip_null_bytes only scrubbed values, leaving such a key to fail the delivery write with the same DataError the helper exists to prevent. Scrub keys too, and document the cross-file invariant at the insight merge site. Generated-By: PostHog Code Task-Id: 900469b2-6987-469a-a26b-3f040acd912f
A failed insight query stores str(exception) in content_snapshot's query_error message, bypassing the result serializer's scrub. ClickHouse error text can echo the offending query data, so a NUL there would still fail the delivery write. Scrub it at the source, making the "pre-scrubbed at source" invariant hold for every field on the insight snapshot. Generated-By: PostHog Code Task-Id: 900469b2-6987-469a-a26b-3f040acd912f
Address PR review: - Move strip_null_bytes to delivery_common (a general-purpose sanitizer with no dependency on insight types); re-export from insight_snapshot so existing importers keep resolving. - Handle tuples in strip_null_bytes for parity with lists, so a future caller passing a tuple (e.g. an un-flattened diagnostics collection) still gets scrubbed instead of silently passing NUL through. - Add AI-report test coverage with NUL-bearing markdown, diagnostics, and prompt so a dropped scrub is caught. Generated-By: PostHog Code Task-Id: 900469b2-6987-469a-a26b-3f040acd912f
🤖 CI report
|
| File | Patch | Uncovered changed lines |
|---|---|---|
products/exports/backend/temporal/subscriptions/delivery_common.py |
90.9% | 43 |
🤖 Agents: add a test covering the lines above, or note why under "How did you test this code?". Machine-readable gap list: the patch-coverage artifact on this run (gh run download 29413136263 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
platform_features |
██░░░░░░░░░░░░░░░░░░ 12.1% |
7 / 58 |
batch_exports |
████████░░░░░░░░░░░░ 39.7% |
8,414 / 21,220 |
demo |
███████████░░░░░░░░░ 56.2% |
1,497 / 2,663 |
warehouse_sources_queue |
████████████░░░░░░░░ 59.2% |
148 / 250 |
tasks |
█████████████░░░░░░░ 67.4% |
25,449 / 37,779 |
data_tools |
██████████████░░░░░░ 70.0% |
63 / 90 |
ai_gateway |
███████████████░░░░░ 75.0% |
9 / 12 |
signals |
████████████████░░░░ 79.1% |
19,074 / 24,106 |
data_modeling |
████████████████░░░░ 80.0% |
4,834 / 6,045 |
cdp |
████████████████░░░░ 80.7% |
3,118 / 3,864 |
wizard |
████████████████░░░░ 82.5% |
772 / 936 |
agent_platform |
█████████████████░░░ 84.2% |
3,112 / 3,695 |
notebooks |
█████████████████░░░ 84.3% |
6,323 / 7,501 |
cohorts |
█████████████████░░░ 86.0% |
3,989 / 4,639 |
actions |
█████████████████░░░ 86.6% |
717 / 828 |
product_tours |
█████████████████░░░ 87.5% |
1,266 / 1,447 |
exports |
██████████████████░░ 88.3% |
6,891 / 7,800 |
visual_review |
██████████████████░░ 88.5% |
5,565 / 6,287 |
business_knowledge |
██████████████████░░ 88.5% |
4,400 / 4,969 |
conversations |
██████████████████░░ 88.9% |
16,126 / 18,130 |
dashboards |
██████████████████░░ 89.0% |
5,647 / 6,345 |
mcp_analytics |
██████████████████░░ 89.1% |
2,502 / 2,807 |
error_tracking |
██████████████████░░ 89.5% |
9,683 / 10,816 |
engineering_analytics |
██████████████████░░ 89.8% |
4,861 / 5,414 |
streamlit_apps |
██████████████████░░ 90.4% |
2,499 / 2,764 |
slack_app |
██████████████████░░ 90.6% |
9,511 / 10,503 |
links |
██████████████████░░ 90.6% |
183 / 202 |
marketing_analytics |
██████████████████░░ 90.8% |
11,514 / 12,684 |
alerts |
██████████████████░░ 90.9% |
3,416 / 3,760 |
product_analytics |
██████████████████░░ 91.1% |
5,527 / 6,068 |
managed_migrations |
██████████████████░░ 91.9% |
908 / 988 |
workflows |
██████████████████░░ 92.0% |
4,795 / 5,210 |
data_warehouse |
██████████████████░░ 92.1% |
17,872 / 19,415 |
mcp_store |
██████████████████░░ 92.1% |
3,665 / 3,981 |
notifications |
███████████████████░ 92.7% |
1,026 / 1,107 |
web_analytics |
███████████████████░ 92.7% |
13,607 / 14,674 |
ai_observability |
███████████████████░ 92.8% |
14,771 / 15,923 |
surveys |
███████████████████░ 92.9% |
5,660 / 6,094 |
posthog_ai |
███████████████████░ 93.2% |
1,312 / 1,408 |
tracing |
███████████████████░ 93.2% |
2,423 / 2,599 |
approvals |
███████████████████░ 93.3% |
3,395 / 3,640 |
reminders |
███████████████████░ 93.4% |
468 / 501 |
early_access_features |
███████████████████░ 93.8% |
848 / 904 |
legal_documents |
███████████████████░ 94.1% |
1,568 / 1,667 |
endpoints |
███████████████████░ 94.1% |
8,606 / 9,143 |
skills |
███████████████████░ 94.4% |
2,827 / 2,995 |
revenue_analytics |
███████████████████░ 94.5% |
3,598 / 3,809 |
messaging |
███████████████████░ 94.5% |
2,530 / 2,677 |
review_hog |
███████████████████░ 94.6% |
6,475 / 6,848 |
logs |
███████████████████░ 95.3% |
9,525 / 9,991 |
growth |
███████████████████░ 95.5% |
2,734 / 2,864 |
experiments |
███████████████████░ 95.6% |
24,138 / 25,244 |
replay_vision |
███████████████████░ 95.7% |
13,300 / 13,896 |
feature_flags |
███████████████████░ 96.1% |
14,750 / 15,354 |
warehouse_sources |
███████████████████░ 96.2% |
219,800 / 228,543 |
annotations |
███████████████████░ 96.2% |
732 / 761 |
user_interviews |
███████████████████░ 96.4% |
2,242 / 2,325 |
access_control |
███████████████████░ 96.8% |
849 / 877 |
data_catalog |
███████████████████░ 97.1% |
2,034 / 2,095 |
customer_analytics |
███████████████████░ 97.3% |
7,442 / 7,648 |
analytics_platform |
████████████████████ 98.0% |
2,102 / 2,145 |
metrics |
████████████████████ 98.3% |
2,363 / 2,405 |
live_debugger |
████████████████████ 99.2% |
613 / 618 |
field_notes |
████████████████████ 99.4% |
158 / 159 |
Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.
ℹ️ ClickHouse migration SQL — 1 migration(s)
ClickHouse migration SQL per cloud environment
- unset
- all
CREATE OR REPLACE VIEW custom_metrics_test AS SELECT 'ClickHouseCustomMetric_Test' AS name, map('instance', hostname()) AS labels, 1 AS value, 'Test to check that the metric endpoint is working' AS help, 'gauge' AS type
CREATE OR REPLACE VIEW custom_metrics AS SELECT * REPLACE (toFloat64(value) as value) FROM custom_metrics_test
- all
- US, EU, DEV
- events, shufflehog
CREATE OR REPLACE VIEW custom_metrics_test AS SELECT 'ClickHouseCustomMetric_Test' AS name, map('instance', hostname()) AS labels, 1 AS value, 'Test to check that the metric endpoint is working' AS help, 'gauge' AS type
CREATE OR REPLACE VIEW custom_metrics AS SELECT * REPLACE (toFloat64(value) as value) FROM custom_metrics_test
- events, shufflehog
Remove the now-moot module-level note about strip_null_bytes moving to delivery_common, and expand the comment on the strip gate in _serialize_insight_result to explain the cost (multi-MB payloads), correctness (orjson always escapes a real NUL, incl. in keys, so the byte check never false-negatives), and why the AI path scrubs unconditionally instead. Generated-By: PostHog Code Task-Id: 900469b2-6987-469a-a26b-3f040acd912f
|
Note 🤖 stamphog reviewed Gates denied due to a deny-listed infrastructure change (terraform SLO monitoring query) and T2-never tier classification; needs human review before merging.
Gate mechanics and policy version
Updated in place — this replaces 1 earlier stamphog review(s) on this PR. |
|
Reviews (1): Last reviewed commit: "chore(subscriptions): document the NUL-s..." | Re-trigger Greptile |
Problem
The SLO Monitoring dashboard's "28d Success Rate" insight could misrepresent subscription-delivery health in two ways:
greatest(daily_starts − daily_successes, 0)), so a delivery that started late in a day and finished after midnight was counted as a failure on the start day and dropped on the next. For batch-heavy operations this manufactured a large number of phantom failures.Separately, a subscription delivery could fail its entire write when snapshot content contained a NUL byte (
\x00): Django's JSONField serializes NUL to a unicode escape that Postgres text/jsonb columns reject, raising aDataError. This affected insight query results, and — unguarded — AI report markdown, step diagnostics, the user prompt, and query-error messages.Changes
slo-monitoring/insights.tf): pair starts to completions bycorrelation_id(the approach the burn-rate query on the same dashboard already uses) so cross-midnight deliveries are attributed to their start day; compute over 56 days but display only the fully-warmed last 28. Uncorrelated events keep the per-day fallback. Also dropped a redundantsample_rateround-trip (weightis already1/sample_rate).strip_null_byteshelper (keys and values) applied at every untrusted ingress that reachescontent_snapshot— insight results, query-error messages, AI markdown/diagnostics, and the user prompt. The large insight payload stays behind a cheap byte-level gate so clean deliveries skip the walk.Why: the dashboard metric was firing on artifacts rather than real regressions, and the NUL
DataErrorwas failing real deliveries outright.How did you test this code?
test_serialize_insight_result_strips_null_bytescovering NUL in result strings, map-derived dict keys, and columns; it round-trips through stdlibjson(what JSONField uses) and asserts the output is NUL-free.strip_null_bytesalgorithm (keys + values at all nesting depths, plus the escape-detection gate) with a standalone simulation.sample_rate→weightsimplification is result-identical.ruff format/ruff checkon all changed Python.Automatic notifications
Docs update
🤖 Agent context
Autonomy: Human-driven (agent-assisted) — DRI assigned as the PR assignee.
Investigation started from an SLO snapshot report that flagged a subscription-delivery success-rate drop. Digging into the data showed the "drop" was mostly the dashboard metric's warm-up + day-boundary artifacts (true per-operation success rate is high), which motivated the query rewrite; the NUL
DataErrorsurfaced as a recurring real delivery failure in the same data./simplify(removed the redundantsample_rateround-trip; promoted the NUL helper to a shared function and applied it on the AI-report path) and/code-review(high effort). The review caught thatstrip_null_bytesscrubbed dict values but not keys — a NUL in aMap(String, …)key would still fail the write — now fixed and tested. A confirmation pass then flagged that query-error messages bypassed the scrub; closed at the source.Created with PostHog from a Slack thread