Skip to content

Commit 36df1a1

Browse files
committed
fix(webapp,clickhouse): keep ClickHouse row snippets out of logs
The untruncated ClickHouse error text is kept only so the recovery path can read the failing-row hint, but ClickHouse embeds a snippet of the offending row in its parse errors. It is now a non-enumerable field, so structured logging and error reporting cannot pick it up while direct reads still work. The dropped-row count reported zero on tables whose materialized views fold MV rows into written_rows, which reads as "no data lost" when rows had in fact been skipped. Reaching an allow_errors insert always means at least one row is un-ingestable, so the count now reports that floor and flags itself inexact, and a batch only counts as wholly dropped when ClickHouse's summary says so exactly. The skip path logs at warn to match, since it always loses a row. Also drops the two event-repository counters that could never leave zero (that path skips rows rather than stripping columns, so nothing fed them), and names the two causes of an allow_errors bail as a bailReason log field instead of reporting both as a strip-budget hit.
1 parent 535889f commit 36df1a1

8 files changed

Lines changed: 235 additions & 121 deletions

File tree

apps/webapp/app/services/runsReplicationService.server.ts

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -181,23 +181,26 @@ export class RunsReplicationService {
181181
* Counts batches where every row was un-ingestable even with its JSON
182182
* stripped, so nothing landed. Row isolation lands anything strippable, so
183183
* this should stay at zero; a non-zero value means the recovery itself is
184-
* failing. Counter only, does not gate behaviour.
184+
* failing. Only incremented when ClickHouse's summary says so exactly
185+
* (`written_rows === 0`). Counter only, does not gate behaviour.
185186
*/
186187
private _permanentlyDroppedBatches = 0;
187188

188189
/**
189190
* Counts batches that took the row-isolation recovery path: a
190191
* `Cannot parse JSON object` failure the sanitizer could not repair, where we
191-
* bisected to the poison rows and landed the batch with their JSON stripped.
192-
* Reliable per-event signal that user data is hitting the ceiling.
192+
* followed ClickHouse's `at row N` hint to the poison rows and landed the batch
193+
* with their JSON stripped. Reliable per-event signal that user data is hitting
194+
* the ceiling.
193195
*/
194196
private _rowIsolationRecoveries = 0;
195197

196198
/**
197-
* Counts batches whose isolation blew the per-batch insert budget (a poison
198-
* flood), so the remaining rows were stripped in one insert instead of
199-
* bisected further. A burst signal; clean rows in those batches also lose
200-
* their JSON.
199+
* Counts batches that gave up on isolating rows precisely and fell back to a
200+
* single `allow_errors` insert, either because the per-batch strip budget was
201+
* spent (a poison flood) or because ClickHouse gave no usable row hint. The
202+
* remaining un-ingestable rows are skipped; `bailReason` in the log line says
203+
* which cause it was.
201204
*/
202205
private _recoveryCapHits = 0;
203206

@@ -210,6 +213,8 @@ export class RunsReplicationService {
210213
/**
211214
* Counts rows dropped entirely because they could not be parsed even with
212215
* their JSON stripped. The true data-loss signal; expected to stay near zero.
216+
* A lower bound on `task_runs_v2`, whose materialized views make the exact
217+
* count underivable from ClickHouse's insert summary (see `droppedRowCount`).
213218
*/
214219
private _permanentlyDroppedRows = 0;
215220

@@ -290,7 +295,7 @@ export class RunsReplicationService {
290295

291296
this._recoveryCapHitsCounter = this._meter.createCounter("runs_replication.recovery_cap_hits", {
292297
description:
293-
"Batches whose row isolation hit the per-batch insert budget and stripped the remainder in one insert (poison-flood signal)",
298+
"Batches that fell back to a single allow_errors insert instead of isolating rows, because the per-batch strip budget was spent or ClickHouse gave no usable row hint",
294299
unit: "batches",
295300
});
296301

@@ -302,7 +307,7 @@ export class RunsReplicationService {
302307

303308
this._rowsDroppedCounter = this._meter.createCounter("runs_replication.rows_dropped", {
304309
description:
305-
"Rows dropped entirely because they could not parse even with their JSON stripped",
310+
"Rows dropped entirely because they could not parse even with their JSON stripped; a lower bound on task_runs_v2, whose materialized views make the exact count underivable from ClickHouse's insert summary",
306311
unit: "rows",
307312
});
308313

@@ -1232,7 +1237,7 @@ export class RunsReplicationService {
12321237
if (outcome.rowsDropped > 0) {
12331238
this._permanentlyDroppedRows += outcome.rowsDropped;
12341239
this._rowsDroppedCounter.add(outcome.rowsDropped, { table: contextLabel });
1235-
if (outcome.rowsDropped === batchSize) {
1240+
if (outcome.rowsDroppedExact && outcome.rowsDropped === batchSize) {
12361241
this._permanentlyDroppedBatches += 1;
12371242
this._droppedBatchesCounter.add(1, { table: contextLabel });
12381243
}

apps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.ts

Lines changed: 17 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -121,39 +121,28 @@ export class ClickhouseEventRepository implements IEventRepository {
121121
private _tracer: Tracer;
122122
private _version: "v1" | "v2";
123123
/**
124-
* Counts batches where every row was un-ingestable even with its JSON
125-
* stripped, so nothing landed. Row isolation lands anything strippable, so
126-
* this should stay at zero; a non-zero value means the recovery is failing.
124+
* Counts batches where every row was un-ingestable, so nothing landed. Only
125+
* incremented when ClickHouse's summary says so exactly (`written_rows === 0`);
126+
* expected to stay at zero, since a whole batch of un-ingestable events means
127+
* something upstream is broken rather than one bad payload.
127128
*/
128129
private _permanentlyDroppedBatches = 0;
129130
private readonly _droppedBatchesCounter: Counter;
130131

131132
/**
132-
* Counts batches that took the row-isolation recovery path: a
133-
* `Cannot parse JSON object` failure the sanitizer could not repair, where we
134-
* bisected to the poison rows and landed the batch with their JSON stripped.
135-
* Reliable per-event signal.
133+
* Counts batches that took the bad-row-skip recovery path: a
134+
* `Cannot parse JSON object` failure the sanitizer could not repair, where one
135+
* `allow_errors` insert landed the good rows and skipped the un-ingestable
136+
* ones. Every such batch lost at least one row, so this is the alertable
137+
* signal for these tables.
136138
*/
137139
private _rowIsolationRecoveries = 0;
138140
private readonly _rowIsolatedBatchesCounter: Counter;
139141

140142
/**
141-
* Counts batches whose isolation blew the per-batch insert budget (a poison
142-
* flood), so the remaining rows were stripped in one insert. A burst signal.
143-
*/
144-
private _recoveryCapHits = 0;
145-
private readonly _recoveryCapHitsCounter: Counter;
146-
147-
/**
148-
* Counts rows that landed with their un-ingestable JSON stripped (the span
149-
* kept its place in the trace, only the attributes content was lost).
150-
*/
151-
private _rowsStripped = 0;
152-
private readonly _rowsStrippedCounter: Counter;
153-
154-
/**
155-
* Counts rows dropped entirely because they could not parse even with their
156-
* JSON stripped. The true data-loss signal; expected to stay near zero.
143+
* Counts rows skipped as un-ingestable. A floor, not an exact count: these
144+
* tables carry row-multiplying materialized views, so ClickHouse's insert
145+
* summary can't separate skipped base rows from MV rows (see `droppedRowCount`).
157146
*/
158147
private _permanentlyDroppedRows = 0;
159148
private readonly _rowsDroppedCounter: Counter;
@@ -171,22 +160,12 @@ export class ClickhouseEventRepository implements IEventRepository {
171160
});
172161
this._rowIsolatedBatchesCounter = meter.createCounter("ingest.flush.batches_row_isolated", {
173162
description:
174-
"Batches recovered by isolating un-ingestable rows (landed the rest) after a ClickHouse JSON parse error",
163+
"Batches recovered by skipping un-ingestable rows (landed the rest) after a ClickHouse JSON parse error; each lost at least one row",
175164
unit: "batches",
176165
});
177-
this._recoveryCapHitsCounter = meter.createCounter("ingest.flush.recovery_cap_hits", {
178-
description:
179-
"Batches whose row isolation hit the per-batch insert budget and stripped the remainder in one insert (poison-flood signal)",
180-
unit: "batches",
181-
});
182-
this._rowsStrippedCounter = meter.createCounter("ingest.flush.rows_stripped", {
183-
description:
184-
"Rows landed with their un-ingestable JSON stripped (kept the row, lost only the JSON content)",
185-
unit: "rows",
186-
});
187166
this._rowsDroppedCounter = meter.createCounter("ingest.flush.rows_dropped", {
188167
description:
189-
"Rows dropped entirely because they could not parse even with their JSON stripped",
168+
"Rows skipped as un-ingestable, as a lower bound: these tables' materialized views make the exact count underivable from ClickHouse's insert summary",
190169
unit: "rows",
191170
});
192171

@@ -243,22 +222,12 @@ export class ClickhouseEventRepository implements IEventRepository {
243222
return this._permanentlyDroppedBatches;
244223
}
245224

246-
/** Exposed for tests and metrics — batches that took the row-isolation recovery path. */
225+
/** Exposed for tests and metrics — batches that took the bad-row-skip recovery path. */
247226
get rowIsolationRecoveries() {
248227
return this._rowIsolationRecoveries;
249228
}
250229

251-
/** Exposed for tests and metrics — batches whose isolation hit the per-batch insert budget. */
252-
get recoveryCapHits() {
253-
return this._recoveryCapHits;
254-
}
255-
256-
/** Exposed for tests and metrics — rows that landed with their un-ingestable JSON stripped. */
257-
get rowsStripped() {
258-
return this._rowsStripped;
259-
}
260-
261-
/** Exposed for tests and metrics — rows dropped entirely (could not parse even stripped). */
230+
/** Exposed for tests and metrics — rows skipped as un-ingestable (a lower bound). */
262231
get permanentlyDroppedRows() {
263232
return this._permanentlyDroppedRows;
264233
}
@@ -416,20 +385,10 @@ export class ClickhouseEventRepository implements IEventRepository {
416385
this._rowIsolationRecoveries += 1;
417386
this._rowIsolatedBatchesCounter.add(1, { table: contextLabel });
418387

419-
if (outcome.capped) {
420-
this._recoveryCapHits += 1;
421-
this._recoveryCapHitsCounter.add(1, { table: contextLabel });
422-
}
423-
424-
if (outcome.rowsStripped > 0) {
425-
this._rowsStripped += outcome.rowsStripped;
426-
this._rowsStrippedCounter.add(outcome.rowsStripped, { table: contextLabel });
427-
}
428-
429388
if (outcome.rowsDropped > 0) {
430389
this._permanentlyDroppedRows += outcome.rowsDropped;
431390
this._rowsDroppedCounter.add(outcome.rowsDropped, { table: contextLabel });
432-
if (outcome.rowsDropped === batchSize) {
391+
if (outcome.rowsDroppedExact && outcome.rowsDropped === batchSize) {
433392
this._permanentlyDroppedBatches += 1;
434393
this._droppedBatchesCounter.add(1, { table: contextLabel });
435394
}

0 commit comments

Comments
 (0)