feat(exporter): unified Arrow exporter with GUC + end-to-end TAP test - #116
Merged
Conversation
JoshDreamland
force-pushed
the
unified_arrow_exporter
branch
3 times, most recently
from
June 18, 2026 21:03
edfb438 to
0e48f7f
Compare
…to-end Introduce a new exporter that builds Arrow IPC RecordBatches through the typed StatsExporter column-factory interface (StatLC/StatHC/StatTimestamp) instead of the open-coded ArrowBatchBuilder used by arrow_batch.cc. Composition over inheritance: the new exporter holds an OTelExporter for gRPC transport (SendArrowBatch) but doesn't extend it, so the per-row LogRecord state machine in OTelExporter — which is unused on this path post-PR-#72 — stays out of scope. Wire shape targets events_raw (the unified schema authored in PR #99), not the legacy query_logs_arrow: * query_id, parent_query_id: Int64 (no sprintf decimal-string encoding) * pid: Int32 * err_elevel: UInt8 * buffer counters (shared/local/temp_blks_*, *_blk_*_time_us, wal_*, cpu_*_time_us): Int64 * parallel_workers_planned/launched: Int16 * jit_*: Int32 * LC strings (db_*, err_sqlstate, app, server_role, region, cell, service_version, read_replica_type) -> DictionaryUtf8 * HC strings (query_text, err_message, client_addr, instance_ubid, server_ubid, host_id, pod_name) -> plain utf8 * ts: arrow::timestamp(MICRO, "UTC") matching DateTime64(6, 'UTC') Column<T> wrappers are nested private types inside OTelArrowExporter (not at namespace scope) so they can inherit from the protected Column<T> base — same convention OTelExporter and ClickHouseExporter use for their own column types. Columns the caller doesn't explicitly populate are synthesized in BeginRow by the exporter itself, so stats_exporter.cc's column-emission loop stays unchanged: * parent_query_id (hardcoded 0 until PR #95 lands and PschEvent carries the field — events_raw requires the column on every insert, no DEFAULT) * 8 envelope columns from pg_stat_ch.extra_attributes (instance_ubid, server_ubid, server_role, region, cell, host_id, pod_name) plus read_replica_type (default 'none' if extra_attributes didn't supply) * service_version pinned to the compile-time PG_STAT_CH_VERSION macro This commit only adds the exporter file (no dispatcher wiring yet) — the next commit adds the GUC and routes batches through it when on. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
New bool GUC pg_stat_ch.use_unified_arrow_exporter (default off, PGC_SIGHUP)
opts a producer instance into the OTelArrowExporter added in the previous
commit. When on (together with use_otel and otel_arrow_passthrough), the
bgworker constructs an OTelArrowExporter at init time and PschExportBatch
goes through ExportEventStats (the typed column interface) instead of
calling ExportEventsAsArrow (the legacy bypass that uses arrow_batch.cc
directly).
When off — the default — behavior is preserved bit-for-bit:
* arrow_batch.cc / ExportEventsAsArrow remain reachable on the
arrow_passthrough path
* The OTelExporter column-emission path (off-arrow OTel logs) remains
reachable when arrow_passthrough is off
* The CH-native ClickHouseExporter remains reachable when use_otel is off
The sprintf-decimal-string ID encoding in arrow_batch.cc lives on for
the legacy path; the new exporter neither calls into it nor perpetuates
it. After the GUC has been on in prod long enough to retire the legacy
query_logs_arrow table, the arrow_batch.cc + ExportEventsAsArrow* + the
otel_arrow_passthrough GUC itself can be deleted in a single follow-on.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…_raw
Closes the test gap that's existed since the Arrow path went live: no
existing test proves that pg_stat_ch's Arrow IPC output can actually be
ingested by ClickHouse against the unified events_raw schema. t/026
asserts on the IPC schema shape via pyarrow but never pushes the bytes
into CH; t/010 etc. exercise the CH-native Block path, not Arrow.
The new test wires the full producer-to-CH chain locally, bypassing
the OTel collector + receiver service entirely:
1. Spin up a node with use_unified_arrow_exporter=on +
debug_arrow_dump_dir set, an OTel endpoint that doesn't resolve so
gRPC send fails — MaybeDumpArrowBatch fires BEFORE send so IPC
files land on disk regardless.
2. Run a deliberately-shaped workload (SELECT, CREATE, INSERT,
SELECT count, DROP — five distinct statements).
3. Force pg_stat_ch_flush(), wait for IPC files in $dump_dir.
4. TRUNCATE pg_stat_ch.events_raw, then for each IPC file:
curl -X POST --data-binary @$f \
'http://localhost:18123/?query=INSERT INTO pg_stat_ch.events_raw FORMAT ArrowStream'
A type mismatch on the wire (e.g. if the producer regressed to
writing query_id as String) would surface here as a 4xx with a
clear error rather than silently corrupting data.
5. SELECT count() FROM events_raw, assert >= 5 rows.
6. Pull system.columns and assert each id/counter column has the
declared type from PR #99's schema (no silent string-typed regressions).
7. Pinpoint the marker SELECT row and assert db_name/db_operation/
query_text values match what we sent.
8. Assert envelope columns (instance_ubid, server_role, region, cell,
read_replica_type) carry the values from pg_stat_ch.extra_attributes.
9. Assert parent_query_id is 0 across all rows (synthesized by the
exporter until PR #95 lands).
Skips cleanly when Docker / the test CH container / the events_raw
schema aren't available — same patterns as t/010, t/013, t/021.
The "no OTel collector required" property makes this test purely a
producer⇄CH wire-format check. The clickgres-platform Go receiver is
not exercised here, since for verifying that the bytes match the
schema, a curl invocation is the simplest possible expression of "POST
this Arrow IPC body to CH" — the receiver's only added value over
curl in prod is OTel-collector pipeline integration, which we don't
care about for wire-format correctness.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
JoshDreamland
force-pushed
the
unified_arrow_exporter
branch
from
June 18, 2026 21:12
0e48f7f to
021e75f
Compare
JoshDreamland
marked this pull request as ready for review
June 18, 2026 21:22
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new unified Arrow IPC exporter (behind a new GUC) that emits the events_raw-shaped ArrowStream payload via the existing OTel gRPC transport, plus an end-to-end TAP test that validates ClickHouse can ingest the dumped IPC bytes.
Changes:
- Introduces
OTelArrowExporterimplementingStatsExporterend-to-end with typed Arrow builders targetingevents_raw. - Adds
pg_stat_ch.use_unified_arrow_exporter(PGC_SIGHUP, default off) and dispatch logic to select the unified path vs the legacyarrow_batch.ccbypass. - Adds
t/036_unified_arrow_e2e.plto dump Arrow IPC and insert it into ClickHouse usingFORMAT ArrowStream, asserting basic correctness.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/regression/expected/guc.out | Updates expected GUC listing to include pg_stat_ch.use_unified_arrow_exporter. |
| t/036_unified_arrow_e2e.pl | New TAP test for Arrow IPC → ClickHouse events_raw ingestion. |
| src/export/stats_exporter.cc | Wires unified exporter construction and routes unified batches through ExportEventStats. |
| src/export/otel_arrow_exporter.h | Declares MakeUnifiedArrowExporter() factory. |
| src/export/otel_arrow_exporter.cc | Implements unified Arrow IPC builder/exporter and envelope column synthesis. |
| src/config/guc.c | Adds new boolean GUC definition and backing variable. |
| include/config/guc.h | Exposes the new GUC variable to the exporter code. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Three real findings worth addressing, plus a TODO for one that's worth
flagging but bigger than this PR:
src/export/otel_arrow_exporter.cc:
* NumExported() now returns row_count_ instead of delegating to the
inner OTelExporter. The inner exporter's exported_count_ accumulates
across batches because we never call inner_->BeginBatch() (its
per-record LogRecord state machine is unused on this path), and
PschExportBatch fetch_add's NumExported() into shared stats once per
successful commit — so the cumulative inner count would cause
quadratic over-reporting. (Cursor Bugbot, medium.)
* ExtraAttrs class comment said "last write wins on duplicate keys"
but Get() does a linear scan from the front and returns the first
match. Comment now reflects what the code actually does. Duplicate
keys in pg_stat_ch.extra_attributes are a user error in practice;
no behavior change. (Copilot.)
* Added a TODO(memory-budget) at CommitBatch documenting the missing
mid-batch flush against psch_otel_max_block_bytes. The legacy
ExportEventsAsArrowInternal flushes when the builder exceeds 3 MiB;
this exporter ships the whole batch in one IPC. Acceptable while
the GUC defaults off, but the budget check has to land before the
default flips. Plumbing it through requires either invalidating
caller-held column shared_ptrs at the flush boundary or threading
a per-row size hook through the StatsExporter interface. (Cursor
Bugbot, high — disagreeing on severity for the shadow-rollout
window but acknowledging the underlying gap.)
* Comment note on MaybeDumpArrowBatch acknowledging the intentional
duplication with stats_exporter.cc — both copies die when the
legacy path retires, so extracting a shared helper now would be a
header just to delete it later. (Copilot, push-back.)
t/036_unified_arrow_e2e.pl:
* curl now uses --fail-with-body so HTTP 4xx/5xx from CH surfaces as
a non-zero exit at the assertion site. The downstream SELECT count()
assertion would catch a real ingestion failure too, but the sharper
error at the source is a strict improvement. (Copilot.)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The bgworker constructs the exporter implementation once at init time
based on this GUC, but PschExportBatch was re-reading it every cycle to
choose between the legacy ExportEventsAsArrow bypass and the new
unified ExportEventStats path. Under PGC_SIGHUP, the two reads can
disagree:
- Init=on, runtime=off: dispatcher takes the legacy bypass and calls
SendArrowBatch on an OTelArrowExporter, which doesn't override it,
so every batch silently drops.
- Init=off, runtime=on: dispatcher runs the unified path through a
plain OTelExporter, which ships OTLP log records instead of Arrow
IPC. events_raw never sees the data.
Flag is a producer-shape feature toggle, not an operational tunable;
flipping it should be a deliberate restart.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| if (psch_use_otel && psch_otel_arrow_passthrough && psch_use_unified_arrow_exporter) { | ||
| // New unified path: typed Arrow column wrappers driving the | ||
| // StatsExporter interface end-to-end, writing the events_raw schema. | ||
| g_exporter.exporter = MakeUnifiedArrowExporter(); |
There was a problem hiding this comment.
Passthrough reload mismatches exporter
Medium Severity
PschExporterInit only builds MakeUnifiedArrowExporter when otel_arrow_passthrough is already on at postmaster start, but that GUC is PGC_SIGHUP while use_unified_arrow_exporter is PGC_POSTMASTER. After reload turns passthrough on with unified already enabled, PschExportBatch skips the legacy Arrow bypass yet still runs ExportEventStats on a plain OTelExporter, so batches go out as per-record OTLP instead of Arrow IPC.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit ae0334c. Configure here.
Comment on lines
+124
to
+142
| explicit ExtraAttrs(const char* raw) { | ||
| if (raw == nullptr) { | ||
| return; | ||
| } | ||
| std::string_view input(raw); | ||
| while (!input.empty()) { | ||
| const size_t delim = input.find(';'); | ||
| const std::string_view token = | ||
| (delim == std::string_view::npos) ? input : input.substr(0, delim); | ||
| const size_t sep = token.find(':'); | ||
| if (sep != std::string_view::npos) { | ||
| attrs_.emplace_back(std::string(token.substr(0, sep)), std::string(token.substr(sep + 1))); | ||
| } | ||
| if (delim == std::string_view::npos) { | ||
| break; | ||
| } | ||
| input.remove_prefix(delim + 1); | ||
| } | ||
| } |
Comment on lines
+368
to
+370
| "PGC_POSTMASTER: the bgworker picks the exporter implementation at init time, " | ||
| "so a runtime change would mismatch the per-cycle dispatcher (Arrow batches " | ||
| "would silently drop, since OTelArrowExporter does not implement SendArrowBatch).", |
Comment on lines
+19
to
+21
| # 6. Assert: row count matches what we sent, column types are what | ||
| # events_raw declares (no silent string-to-Int coercion), known queries | ||
| # land with the right db_name/db_operation/query_text values. |
Comment on lines
+44
to
+46
| # Verify the events_raw schema is present (applied by the goose migration | ||
| # step in CI). If we hit an empty CH or a different schema, fail loudly | ||
| # rather than silently inserting into nothing. |
…_block_bytes OTelArrowExporter accumulated rows into a single Arrow IPC up to psch_batch_max (default 200000) with no byte ceiling, so a queue backlog could produce a payload exceeding gRPC's 4 MiB wire cap or otelcol's HTTP body cap. Closes the gap relative to ExportEventsAsArrowInternal which flushes mid-batch when the builder's estimated bytes cross psch_otel_max_block_bytes. Column wrappers each take a pointer to bytes_estimate_ and bump it per Append (sizeof for fixed-width, value bytes + 4 for var-length offsets). BeginRow samples bytes_estimate_ at the row boundary; if it has crossed max_block_bytes_ and at least one row is already accumulated, Flush() runs, ships the chunk, and resets per-flush state. Arrow ArrayBuilder::Finish leaves builders empty and reusable, so subsequent chunks share the same slot vector + column shared_ptrs without any re-registration. NumExported now reports exported_in_batch_ (cumulative across all chunks in the active batch) instead of the per-chunk row_count_, otherwise mid-batch flushes would erase the dispatcher's view of work done. A sticky batch_failed_ flag poisons CommitBatch after any mid-batch Flush failure so partial batches never count as a success. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
serprex
approved these changes
Jun 19, 2026
The central OTel collector's routingconnector matches on the pg_stat_ch.block_format OTLP attribute to fan batches between the legacy receiver (writing query_logs_arrow) and a new receiver configured for events_raw. Producer-side migration is therefore a single-value swap rather than an endpoint reconfiguration — keeps the extension dumb about deployment topology and the routing decision in the central collector where it belongs. The change widens StatsExporter::SendArrowBatch with an explicit string_view block_format argument (no default — Google Style forbids default args on virtuals because they dispatch on static type, not dynamic). Threaded through PopulateResource and the per-record attribute setter in OTelExporter::SendArrowBatch. Two callers: - ExportEventsAsArrowInternal (legacy ArrowBatchBuilder path) passes "arrow_ipc" — preserves the existing wire-format marker so the legacy receiver continues to be the routing target. - OTelArrowExporter::Flush (unified path) passes "arrow_events_raw" — the distinct value the routingconnector will key on to dispatch to the events_raw receiver. Coordinated with collector values in clickgres-platform/services/datagres-otelcol (separate PR). No behavior change for the legacy path. No-op for any deployment whose collector doesn't have the routingconnector configured (the attribute is just additional metadata the receiver can ignore). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment on lines
+31
to
+38
| #include <cstdint> | ||
| #include <cstdio> | ||
| #include <memory> | ||
| #include <string> | ||
| #include <string_view> | ||
| #include <utility> | ||
| #include <vector> | ||
|
|
Comment on lines
+360
to
+375
| DefineCustomBoolVariable( | ||
| "pg_stat_ch.use_unified_arrow_exporter", | ||
| "Use the StatsExporter-implementing Arrow exporter instead of arrow_batch.cc.", | ||
| "When enabled together with use_otel and otel_arrow_passthrough, the bgworker " | ||
| "builds Arrow IPC via the typed StatsExporter column interface, writing the " | ||
| "events_raw schema (typed integer ids, no sprintf decimal-string encoding). " | ||
| "When off, the legacy arrow_batch.cc path runs and produces the query_logs_arrow " | ||
| "wire shape. Default off; flip on to opt a producer into the new exporter. " | ||
| "PGC_POSTMASTER: the bgworker picks the exporter implementation at init time, " | ||
| "so a runtime change would mismatch the per-cycle dispatcher (Arrow batches " | ||
| "would silently drop, since OTelArrowExporter does not implement SendArrowBatch).", | ||
| &psch_use_unified_arrow_exporter, | ||
| false, | ||
| PGC_POSTMASTER, | ||
| 0, | ||
| NULL, NULL, NULL); |
Comment on lines
+132
to
+141
| my $rc = system("curl -sS --fail-with-body -X POST " . | ||
| "-H 'Content-Type: application/octet-stream' " . | ||
| "--data-binary \@$f '$insert_url' 2>$err_file >/dev/null"); | ||
| my $stderr = -s $err_file ? do { | ||
| open my $fh, '<', $err_file; local $/; <$fh> | ||
| } : ''; | ||
| is($rc, 0, basename($f) . " accepted by CH ArrowStream parser" . | ||
| ($stderr ? " (stderr: $stderr)" : "")); | ||
| ++$total_posted; | ||
| } |
3 tasks
Comment on lines
+237
to
+242
| void Append(const T& v) final { | ||
| const arrow::Status s = builder_->Append(static_cast<typename BuilderT::value_type>(v)); | ||
| if (!s.ok()) { | ||
| LogArrowFailure("Arrow numeric append", s); | ||
| return; | ||
| } |
Comment on lines
+368
to
+370
| "PGC_POSTMASTER: the bgworker picks the exporter implementation at init time, " | ||
| "so a runtime change would mismatch the per-cycle dispatcher (Arrow batches " | ||
| "would silently drop, since OTelArrowExporter does not implement SendArrowBatch).", |
| svc_ver_->Append(service_version_val_); | ||
| host_id_->Append(host_id_val_); | ||
| pod_name_->Append(pod_name_val_); | ||
| } |
There was a problem hiding this comment.
Failed batch still appends columns
Medium Severity
When a mid-batch Flush fails, OTelArrowExporter sets batch_failed_ and BeginRow returns without bumping row_count_ or envelope fields, but ExportEventStatsInternal still appends the full per-event column set for every remaining event. Arrow builders then diverge in row counts and the dequeued events for that batch are dropped even though CommitBatch returns false.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit ffa9b91. Configure here.
| // values in clickgres-platform/services/datagres-otelcol. | ||
| if (!inner_->SendArrowBatch(buf->data(), buf_len, row_count_, "arrow_events_raw")) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Arrow build failures skip backoff
Medium Severity
When OTelArrowExporter::Flush fails during Arrow Finish, IPC serialization, or ZSTD setup, it only logs a warning and returns false. Unlike SendArrowBatch gRPC failures, these paths never call RecordExporterFailure, so NumConsecutiveFailures stays at zero and the bgworker does not apply export backoff while batches keep failing for encoding reasons.
Reviewed by Cursor Bugbot for commit ffa9b91. Configure here.
| return; | ||
| } | ||
| *bytes_ += sizeof(typename BuilderT::value_type); | ||
| } |
There was a problem hiding this comment.
Append errors desync row counts
Medium Severity
Arrow column wrappers log a failed Append and return without surfacing error state, while BeginRow still increments row_count_ and the export loop keeps filling other columns. Builder array lengths can diverge from row_count_, so RecordBatch::Make or IPC write may fail unpredictably or reject valid-looking batches.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit ffa9b91. Configure here.
Serp's interface refactor moved Crunch() and Clear() out of the
StatsExporter::Column<T> base into a CH-private ChColumn interface (only
ClickHouseExporter's columns implement it, via multiple inheritance with
Column<T>). OTelArrowExporter's nested column wrappers still declared
'void Crunch() final {}' as no-op overrides — with the virtual gone from
the base, 'final' now applies to a nothing, which is a compile error
(only virtual member functions can be marked 'final'). CI turned red
across the build matrix on the merge commit for this reason.
Fix: delete the five no-op Crunch() overrides from ArrowNumColumn,
ArrowDictStrColumn, ArrowDictSvColumn, ArrowUtf8SvColumn, and
ArrowTimestampColumn. Builder finish already lives centrally in
OTelArrowExporter::Flush() (invoked mid-batch on the block-budget
crossing and once at CommitBatch for the residual chunk); the column
wrappers only own Append, so having them empty-implement a nonexistent
virtual was never load-bearing.
The rest of the refactor is sound: ClickHouseExporter's FixedCol /
StringCol correctly multi-inherit from Column<T> + ChColumn; columns_
stores shared_ptr<ChColumn> upcasts safely; both bases have virtual
destructors so lifetime is correct regardless of which pointer holds
the last reference.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment on lines
+31
to
+37
| #include <cstdint> | ||
| #include <cstdio> | ||
| #include <memory> | ||
| #include <string> | ||
| #include <string_view> | ||
| #include <utility> | ||
| #include <vector> |
Comment on lines
+533
to
+535
| auto schema = arrow::schema(std::move(fields)); | ||
| auto record_batch = arrow::RecordBatch::Make(std::move(schema), row_count_, std::move(arrays)); | ||
|
|
Comment on lines
+132
to
+134
| my $rc = system("curl -sS --fail-with-body -X POST " . | ||
| "-H 'Content-Type: application/octet-stream' " . | ||
| "--data-binary \@$f '$insert_url' 2>$err_file >/dev/null"); |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 5 total unresolved issues (including 4 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8a0f60a. Configure here.
| // ExportEventStats. | ||
| if (psch_use_otel && psch_otel_arrow_passthrough && !psch_use_unified_arrow_exporter) { | ||
| elog(DEBUG1, "pg_stat_ch: exporting batch of %zu events as Arrow IPC (legacy)", | ||
| events.size()); |
There was a problem hiding this comment.
Partial export return without stats
Medium Severity
If the unified exporter succeeds on one or more mid-batch Flush calls then CommitBatch fails, PschExportBatch skips updating psch_shared_state->exported and PschRecordExportSuccess, yet still returns exporter->NumExported() reflecting rows from the successful flushes. The bgworker and operators can see a positive export count while shared export stats and success bookkeeping show a failed batch.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8a0f60a. Configure here.
JoshDreamland
added a commit
that referenced
this pull request
Jul 23, 2026
t/013_clickhouse_tls.pl's reconnect subtest polled for the resumed export within 30s of restarting the ClickHouse container. The exporter's reconnect backoff (stats_exporter.cc PschGetRetryDelayMs) is exponential — base 1s, doubling per consecutive failure, capped at 60s. Failed flush attempts start accumulating as soon as CH goes down, well before the container finishes restarting, and the cumulative wait before the Nth retry is the sum of all prior delays: 1, 3, 7, 15, 31, 63, 123s. A restart slow enough to rack up 5-6 failures — plausible under a loaded CI runner — pushes the bgworker's next attempt past the 30s mark even though ClickHouse itself came back much sooner. Observed exactly this on CI twice in a row while rebasing PR #117 onto post-#116 main: identical assertion, identical 'got 0' failure, while the same commit had passed cleanly on a less-loaded run days earlier. Not a regression — the backoff constants and this test's timeout have coexisted; it just needed a slow enough runner to surface. Bump to 90s, comfortably past the 6-failure (63s) cumulative-backoff case plus round-trip margin. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
JoshDreamland
added a commit
that referenced
this pull request
Jul 29, 2026
* test: producer-to-collector routing on pg_stat_ch.block_format Closes the producer-side contract gap: the unified Arrow exporter emits pg_stat_ch.block_format=arrow_events_raw on its OTLP envelope, while the legacy ArrowBatchBuilder path emits arrow_ipc. The central OTel collector's routingconnector is supposed to fan batches between the legacy query_logs_arrow target and the new events_raw target based on that marker. This test pins the producer half of the contract. New TAP test t/037_arrow_routing.pl exercises both arms: - arm 1 (unified=off, arrow_passthrough=on): legacy.jsonl grows, events_raw.jsonl and default.jsonl don't. - arm 2 (unified=on): events_raw.jsonl grows with arrow_events_raw in the routed payload, legacy.jsonl and default.jsonl don't. Stock otelcol-contrib image (matches existing t/024 setup, version 0.120.0). Routingconnector keyed on the resource attribute (where pg_stat_ch.block_format lives via PopulateResource), two file exporters writing JSONL to a bind-mounted dir the test reads from. No CH receiver — the events_raw column-set contract isn't validated here; that's covered by t/036 (producer Arrow IPC -> direct CH ingest) and by a separate receiver-faithful test in clickgres-platform. New helpers in t/psch.pm (psch_routing_collector_available, psch_start_routing_collector, psch_stop_routing_collector) live on a separate compose profile + container/ports so the new test can coexist with t/024's collector if both fire in the same suite. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: extend TLS reconnect timeout to cover worst-case backoff t/013_clickhouse_tls.pl's reconnect subtest polled for the resumed export within 30s of restarting the ClickHouse container. The exporter's reconnect backoff (stats_exporter.cc PschGetRetryDelayMs) is exponential — base 1s, doubling per consecutive failure, capped at 60s. Failed flush attempts start accumulating as soon as CH goes down, well before the container finishes restarting, and the cumulative wait before the Nth retry is the sum of all prior delays: 1, 3, 7, 15, 31, 63, 123s. A restart slow enough to rack up 5-6 failures — plausible under a loaded CI runner — pushes the bgworker's next attempt past the 30s mark even though ClickHouse itself came back much sooner. Observed exactly this on CI twice in a row while rebasing PR #117 onto post-#116 main: identical assertion, identical 'got 0' failure, while the same commit had passed cleanly on a less-loaded run days earlier. Not a regression — the backoff constants and this test's timeout have coexisted; it just needed a slow enough runner to surface. Bump to 90s, comfortably past the 6-failure (63s) cumulative-backoff case plus round-trip margin. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: fix collector lifecycle leaks and strengthen routing assertions Addresses review feedback (1 Copilot + 4 Cursor comments) on t/037_arrow_routing.pl, verified live via prove against a real build (not just code review): - Track whether this run started the routing collector ($started_routing_collector, set before the eval'd start attempt so a partial startup still counts). Only tear it down if so — a developer's manually-started collector, or one left behind by a crashed prior run, is left alone. - Move teardown into an END block (with 'local $?' to stop system() inside END from clobbering the script's exit code) so cleanup runs on every exit path: normal completion, plan skip_all, or a die() anywhere in the test body. Previously a die (e.g. the open-or-die below) skipped cleanup entirely, leaking the container and its fixed host ports (14317, 23133) -- ports t/026_arrow_dump.pl and t/036_unified_arrow_e2e.pl rely on being unbound. - Replace the bare 'open ... or die' on arm 2's spot-check with a Test::More SKIP block gated on whether the file actually grew, so a growth timeout produces a clean failed assertion instead of aborting the script mid-run. - Add per-arm producer-side bookkeeping (psch_wait_for_export + psch_get_stats asserting exported/send_failures), matching the existing t/024_otel_export.pl idiom, before the JSONL-growth checks. On failure this distinguishes 'producer never exported' from 'producer exported but routing/collector dropped it'. - Document the fixed-host-port single-instance-per-machine constraint in the header rather than parameterizing container/output naming: CI runs the TAP suite sequentially (prove without -j, single TAP job), and parameterizing names without also parameterizing the ports (14317/23133) wouldn't actually fix concurrent local runs while risking t/026 and t/036's assumption that those ports are unbound. Live-run verification surfaced a genuine pre-existing race unrelated to the above: the otelcol fileexporter writes a JSON line and its trailing newline as separate writes, and arm 1's node can flush trailing batches right up through stop(). If arm 2 snapshots its baselines mid-write, those late bytes get mis-attributed. Added wait_for_quiet() (polls combined JSONL size across all three files until stable for 1.5s, 15s cap) before arm 2's baselines to close the window. Confirmed via 3 live prove runs against a real build: one initial 11/12 run caught the race, then 12/12 with the collector auto-started (and torn down after), and 12/12 with the collector pre-started manually (and correctly left running after). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: diagnose why the routing collector fails healthcheck in CI t/037_arrow_routing.pl has skipped in every GitHub Actions CI run so far with only 'Routing collector container failed to become healthy' and no further detail. Locally, the exact same compose config starts and reports healthy in well under a second, and 'docker compose ... components' confirms the routing connector exists in this image version — ruling out a config bug or missing component. The failure appears specific to the GH-hosted runner environment, but there's been no way to tell why from the CI log alone. Dump 'docker compose ps' and 'docker compose logs' into the die message on healthcheck timeout, so the next occurrence (this one, on this push) actually carries diagnostic evidence instead of a bare 'failed to become healthy'. Also fixes a latent (currently harmless) bug found along the way: the service's Docker-level healthcheck used 'wget', which doesn't exist in the distroless otel/opentelemetry-collector-contrib image — it would always report unhealthy regardless of actual readiness. Doesn't affect today's behavior (psch_start_routing_collector polls host-side HTTP directly, independent of Docker's own healthcheck), but is misleading in 'docker compose ps' and would hang forever if anyone later adds '--wait' to the start command. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: route routing-collector diagnostics through warn, not die message The prior commit folded 'docker compose ps'/'docker compose logs' output into the die message on healthcheck timeout, but that die message becomes the reason string for t/037_arrow_routing.pl's 'plan skip_all' — and TAP's protocol doesn't tolerate embedded newlines there. Test::More silently dropped everything past the first line, so the CI run that should have shown the diagnostics showed nothing new at all (confirmed: this exact commit's SHA ran in CI, zero trace of the dump anywhere in the log). Emit the dump via warn (STDERR) immediately before the die instead, decoupled from the skip reason string entirely. prove -v interleaves STDERR with STDOUT per test file, so this should show up inline in the CI log this time. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: use Test::More::diag for routing-collector diagnostics, not warn warn (STDERR) didn't work either: confirmed empirically across two CI runs that plain STDERR output from a test ending in SKIP (rather than FAIL) never appears in prove -v's displayed output, even though the warn() call itself executes. diag() writes through Test::Builder's own output handle instead of raw STDERR, and prove -v documents showing diag() output regardless of the subtest's pass/fail/skip outcome. Verified locally with a throwaway script: a plan-skip_all test emitting a multi-line diag() shows every line, '#'-prefixed, under prove -v — exactly the behavior needed here. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: chmod routing collector output dir before starting the container Root cause found via the new Test::More::diag diagnostic: the otelcol-contrib container (runs as UID 10001 by default) crashed immediately on startup in every GitHub Actions CI run with error: open /output/legacy.jsonl: permission denied Error: cannot start pipelines: open /output/legacy.jsonl: permission denied On a fresh CI checkout, docker/otel-routing/output/ is owned by the runner account with default 0755 permissions. UID 10001 falls into the 'other' bucket, which gets r-x but not w on the bind-mounted directory, so the file exporter can't even create legacy.jsonl / events_raw.jsonl / default.jsonl. This is why t/037_arrow_routing.pl has skipped in literally every CI run since it was added: the collector never gets far enough to open its health-check port, so the 30x1s host-side curl poll always times out. Never reproduced locally: confirmed macOS's Docker Desktop/colima VM bind-mount layer doesn't enforce the same UID-based permission checks a real Linux host does (verified by force-running the container with --user 10001:10001 against a 0755-owned directory locally — started fine, unlike the real failure). The GH Actions runner is a real Linux host where these semantics apply for real. Fix: chmod the output dir to 0777 in psch_start_routing_collector right before 'docker compose up', rather than committing directory permission bits (which don't reliably survive git checkout across platforms/configs anyway). Self-healing every run; no risk since the directory holds only gitignored scratch test output. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: address remaining Copilot/Cursor findings on the routing test Three real, still-open findings from the review pass: 1. (Copilot) Header comment called the collector startup a compose 'profile' — it's a dedicated compose file, no --profile flag involved anywhere. Corrected the wording. 2. (Copilot + Cursor, duplicate findings) wait_for_quiet's return value was ignored at its one call site (settling between arm 1 and arm 2). If settling timed out, the test proceeded with unstable baselines instead of failing loudly. Wrapped in ok() so a timeout is now an explicit, attributable failure. 3. (Cursor) Isolation checks raced async writes: each arm calls wait_for_growth on its OWN expected file, then immediately snapshots the OTHER two files for the 'did NOT grow' assertions — with no settle time in between. A genuine misrouting bug whose erroneous write to another sink landed a moment late could race that snapshot and produce a false pass, defeating the entire point of the isolation check. Added a short wait_for_quiet on just the other two files (1s stable / 5s cap — narrower than the between-arms settle, since this is guarding against a fast erroneous write, not accommodating a slow legitimate one) before taking those snapshots, in both arms. Also fixed, Low severity (Cursor): arm 2's marker spot-check used 'open ... or die', so a transient reopen failure (the file's existence was already confirmed by the growth check, so this really would mean something changed underneath us) aborted the whole script instead of recording a failed assertion. Restructured as nested SKIP blocks — ok(open) now records pass/fail explicitly, and only the content checks that depend on a successful open get skipped if it fails. One further Copilot finding (t/psch.pm:315, healthcheck 30s vs compose's 50s) is now moot: the compose-level healthcheck it refers to was removed entirely in an earlier commit (wget doesn't exist in the distroless otelcol-contrib image, so it could never succeed regardless of timing) — replied and resolved on GitHub rather than included here since there's no code left for it to apply to. Syntax-checked (perl -c); could not get a live prove run in this local environment (pg_regress binary missing from a stale local install_tap tree, unrelated to this diff) — relying on CI, which has been the working verification loop all session for this file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Land the unified Arrow exporter behind a GUC, with an end-to-end TAP test that proves Arrow IPC bytes from pg_stat_ch can be ingested by ClickHouse against the
events_rawschema, and emit a distinctpg_stat_ch.block_formatdiscriminator on the OTLP envelope so the central OTel collector'sroutingconnectorcan fan unified-path batches to a receiver configured forevents_raw(separate from the legacyquery_logs_arrowpath).Four reviewable commits:
feat(exporter): add OTelArrowExporter implementing StatsExporter end-to-end— newsrc/export/otel_arrow_exporter.{cc,h}. Implements theStatLC*/StatHC*/StatTimestamp/Db*Columninterface with typed Arrow column wrappers. Composition: holds an innerOTelExporterfor gRPC transport (SendArrowBatch), doesn't extend it. Wire shape targetsevents_raw:query_id→Int64(no sprintf decimal-string)pid→Int32err_elevel→UInt8Int64parallel_workers_*→Int16jit_*→Int32DictionaryUtf8; HC strings → plainutf8ts→arrow::timestamp(MICRO, "UTC")matchingDateTime64(6, 'UTC')Synthesizes envelope columns (the 8
pg_stat_ch.extra_attributes-derived columns,read_replica_typedefaulting to'none',service_versionpinned toPG_STAT_CH_VERSION) sostats_exporter.cc's column-emission loop stays unchanged. Does not emitparent_query_id— that column isn't in the events_raw schema yet; PR feat: emit parent_query_id to link nested SPI queries #95 (still open) will add both the schema migration and the producer-side wiring together.feat(exporter): add use_unified_arrow_exporter GUC + dispatcher wiring— new bool GUCpg_stat_ch.use_unified_arrow_exporter(default off,PGC_POSTMASTER). When on withuse_otel+otel_arrow_passthrough,PschExporterInitconstructs the new exporter andPschExportBatchroutes throughExportEventStats(the typed interface) instead of the legacyExportEventsAsArrowbypass. When off, behavior is preserved bit-for-bit — legacyarrow_batch.ccpath, CH-native path, and OTel column-emission path all still reachable on their existing GUC combinations.test: end-to-end round-trip of the unified Arrow exporter into events_raw—t/036_unified_arrow_e2e.pl. Spins up a node withuse_unified_arrow_exporter=on, runs a deliberately-shaped workload (SELECT/CREATE/INSERT/SELECT count/DROP), waits for IPC files indebug_arrow_dump_dir, curls each file into CH asINSERT INTO pg_stat_ch.events_raw FORMAT ArrowStream, asserts:db_name/db_operation/query_textextra_attributesfeat(exporter): emit arrow_events_raw block_format on the unified path— the producer-side half of the migration. WidensStatsExporter::SendArrowBatch's signature with an explicitstring_view block_formatargument (no default — Google Style forbids defaults on virtuals). Threaded throughPopulateResourceand the per-record attribute setter. Two callers pass their intended marker:ExportEventsAsArrowInternal(legacy path):"arrow_ipc"(preserves existing wire-format marker).OTelArrowExporter::Flush(unified path):"arrow_events_raw".The central OTel collector's
routingconnectormatches on this attribute to fan batches between the legacyquery_logs_arrowreceiver target and a receiver configured forevents_raw. Producer-side migration is therefore a single-value swap rather than an endpoint reconfiguration — keeps the extension dumb about deployment topology and the routing decision in the central collector where it belongs.The "no OTel collector required" property of the e2e TAP test — bypassing the clickgres-platform Go receiver entirely via the
debug_arrow_dump_dir+ curl chain — makes this a pure producer⇄CH wire-format check. The collector-sideroutingconnectorconfig + the second receiver target are coordinated in a separateclickgres-platform/services/datagres-otelcolPR; an additional generic routing-validation test in this repo and a receiver-faithful end-to-end test inclickgres-platform(pg_stat_ch as submodule) are tracked follow-ups.Release intent
This is the first GUC-toggleable producer-side change of the unification effort. Worth tagging as a release: the GUC default is
off, so existing producers are unaffected, but it gives prod operators a knob to opt instances into the new exporter for the cutover fromquery_logs_arrow(the legacy receiver table) toevents_raw(PR #99's unified shape).Out of scope (future commits)
arrow_batch.cc,ExportEventsAsArrow, thepsch_otel_arrow_passthroughGUC, and theOTelExportercolumn-emission machinery (IntColumn/SvColumn/StrColumn/DateTimeCol/DurationCol+BeginBatch/BeginRow/FlushChunk/the chunk-state machinery) — all becomes dead code once the GUC default flips and the legacyquery_logs_arrowtable is retired. ~200 LOC saving.parent_query_idthrough to events_raw, both the schema-migration and producer side. PR feat: emit parent_query_id to link nested SPI queries #95 owns this end-to-end.routingconnectorconfig + second receiver target wiring — lives inclickgres-platform/services/datagres-otelcol.clickgres-platformwith pg_stat_ch as submodule) — separate follow-up PRs.Test plan
t/036_unified_arrow_e2e.pl(the new test)t/010(CH-native),t/013(TLS),t/024(OTel column-emission),t/026(Arrow dump shape) still pass — the GUC defaults off so none of those paths change behavior🤖 Generated with Claude Code