fix(core): keep Tokio runtime alive for OTel batch exporter - #982
Conversation
225596d to
a7c634b
Compare
a7c634b to
634c41b
Compare
The default BatchSpanProcessor drives export calls via futures_executor::block_on, which cannot poll Tokio I/O. Keep a persistent multi-thread runtime in TracingGuard for tonic's transport. Signed-off-by: Ladislav Smola <lsmola@redhat.com>
Adds an integration test that starts a real gRPC collector (using opentelemetry-proto TraceService), configures the proxy to export spans to it, and asserts that spans arrive. This exercises the full export pipeline: TracingGuard keeps the Tokio runtime alive, BatchSpanProcessor flushes, tonic gRPC transport delivers. Signed-off-by: Ladislav Smola <lsmola@redhat.com>
Mirrors the experimental-features matrix from tests.yaml but for integration test crates. Runs cargo-hack --each-feature on praxis-tests-integration, covering otel, cpex-policy-engine, and basic-auth-filter feature gates that were previously untested in CI. Excludes no-mac-cert-rotation-tests (platform flag, not a feature). Signed-off-by: Ladislav Smola <lsmola@redhat.com>
634c41b to
e65a9a6
Compare
praxis-bot
left a comment
There was a problem hiding this comment.
PR Review: fix(core): keep Tokio runtime alive for OTel batch exporter
Fixes silent span loss caused by the default BatchSpanProcessor using futures_executor::block_on, which cannot drive Tokio I/O primitives needed by tonic's gRPC transport. The fix creates a dedicated multi-thread Tokio runtime (1 worker) that stays alive in TracingGuard for both the initial channel build and all subsequent batch exports.
Overall Assessment
The core fix is sound: creating the exporter inside runtime.block_on() binds the tonic channel to that runtime's I/O driver, and storing the runtime in TracingGuard keeps it alive. Drop ordering is correct -- the explicit Drop impl shuts down the provider (flushing spans) while the runtime is still alive, then field destructors clean up. The new integration test with a fake gRPC collector is a valuable addition.
Four items to address, all medium severity.
Findings
| # | Severity | File | Finding |
|---|---|---|---|
| 1 | Medium | tests/integration/Cargo.toml |
opentelemetry-proto should be a workspace dependency |
| 2 | Medium | core/src/logging.rs |
Unit test should also verify the runtime element is None |
| 3 | Medium | tests/integration/tests/suite/examples/tracing_otlp.rs |
Inline comment in test body violates convention |
| 4 | Medium | tests/integration/tests/suite/examples/tracing_otlp.rs |
Fixed sleep for flush wait; poll loop would be more robust |
| tokio-rustls = { workspace = true } | ||
| tokio-stream = { workspace = true } | ||
| tokio-tungstenite = { workspace = true } | ||
| opentelemetry-proto = { version = "0.32.0", default-features = false, features = ["gen-tonic", "trace"] } |
There was a problem hiding this comment.
[Medium] Project convention: "Prefer workspace dependencies to keep versions consistent across crates." All other opentelemetry* crates are declared in the root [workspace.dependencies]. Add opentelemetry-proto there and reference it here as { workspace = true, default-features = false, features = ["gen-tonic", "trace"] }.
| let provider = build_otel_provider(&config).expect("should succeed with no endpoint"); | ||
| assert!( | ||
| provider.is_none(), | ||
| provider.0.is_none(), |
There was a problem hiding this comment.
[Medium] build_otel_provider now returns a tuple. This assertion verifies only the provider (.0) is None, but should also verify the runtime (.1) is None when no endpoint is configured:
assert!(
provider.0.is_none() && provider.1.is_none(),
"both provider and runtime should be None when no endpoint configured"
);| let (status, _) = http_get(proxy.addr(), "/", None); | ||
| assert_eq!(status, 200); | ||
|
|
||
| // Wait for the batch exporter to flush (interval=1s, add margin). |
There was a problem hiding this comment.
[Medium] Convention: "Never use inline comments inside test function bodies. All explanatory text must be either an assertion message or a tracing::info! / debug! / trace! call." Remove this comment and move the intent into the assertion message on line 117 if needed.
| assert_eq!(status, 200); | ||
|
|
||
| // Wait for the batch exporter to flush (interval=1s, add margin). | ||
| std::thread::sleep(std::time::Duration::from_secs(3)); |
There was a problem hiding this comment.
[Medium] A fixed 3-second sleep can be fragile on slow CI runners. Consider a poll loop with a timeout instead:
let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10);
while span_count.load(Ordering::Relaxed) == 0 {
assert!(
std::time::Instant::now() < deadline,
"timed out waiting for spans to arrive at fake collector"
);
std::thread::sleep(std::time::Duration::from_millis(100));
}This is both faster in the happy path (returns as soon as spans arrive) and more tolerant of slow environments.
|
Thank you for your contribution! This has been merged into |
Problem
The default
BatchSpanProcessorin opentelemetry_sdk 0.32 drives exportcalls via
futures_executor::block_on, which cannot poll Tokio I/Oprimitives. The tonic gRPC transport requires a Tokio reactor for its
Hyper-based HTTP/2 connections. Without a live Tokio runtime, traces are
silently dropped — the exporter builds successfully but every batch
export fails at the network layer.
Fix
Create a persistent multi-thread Tokio runtime (1 worker thread) stored
in
TracingGuardthat stays alive for both the initial tonic channelbuild and all subsequent batch export calls. The runtime is dropped on
graceful shutdown alongside the tracer provider.
Testing
Verified end-to-end on KIND with OTel Collector + Tempo:
Related: praxis-proxy/forge#2 (OTel benchmark demo)