feat(metering): track written row count per S3 export file#6736
Conversation
Adds nango.billing.usage.clickhouse.s3_export.rows (distribution, tagged by canonical metric name). Emitted after each successful export, sourcing `written_rows` from the ClickHouse X-ClickHouse-Summary response header that `client.command()` already returns. Visibility beyond the existing file-count gauge: we now know how many rows shipped per (day, metric), which is enough to plot per-metric volume trends in Datadog and pick up a sudden drop / spike before it reaches Orb. Also the foundation for future daily-parity reconciliation against Orb's ingested counts. INSERT INTO FUNCTION s3(...) is atomic on the S3 side (multipart completion is all-or-nothing), so on the success path the row count matches the file's line count exactly. On the failure path the exec throws and no metric is emitted; the only observability gap is the rare case where CH completed the write but the response was lost in transit — the file is still correct on S3, we just miss the row-count sample for that tick. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Per review. The cron produces exactly one row-count sample per (day, metric) per day — subsequent firings that day skip via the HeadObject check — so a distribution's percentiles don't add anything over a counter's sum/rate, and the counter shape is cheaper in DD. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Confidence score: 4/5
- In
packages/metering/lib/crons/billingEventsS3Export.ts, defaultingwrittenRowsto0whenres.summary.written_rowsis missing can silently turn an upstream metadata/header problem into a false “zero rows written” signal, which can mislead billing export monitoring and delay incident detection—treat missing summary data as an explicit error or emit a separate “unknown/missing summary” metric before merging.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/metering/lib/crons/billingEventsS3Export.ts">
<violation number="1" location="packages/metering/lib/crons/billingEventsS3Export.ts:201">
P2: When `res.summary` or `res.summary.written_rows` is absent, `writtenRows` silently defaults to 0 and gets emitted as a distribution metric. A missing summary header (e.g., proxy stripping it, or a ClickHouse config/version change) becomes indistinguishable from a genuinely empty export — the main observability signal this feature adds goes dark with no warning.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // exists fully or not at all), so on the success path this row | ||
| // count matches the line count in the S3 file exactly. | ||
| const res = await client.command({ query: exportSql({ metric, day, eventName, key }) }); | ||
| const writtenRows = Number(res.summary?.written_rows ?? 0); |
There was a problem hiding this comment.
P2: When res.summary or res.summary.written_rows is absent, writtenRows silently defaults to 0 and gets emitted as a distribution metric. A missing summary header (e.g., proxy stripping it, or a ClickHouse config/version change) becomes indistinguishable from a genuinely empty export — the main observability signal this feature adds goes dark with no warning.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/metering/lib/crons/billingEventsS3Export.ts, line 201:
<comment>When `res.summary` or `res.summary.written_rows` is absent, `writtenRows` silently defaults to 0 and gets emitted as a distribution metric. A missing summary header (e.g., proxy stripping it, or a ClickHouse config/version change) becomes indistinguishable from a genuinely empty export — the main observability signal this feature adds goes dark with no warning.</comment>
<file context>
@@ -192,12 +192,21 @@ export async function exec(): Promise<void> {
+ // exists fully or not at all), so on the success path this row
+ // count matches the line count in the S3 file exactly.
+ const res = await client.command({ query: exportSql({ metric, day, eventName, key }) });
+ const writtenRows = Number(res.summary?.written_rows ?? 0);
+ logger.info(`Exported ${eventName} for day=${day} (rows=${writtenRows})`);
metrics.increment(metrics.Types.BILLING_USAGE_CLICKHOUSE_S3_EXPORT_FILE_RESULT, 1, {
</file context>
There was a problem hiding this comment.
A 0 is what we want, we will have a monitor which will check similarity btw days
There was a problem hiding this comment.
A 0 is intentional here, and the separate similarity monitor covers the anomaly detection. The parent comment is too broad for this PR.
| BILLING_USAGE_CLICKHOUSE_S3_EXPORT_FILE_RESULT = 'nango.billing.usage.clickhouse.s3_export.file.result', | ||
| BILLING_USAGE_CLICKHOUSE_S3_EXPORT_RUN_RESULT = 'nango.billing.usage.clickhouse.s3_export.run.result', | ||
| BILLING_USAGE_CLICKHOUSE_S3_EXPORT_DURATION_MS = 'nango.billing.usage.clickhouse.s3_export.duration_ms', | ||
| BILLING_USAGE_CLICKHOUSE_S3_EXPORT_ROWS = 'nango.billing.usage.clickhouse.s3_export.rows', |
There was a problem hiding this comment.
what about some sort of reverse dns naming?
| BILLING_USAGE_CLICKHOUSE_S3_EXPORT_ROWS = 'nango.billing.usage.clickhouse.s3_export.rows', | |
| BILLING_USAGE_CLICKHOUSE_S3_EXPORT_ROWS = 'nango.billing.usage.clickhouse.export.s3.rows', |
There was a problem hiding this comment.
I would prefer to keep the name that Im proposing, not because is better but because it follows the current pattern and I already have a bunch of DD panels and monitors using the s3_export. Does it sound ok to you? 🙏
Summary
nango.billing.usage.clickhouse.s3_export.rows(counter, tagged by canonical metric name) — emitted after each successful S3 export, sourced from thewritten_rowsfield of ClickHouse'sX-ClickHouse-Summaryresponse header thatclient.command()already surfaces.command()call already returned the summary; we were just discarding it.Context
Part of NAN-6204. Complements the existing
s3_export.file.resultcounter — file count answers "did we ship?", row count answers "did we ship the right amount?".Foundation for the sibling parity-monitoring task: reconciling our exported row counts against Orb's ingested counts (daily, per metric).
Guarantees of the metric
On the success path, the reported
written_rowsmatches the exact number of rows in the S3 file. This holds becauseINSERT INTO FUNCTION s3(...)uses S3's multipart upload internally, and the target object appears atomically onCompleteMultipartUpload— either the object exists with the full payload or it doesn't exist at all. There is no observable intermediate state where the file is partially uploaded.Failure modes and what the metric shows in each
command()resultsummary.written_rows = NNcounted.file.result{success:false}counter fires insteadCompleteMultipartUploadobjectExists() === trueand skips, so the file is never re-uploaded. We lose that one row-count sample but nothing else.The only observability gap is the last row — the "response lost in transit" case, where the file is on S3 but we don't have a row-count sample for that (day, metric). We accept this because the file itself is fine, and how often this shape shows up is already tracked by the existing
.file.result{success:false}counter.Metric shape
metrics.increment).metric(canonical event name —records,proxy,function_executions, etc.).HeadObjectcheck.Test plan
nango.billing.usage.clickhouse.s3_export.rowssamples for each canonical metric.SELECT count(*)from the same day/metric to confirm the summary matches ground truth.Existing integration test at
billingEventsS3Export.integration.test.tsusesmetricRowsSql(the SELECT portion only, no S3 target), so it can't exercise the summary path without an S3-compatible endpoint. Live validation in dev instead.🤖 Generated with Claude Code