WIP: integrate OpenTelemetry SDK for metrics/logs export

[JoshDreamland]

No description provided.

[CLAassistant]

All committers have signed the CLA.

[iskakaushik]

why extern?

[Copilot]

Pull request overview

This PR introduces OpenTelemetry (OTel) exporter support alongside the existing ClickHouse exporter, refactoring the export architecture to use a common interface. However, the PR appears to be work-in-progress based on the title "Stash commit" and contains several critical bugs that would prevent it from compiling or functioning correctly.

Changes:

• Introduces a StatsExporter interface to abstract export backends
• Adds OpenTelemetry exporter implementation with metrics and logging support
• Refactors ClickHouse exporter to implement the new interface
• Adds GUC configuration variables for OTel endpoint and hostname selection

Reviewed changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated 24 comments.

Show a summary per file

File	Description
src/worker/bgworker.cc	Updates include paths from clickhouse_exporter.h to stats_exporter.h and updates comment references
src/export/stats_exporter.h	New header defining C API for stats exporters
src/export/stats_exporter.cc	New implementation coordinating between ClickHouse and OTel exporters based on configuration
src/export/otel_exporter.h	New header declaring OpenTelemetry exporter factory function
src/export/otel_exporter.cc	New OpenTelemetry exporter implementation with metrics histograms, counters, and log records
src/export/exporter_interface.h	New abstract interface defining the exporter contract
src/export/clickhouse_exporter.h	Refactored to declare factory function instead of C API
src/export/clickhouse_exporter.cc	Refactored into class implementing StatsExporter interface
src/config/guc.h	Adds declarations for use_otel, otel_endpoint, and hostname GUC variables
src/config/guc.cc	Implements new GUC variables for OTel configuration
.gitignore	Adds CMake build artifacts to ignore list

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Missing BeginBatch and BeginRow calls in export flow. The ExportEventStats function creates columns but never calls exporter->BeginBatch() before creating columns or exporter->BeginRow() for each event. The OTelExporter implementation expects BeginRow to be called to initialize row state (line 42-47 in otel_exporter.cc). Without these calls, the OTel exporter will not function correctly.

[Copilot]

Misleading error message. The error message says "Failed to connect to ClickHouse" but this code now supports both ClickHouse and OpenTelemetry exporters. The message should be generic, such as "Failed to connect to exporter backend" or dynamically include the backend type.

[Copilot]

Misleading log message. The shutdown message says "ClickHouse exporter shutdown" but the exporter could be either ClickHouse or OpenTelemetry. The message should be generic, such as "Statistics exporter shutdown".

Suggested change

	elog(LOG, "pg_stat_ch: ClickHouse exporter shutdown");
	elog(LOG, "pg_stat_ch: statistics exporter shutdown");

[Copilot]

Incorrect header guard at end. The closing header guard comment references BASIC_EXPORTER_H_ but should match the opening guard. It should be PG_STAT_CH_SRC_EXPORT_EXPORTER_INTERFACE_H_ to be consistent.

[Copilot]

Missing implementation for pure virtual destructors. The interface declares pure virtual destructors for BasicColumn (line 17), Column (line 22), and StatsExporter (line 53), but these destructors must have an implementation even though they're pure virtual. Without implementations, the code will fail to link. These should be defined in a source file, typically as StatsExporter::~StatsExporter() = default;

Suggested change

	inline StatsExporter::BasicColumn::~BasicColumn() = default;
	template<typename T>
	inline StatsExporter::Column<T>::~Column() = default;
	inline StatsExporter::~StatsExporter() = default;

[Copilot]

Misleading debug log message and comment. The log message says "BuildClickHouseBlock() called" and the comment on line 75 mentions "ClickHouse rows", but this function now works with both ClickHouse and OpenTelemetry exporters. The message and comment should be updated to be generic, such as "ExportEventStats() called" and "Build and export stats (records, metrics, backend rows) from events".

Suggested change

	// Build and export stats (records, metrics, ClickHouse rows) from events
	void ExportEventStats(const std::vector<PschEvent>& events, StatsExporter *exporter) {
	elog(DEBUG1, "pg_stat_ch: BuildClickHouseBlock() called with %zu events", events.size());
	// Build and export stats (records, metrics, backend rows) from events
	void ExportEventStats(const std::vector<PschEvent>& events, StatsExporter *exporter) {
	elog(DEBUG1, "pg_stat_ch: ExportEventStats() called with %zu events", events.size());

[Copilot]

Potential issue with signed/unsigned comparison. The code compares val < 0 where val is of template type T. This works for signed integers, but if T is an unsigned type (like uint8_t or uint64_t), the comparison will always be false and the cast to uint64_t at line 148 will be redundant. Consider using std::is_signed<T>::value to conditionally handle negative values only for signed types, or ensure this template is only instantiated for signed types.

[Copilot]

Potential dangling string_view in map. At line 117, when T is string or an integer type that gets converted via to_string(), the value is safely stored as a std::string in the map. However, the SetAttribute call at line 119 passes val directly. If T is string_view, this could lead to a dangling reference if the string_view's underlying data is temporary. Consider always converting to string before storing or using.

Suggested change

	exp->current_row_tags[name] = to_string(val);
	// Also put it in the log record so it's searchable!
	exp->current_log_record->SetAttribute(name, val);
	std::string &stored_value = exp->current_row_tags[name];
	stored_value = to_string(val);
	// Also put it in the log record so it's searchable!
	exp->current_log_record->SetAttribute(name, stored_value);

[Copilot]

Incorrect variable used in GUC definition. The definition for pg_stat_ch.hostname is assigning to &psch_otel_endpoint instead of &psch_hostname. This will cause the hostname configuration to overwrite the otel_endpoint configuration.

[iskakaushik]

Review Comments

Thanks for the WIP! The exporter abstraction is a nice design — having a single ExportEventStats() that works with both backends is clean. Here are some things I noticed:

---

1. psch_use_otel initializer vs boot value mismatch

src/config/guc.cc

The variable initializer (bool psch_use_otel = true) doesn't match the DefineCustomBoolVariable boot value (false). The boot value wins at runtime, but the inconsistency is confusing. Let's pick one and make them match — false seems right since ClickHouse is the default backend.

Also: the two extern keywords on the psch_otel_endpoint and psch_hostname definitions (lines 23-24) should be removed — they're definitions, not declarations.

---

2. Align OTel attribute names with semantic conventions

src/export/stats_exporter.cc

Let's align the OTel attribute names with the OpenTelemetry Database Semantic Conventions. For example:

• "db" → "db.name"
• "username" → "db.user"
• "query" → "db.statement"
• "cmd_type" → "db.operation.name"
• "duration_us" → "db.client.operation.duration" (consider converting to seconds as the convention expects)

For the ClickHouse backend these can stay as-is (they map to the events_raw table schema), so this mapping should happen in the OTel exporter's column implementations rather than in the shared ExportEventStats(). Maybe a name_override or a name-mapping hook in the exporter interface?

---

3. Signed metrics silently clamped to 0 in OTel histograms

src/export/otel_exporter.cc — HistogramColumn::Append

Many of the fields routed through MetricInt64 (e.g., shared_blks_hit, cpu_user_time_us, WAL counters) are legitimately signed in PostgreSQL — they just happen to be non-negative in practice. Silently clamping to 0 and logging a warning is lossy.

A cleaner approach: split the interface into MetricUInt64 (for genuinely unsigned values like rows, wal_bytes, duration_us) and MetricInt64 (for signed counters). The OTel exporter can map signed metrics to int64 histograms (OTel supports signed histogram values as of the latest spec), or record them as gauges instead of histograms. This avoids the cast-and-clamp entirely.

---

4. Crunch() contract is underspecified

src/export/exporter_interface.h

The Crunch() contract is underspecified — in the ClickHouse exporter it's called once per batch (in CommitBatch), but in the OTel exporter it's called once per row (in EndRow). This means the same interface method has fundamentally different call timing depending on the backend.

Suggestion: make the contract explicit. Either:

• Rename to something like FlushRow() / FinalizeBatch() with clear per-row vs per-batch semantics, or
• Document in the interface that Crunch() is called by the framework at a backend-defined time and implementers must handle both patterns

As it stands, a new exporter implementer would have to study both existing backends to understand when their Crunch() gets called.

---

5. cmd_type should be a TagString

src/export/stats_exporter.cc

cmd_type should be a TagString rather than RecordString — it's a natural grouping dimension. You'd want to see duration_us histograms broken down by SELECT vs INSERT vs UPDATE, and in OTel that means it needs to be in the tag set.

---

6. Column lifecycle vs BeginBatch() ordering

src/export/clickhouse_exporter.cc

BeginBatch() calls columns.clear(), which drops the exporter's shared_ptr references to all columns. If BeginBatch() is called after column creation (e.g., between batches), the caller's shared_ptrs keep the objects alive but CommitBatch() won't iterate them since it loops over columns.

The interface comment shows the correct call order (BeginBatch → create columns → loop events → CommitBatch), but the contract that columns must be created after BeginBatch() should be documented explicitly, since getting it wrong silently produces empty inserts.

---

7. Dequeued events lost on export failure — add a comment

src/export/stats_exporter.cc — PschExportBatch

When CommitBatch() fails, the dequeued events are silently lost — same as the old code. This is fine to keep the same semantics for now, but worth noting with a comment so it's a conscious decision:

// NOTE: Events are dequeued before export. On CommitBatch failure,
// these events are lost. This matches the pre-refactor behavior.

---

8. OTel instrument lifecycle on collector restart

src/export/otel_exporter.cc

The OTel instruments (histogram_cache, counter_cache) and providers live for the bgworker process lifetime. If the OTel collector restarts or becomes temporarily unavailable:

• Do the cached Histogram/Counter objects reconnect automatically via the gRPC channel?
• Does ForceFlush return false on transient errors, or does it throw?

Consider adding a ResetConnection() path (similar to how the ClickHouse exporter does client.reset() on failure) that tears down and recreates the providers. Without this, a collector restart could leave the bgworker in a permanently broken state.

---

9. Dual logs + metrics — cardinality and volume

src/export/otel_exporter.cc

Emitting both logs and metrics for every event is a valid design — logs give you full-fidelity per-query records for debugging, metrics give you pre-aggregated dashboards. Two things to watch:

1. Cardinality: Each unique tag combination creates a new time series. With db × username × cmd_type as tags, the series count scales with the product of unique values. Consider whether all current tags should remain tags or whether some should be log-only attributes.
2. Volume: At high QPS, emitting ~30 histogram observations + a full log record per query could overwhelm the collector. A pg_stat_ch.otel_enable_logs GUC to independently toggle the log pipeline would be a nice escape hatch.

---

10. OTel path has no test coverage

The OTel exporter path currently has no test coverage — all existing tests exercise the ClickHouse backend (psch_use_otel defaults to false). We should add OTel-specific tests before merging.

Suggested strategy:

• Unit/integration tests with an in-process mock: Use OpenTelemetry's InMemoryMetricExporter and InMemoryLogRecordExporter as backends instead of gRPC. This lets you assert on exact metric values and log attributes without any external dependencies.
• TAP test with a real collector: Add a Docker-based test (similar to how we test with ClickHouse) that spins up an OTel Collector, runs queries, and verifies data arrives. This can be optional / CI-only.
• Smoke test: At minimum, a TAP test that starts PG with pg_stat_ch.use_otel=on (pointing at a non-existent endpoint) and verifies the bgworker handles connection failures gracefully without crashing.

[Copilot]

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated 9 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

A pure virtual destructor must still have a definition, otherwise this will cause a link error when deleting through a StatsExporter*. Add an out-of-line definition in a .cc file or define it inline (e.g., inline StatsExporter::~StatsExporter() = default;).

[Copilot]

When CommitBatch() fails, PschExportBatch() still returns NumExported(), which can incorrectly report successful exports and interfere with retry/backoff behavior. Return 0 (or the batch count only on success), and also ensure failures are recorded consistently (e.g., call PschRecordExportFailure(...) on commit failure for exporters that don’t do it internally).

Suggested change

	if (psch_shared_state != nullptr) {
	pg_atomic_fetch_add_u64(&psch_shared_state->exported, exporter->NumExported());
	}
	PschRecordExportSuccess();
	}
	return exporter->NumExported();
	int num_exported = exporter->NumExported();
	if (psch_shared_state != nullptr) {
	pg_atomic_fetch_add_u64(&psch_shared_state->exported, num_exported);
	}
	PschRecordExportSuccess();
	return num_exported;
	} else {
	PschRecordExportFailure("Failed to commit exporter batch");
	return 0;
	}

[Copilot]

BeginBatch() clears columns before calling EndRow(), but EndRow() iterates columns to emit metrics. If a row is active, this would silently drop metric emission for that row. Call EndRow() before clearing state.

Suggested change

	columns.clear();
	if (row_active) EndRow(); // Just in case
	if (row_active) EndRow(); // Just in case
	columns.clear();

[Copilot]

exported_count appears to be cumulative across batches (it is never reset in BeginBatch()), which will over-report per-batch exports (and cause PschExportBatch() to return increasing counts). Reset exported_count at the start of each batch (and ensure the semantics match ClickHouse’s per-batch behavior).

[Copilot]

The comment still refers specifically to 'ClickHouse network I/O', but the exporter can now be OpenTelemetry as well. Consider rewording this to 'exporter network I/O' (or mention both backends) so the comment matches the new behavior.

[Copilot]

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The variable name pgch_version uses an abbreviation that doesn't match the project naming convention. Consider renaming to pg_stat_ch_version for consistency with other identifiers in the codebase.

[Copilot]

Setting export_interval_millis to max() creates a very large delay that may be surprising. Consider using a named constant like kManualFlushMode with a comment explaining that this effectively disables periodic exports in favor of manual flushing.