feat(observability): pluggable span exporter#64
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughMakes Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application (run)
participant Obs as Observability::init_observability
participant Log as Logger
participant Trace as Tracer/Exporter
participant Metric as MeterProvider
participant Shutdown as Shutdown Task
App->>Obs: call init_observability()
Obs->>Log: init logging (logforth / fastrace)
Obs->>Trace: init tracing (BoxedSpanExporter or default OTLP)
Obs->>Metric: init metrics (OTLP exporter, periodic reader)
Obs-->>App: return (oneshot::Sender, JoinHandle)
App->>Shutdown: send shutdown via oneshot
Shutdown->>Log: flush/exit logger
Shutdown->>Trace: force_flush/shutdown exporter
Shutdown->>Metric: stop meter provider
Shutdown-->>App: shutdown complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/mod.rs`:
- Line 4: Add a public doc comment for the new exported module by adding a ///
comment above pub mod observability; that briefly explains what the
observability module provides (e.g., logging/tracing/metrics helpers), any
important usage notes, and points to more detailed docs or examples in the
observability:: submodules; ensure the comment is concise and follows crate doc
style so the new public API is documented.
In `@src/utils/observability/mod.rs`:
- Around line 10-19: Add a Rust doc comment for the public trait DynSpanExporter
explaining its purpose in the public API and describing the behavior and intent
of its methods (export, shutdown_with_timeout, force_flush, set_resource); place
a /// doc block immediately above the trait declaration that summarizes what
implementations must do, notes the async export contract and error type
(OTelSdkResult), and briefly documents each method’s role so consumers
understand usage and guarantees.
- Around line 68-85: The test test_boxed_span_exporter merely constructs and
drops a BoxedSpanExporter; update it to assert behavior: for the Some case
assert the returned exporter is the same/wrapped instance (e.g., identity or
type) and for the None branch either assert the created BoxedSpanExporter is
functional by performing a simple export/flush round-trip or replace the real
exporter with a mock exporter that records calls and assert that export was
invoked; locate and modify the test_boxed_span_exporter function and usages of
BoxedSpanExporter::new/opentelemetry_otlp::SpanExporter to add these assertions
or inject a mock exporter to verify delegation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 26e928d2-48b6-4217-b2f7-a2c71882eb17
📒 Files selected for processing (3)
src/lib.rssrc/utils/mod.rssrc/utils/observability/mod.rs
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib.rs (1)
61-64:⚠️ Potential issue | 🟠 MajorEarly
?returns skip deterministic observability shutdown on error paths.At Line 61 and Line 88, errors return before the code path that sends shutdown and awaits the observability
JoinHandle. That can leave teardown/flush detached instead of awaited when startup fails.🔧 Suggested fix
pub async fn run_with_config( config: Arc<config::Config>, ob_shutdown_signal: oneshot::Sender<()>, ob_shutdown_task: tokio::task::JoinHandle<()>, ) -> Result<()> { - let config_provider = config::create_provider(&config) - .await - .context("failed to create config provider")?; + let config_provider = match config::create_provider(&config) + .await + .context("failed to create config provider") + { + Ok(provider) => provider, + Err(e) => { + let _ = ob_shutdown_signal.send(()); + let _ = ob_shutdown_task.await; + return Err(e); + } + }; run_with_provider( config, config_provider, ob_shutdown_signal, ob_shutdown_task, @@ pub async fn run_with_provider( @@ ) -> Result<()> { @@ - let gateway = Arc::new(gateway::Gateway::new( - gateway::providers::default_provider_registry() - .context("failed to build default gateway provider registry")?, - )); + let provider_registry = match gateway::providers::default_provider_registry() + .context("failed to build default gateway provider registry") + { + Ok(registry) => registry, + Err(e) => { + let _ = config_provider.shutdown().await; + let _ = ob_shutdown_signal.send(()); + let _ = ob_shutdown_task.await; + return Err(e); + } + }; + let gateway = Arc::new(gateway::Gateway::new(provider_registry));Also applies to: 86-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib.rs` around lines 61 - 64, Early `?` returns in the startup path (around create_provider and run_with_provider) can bypass sending the observability shutdown signal and awaiting the observability JoinHandle; change the error handling so that instead of using `?` directly on the results of config::create_provider and run_with_provider, capture their Result values, and on Err perform the observability shutdown sequence (send shutdown signal to the observability task and await its JoinHandle) before returning the error; locate the startup calls to config::create_provider and run_with_provider and implement this pattern (or introduce a small cleanup helper/guard invoked on error) so observability teardown always runs on all error paths.
🧹 Nitpick comments (1)
src/utils/observability/trace.rs (1)
45-47: Add a doc comment for the public constructor.
BoxedSpanExporter::newis public but undocumented.📝 Suggested fix
impl BoxedSpanExporter { + /// Wrap a concrete `SpanExporter` as a type-erased boxed exporter. pub fn new<T: SpanExporter + 'static>(span_exporter: T) -> Self { Self(Box::new(span_exporter)) } }As per coding guidelines, "Use /// for doc comments on public items in Rust".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/observability/trace.rs` around lines 45 - 47, Add a Rust doc comment (using ///) to the public constructor BoxedSpanExporter::new explaining its purpose and parameters/return (that it boxes a SpanExporter and returns a BoxedSpanExporter); place the /// comment immediately above the impl fn definition for pub fn new<T: SpanExporter + 'static>(span_exporter: T) -> Self so it documents the public API per project guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/observability/mod.rs`:
- Around line 110-112: The module-level doc for init_observability incorrectly
references a `span_exporter` argument (copied from init_observability_trace);
update the comment block to accurately describe init_observability's actual
parameters and behavior by removing or replacing the `span_exporter` sentence
and, if helpful, referencing the real options it accepts, making sure to remove
the misleading `span_exporter` mention and optionally point to
init_observability_trace for trace-specific details.
- Around line 28-39: The public functions shutdown_handler,
init_observability_log, init_observability_trace, init_observability_metric and
init_observability need the #[fastrace::trace] attribute applied; add the
attribute directly above each pub fn declaration (e.g., place #[fastrace::trace]
above pub fn shutdown_handler(...) and the other init_* functions) and ensure
the fastrace crate is available in the crate root so the attribute resolves (or
use the fully qualified attribute path #[fastrace::trace] as shown); no
behavioral changes are required beyond adding the attribute to each public
function.
- Around line 28-32: The compile error is caused by referencing Future in the
shutdown_handler signature without importing it; update the module to either add
use std::future::Future; at the top or qualify the bound as
std::future::Future<Output = ()> in the shutdown_handler declaration so the
trait is in scope for the F and Fut where-clauses.
In `@src/utils/observability/trace.rs`:
- Around line 24-37: The forwarding implementations on DynSpanExporter
incorrectly call Self::... which can cause ambiguous trait resolution; change
the delegations to use fully-qualified trait syntax SpanExporter::export,
SpanExporter::shutdown_with_timeout, SpanExporter::force_flush, and
SpanExporter::set_resource when calling through the inner exporter (i.e.,
replace Self::export(self, ...), Self::shutdown_with_timeout(self, ...),
Self::force_flush(self), and Self::set_resource(self, ...) with
SpanExporter::export(self, ...), SpanExporter::shutdown_with_timeout(self, ...),
SpanExporter::force_flush(self), and SpanExporter::set_resource(self, ...)
respectively) so the trait methods from SpanExporter are explicitly invoked on
the DynSpanExporter wrapper.
---
Outside diff comments:
In `@src/lib.rs`:
- Around line 61-64: Early `?` returns in the startup path (around
create_provider and run_with_provider) can bypass sending the observability
shutdown signal and awaiting the observability JoinHandle; change the error
handling so that instead of using `?` directly on the results of
config::create_provider and run_with_provider, capture their Result values, and
on Err perform the observability shutdown sequence (send shutdown signal to the
observability task and await its JoinHandle) before returning the error; locate
the startup calls to config::create_provider and run_with_provider and implement
this pattern (or introduce a small cleanup helper/guard invoked on error) so
observability teardown always runs on all error paths.
---
Nitpick comments:
In `@src/utils/observability/trace.rs`:
- Around line 45-47: Add a Rust doc comment (using ///) to the public
constructor BoxedSpanExporter::new explaining its purpose and parameters/return
(that it boxes a SpanExporter and returns a BoxedSpanExporter); place the ///
comment immediately above the impl fn definition for pub fn new<T: SpanExporter
+ 'static>(span_exporter: T) -> Self so it documents the public API per project
guidelines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 95a3fa7f-6f26-482c-9f9a-34c7ccc49e8e
📒 Files selected for processing (3)
src/lib.rssrc/utils/observability/mod.rssrc/utils/observability/trace.rs
This was referenced Apr 28, 2026
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
1 participant
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 by CodeRabbit
New Features
Refactor