Skip to content

feat(metering): track written row count per S3 export file #6736

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pfreixes merged 2 commits into from
Jul 13, 2026
+12 −2
Merged

feat(metering): track written row count per S3 export file #6736

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6582161
feat(metering): track written row count per S3 export file
pfreixes Jul 10, 2026
e2f06b6
fix(metering): use counter (not distribution) for s3_export.rows
pfreixes Jul 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions packages/metering/lib/crons/billingEventsS3Export.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,12 +192,21 @@ export async function exec(): Promise<void> {
}
step = 'export';
logger.info(`Exporting ${eventName} for day=${day}`);
await client.command({ query: exportSql({ metric, day, eventName, key }) });
logger.info(`Exported ${eventName} for day=${day}`);
// ClickHouse populates `written_rows` in the X-ClickHouse-Summary
// response header once the multipart upload to S3 is complete
// (INSERT INTO FUNCTION s3(...) is atomic — the object either
// 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);

@cubic-dev-ai cubic-dev-ai Bot Jul 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

@pfreixes pfreixes Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A 0 is what we want, we will have a monitor which will check similarity btw days

@cubic-dev-ai cubic-dev-ai Bot Jul 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A 0 is intentional here, and the separate similarity monitor covers the anomaly detection. The parent comment is too broad for this PR.

logger.info(`Exported ${eventName} for day=${day} (rows=${writtenRows})`);
metrics.increment(metrics.Types.BILLING_USAGE_CLICKHOUSE_S3_EXPORT_FILE_RESULT, 1, {
metric: metric.canonicalEventName,
success: 'true'
});
metrics.increment(metrics.Types.BILLING_USAGE_CLICKHOUSE_S3_EXPORT_ROWS, writtenRows, {
metric: metric.canonicalEventName
});
} catch (err) {
// Per-metric catch so a single failure (e.g. CH timeout on
// a heavy table) does not abort the rest of the run.
Expand Down
1 change: 1 addition & 0 deletions packages/utils/lib/telemetry/metrics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ export enum Types {
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',

@TBonnin TBonnin Jul 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about some sort of reverse dns naming?

Suggested change
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',

@pfreixes pfreixes Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 🙏

BILLING_USAGE_TRACKER_CALLS = 'nango.billing.usage.tracker.calls',
BILLING_EVENTS_S3_DLQ_FILES = 'nango.billing.events.s3.dlq.files',
BILLING_EVENTS_S3_DLQ_MONITOR_RUN_RESULT = 'nango.billing.events.s3.dlq.monitor.run.result',
Expand Down
Loading