[breaking] replace otlp-grpc with otlp-http - #121
Conversation
There was a problem hiding this comment.
Pull request overview
This PR switches pg_stat_ch’s OpenTelemetry export path from OTLP/gRPC to OTLP/HTTP to avoid the gRPC threadpool’s virtual-memory footprint, updating defaults and local test/docker wiring accordingly.
Changes:
- Replace OTLP gRPC exporter/client usage with OTLP HTTP client export calls in
otel_exporter.cc. - Update default
pg_stat_ch.otel_endpointto an OTLP/HTTP logs URL (http://localhost:4318/v1/logs) and adjust tests/docs accordingly. - Update local OTel collector/docker configuration and build dependencies to use OTLP/HTTP.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| vcpkg.json | Switch opentelemetry-cpp feature from otlp-grpc to otlp-http. |
| t/psch.pm | Update test helper default OTLP endpoint to OTLP/HTTP logs URL. |
| t/026_arrow_dump.pl | Update test config/comment to reflect HTTP export + URL-style endpoint. |
| t/025_otel_reconnect.pl | Update reconnect comment to reflect HTTP request behavior. |
| src/export/stats_exporter.cc | Update shutdown comment to reflect HTTP client teardown. |
| src/export/otel_exporter.cc | Replace gRPC channel/stub export with OtlpHttpClient-based export; validate URL scheme. |
| src/export/exporter_interface.h | Update include-conflict comment after gRPC header removal. |
| src/config/guc.c | Update GUC docs/defaults for OTLP/HTTP logs URL + wording updates. |
| docker/otel/collector-config.yaml | Switch collector OTLP receiver from gRPC:4317 to HTTP:4318. |
| docker/docker-compose.otel.yml | Update published port to 4318 for the HTTP receiver. |
| CMakeLists.txt | Replace gRPC exporter link targets with opentelemetry-cpp::otlp_http_log_record_exporter. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
avoids grpc threadpool eating up virtual memory
26ca808 to
385d944
Compare
| grpc_opts_.endpoint = endpoint; | ||
| http_opts_.url = endpoint; | ||
| } | ||
| grpc_opts_.timeout = std::chrono::milliseconds(psch_otel_log_delay_ms); |
There was a problem hiding this comment.
HTTP export lacks explicit gzip
Medium Severity
The gRPC path always enabled channel gzip for large OTLP log chunks, but ConfigureLogExport never sets http_opts_.compression when building the HTTP client. If SDK defaults leave compression off, multi‑MiB bodies can exceed the psch_otel_log_delay_ms HTTP timeout and fail exports that previously succeeded.
Reviewed by Cursor Bugbot for commit 385d944. Configure here.
| LogExporterWarning("OTel init failed", | ||
| "otel_endpoint must be an http:// or https:// URL, e.g. " | ||
| "http://localhost:4318/v1/logs"); |
| $node->safe_psql('postgres', 'SELECT pg_stat_ch_flush()'); | ||
|
|
||
| # Wait for export to resume; the OTel gRPC channel reconnects automatically | ||
| # Wait for export to resume; curl re-establishes the connection on the next request |
serprex
changed the title
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ 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 b63139a. Configure here.
| } | ||
|
|
||
| // Exception barrier: the OTel exporter's destructors (gRPC stub teardown, | ||
| // Exception barrier: the OTel exporter's destructors (HTTP client teardown, |
There was a problem hiding this comment.
Skipped CommitBatch leaves async uploads
High Severity
The OTLP HTTP exporter's asynchronous chunk uploads can complete after BeginBatch resets the async_ok_records_ and async_failed_chunks_ counters for a new batch. This allows late callbacks from prior batches to update the current batch's statistics, leading to inaccurate exported_count_ and potential stat bleeding across batches.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b63139a. Configure here.
| // In-flight chunk failed, stop building; CommitBatch reports failure | ||
| batch_failed_ = true; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Partial chunk dropped on failure
High Severity
With async chunk uploads, an HTTP failure on an earlier chunk can set batch_failed_ while a later chunk is only partly built. CommitBatch then skips the final FlushChunk, so those in-progress log records are never sent even though the bgworker already dequeued the events from the ring buffer.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b63139a. Configure here.
| #include <cstdlib> | ||
| #include <string> |
| if (batch_failed_) { | ||
| return; | ||
| } | ||
| if (async_failed_chunks_.load() > 0) { | ||
| // In-flight chunk failed, stop building; CommitBatch reports failure | ||
| batch_failed_ = true; | ||
| return; | ||
| } |
| [this, records](opentelemetry::sdk::common::ExportResult result) { | ||
| if (result == opentelemetry::sdk::common::ExportResult::kSuccess) { | ||
| async_ok_records_ += records; | ||
| } else { | ||
| async_failure_result_ = static_cast<int>(result); | ||
| ++async_failed_chunks_; | ||
| } | ||
| return true; | ||
| }, |
|
if we were to actually go with this, GUC would need to guide grpc/http decision |
avoids grpc threadpool eating up virtual memory