Skip to content

Support multiple otlp providers #19

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ScriptSmith merged 2 commits into from
Mar 22, 2026
+54 −20
Merged

Support multiple otlp providers #19

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b2b2757
Support multiple otlp providers
ScriptSmith Mar 22, 2026
e5a186f
Review fix
ScriptSmith Mar 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions src/cli/server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,25 +296,26 @@ pub(crate) async fn run_server(explicit_config_path: Option<&str>, no_browser: b
tracing::info!("Usage logging to database enabled");
}

// Add OTLP sink if configured
// Add OTLP sinks if configured
#[cfg(feature = "otlp")]
if let Some(otlp_config) = &config.observability.usage.otlp
&& otlp_config.enabled
{
use usage_sink::UsageSink as _;
Comment on lines 300 to +301
Copy link
Contributor

@greptile-apps greptile-apps bot Mar 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Trait import placed mid-function

Placing a use statement inside the body of a function (rather than at the top of the module or at the start of the enclosing block) is unusual in Rust and can be surprising to readers. The trait import is needed so that otlp_sink.name() resolves, but a cleaner place would be at the top of the module alongside the other #[cfg(feature = "otlp")] imports (e.g. near where crate::config::UsageOtlpConfig and TracingConfig are imported in usage_sink.rs), or at the very start of the if let Some(buffer) block.

Suggested change
#[cfg(feature = "otlp")]
if let Some(otlp_config) = &config.observability.usage.otlp
&& otlp_config.enabled
{
use usage_sink::UsageSink as _;
#[cfg(feature = "otlp")]
{
use usage_sink::UsageSink as _;
for otlp_config in &config.observability.usage.otlp {

Alternatively, expose name() as an inherent method on OtlpSink (in addition to the trait impl) so no trait import is needed at the call site.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/cli/server.rs
Line: 300-301

Comment:
**Trait import placed mid-function**

Placing a `use` statement inside the body of a function (rather than at the top of the module or at the start of the enclosing block) is unusual in Rust and can be surprising to readers. The trait import is needed so that `otlp_sink.name()` resolves, but a cleaner place would be at the top of the module alongside the other `#[cfg(feature = "otlp")]` imports (e.g. near where `crate::config::UsageOtlpConfig` and `TracingConfig` are imported in `usage_sink.rs`), or at the very start of the `if let Some(buffer)` block.

```suggestion
        #[cfg(feature = "otlp")]
        {
            use usage_sink::UsageSink as _;
            for otlp_config in &config.observability.usage.otlp {
```

Alternatively, expose `name()` as an inherent method on `OtlpSink` (in addition to the trait impl) so no trait import is needed at the call site.

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

#[cfg(feature = "otlp")]
for otlp_config in &config.observability.usage.otlp {
if !otlp_config.enabled {
continue;
}
match usage_sink::OtlpSink::new(otlp_config, &config.observability.tracing) {
Ok(otlp_sink) => {
tracing::info!(name = otlp_sink.name(), "Usage logging to OTLP enabled");
sinks.push(Arc::new(otlp_sink));
tracing::info!("Usage logging to OTLP enabled");
}
Err(e) => {
tracing::error!(error = %e, "Failed to initialize OTLP usage sink");
}
}
}
#[cfg(not(feature = "otlp"))]
if let Some(otlp_config) = &config.observability.usage.otlp
&& otlp_config.enabled
{
if config.observability.usage.otlp.iter().any(|c| c.enabled) {
tracing::warn!(
"OTLP usage sink is enabled in config but the 'otlp' feature is not compiled. \
Rebuild with: cargo build --features otlp"
Expand Down
26 changes: 22 additions & 4 deletions src/config/observability.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -763,10 +763,23 @@ pub struct UsageConfig {
#[serde(default = "default_true")]
pub database: bool,

/// OTLP exporter for usage data.
/// Sends usage records as OTLP log records to any OpenTelemetry-compatible backend.
/// OTLP exporters for usage data.
/// Sends usage records as OTLP log records to one or more OpenTelemetry-compatible backends.
/// Each entry creates an independent exporter, allowing fan-out to multiple destinations.
///
/// ```toml
/// [[observability.usage.otlp]]
/// name = "grafana"
/// endpoint = "https://otlp-gateway.grafana.net/otlp"
/// headers = { Authorization = "Basic xxx" }
///
/// [[observability.usage.otlp]]
/// name = "datadog"
/// endpoint = "https://otel.datadoghq.com"
/// headers = { "DD-API-KEY" = "xxx" }
/// ```
#[serde(default)]
pub otlp: Option<UsageOtlpConfig>,
pub otlp: Vec<UsageOtlpConfig>,
Copy link
Contributor

@greptile-apps greptile-apps bot Mar 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Breaking config change — no migration path

Changing otlp from Option<UsageOtlpConfig> to Vec<UsageOtlpConfig> alters the expected TOML syntax. Any existing deployment that has:

[observability.usage.otlp]
enabled = true
endpoint = "http://localhost:4317"

will now fail to deserialize at startup with a type mismatch, because a TOML inline table ([section]) cannot be coerced into a Vec — the new syntax requires:

[[observability.usage.otlp]]
enabled = true
endpoint = "http://localhost:4317"

Since serde will return an error rather than silently ignoring the old key, this is a hard startup failure for anyone who already has OTLP configured. Consider adding a note in the changelog/PR description, and ideally a custom deserializer or a serde alias / migration note so operators aren't surprised.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/config/observability.rs
Line: 782

Comment:
**Breaking config change — no migration path**

Changing `otlp` from `Option<UsageOtlpConfig>` to `Vec<UsageOtlpConfig>` alters the expected TOML syntax. Any existing deployment that has:

```toml
[observability.usage.otlp]
enabled = true
endpoint = "http://localhost:4317"
```

will now fail to deserialize at startup with a type mismatch, because a TOML inline table (`[section]`) cannot be coerced into a `Vec` — the new syntax requires:

```toml
[[observability.usage.otlp]]
enabled = true
endpoint = "http://localhost:4317"
```

Since `serde` will return an error rather than silently ignoring the old key, this is a hard startup failure for anyone who already has OTLP configured. Consider adding a note in the changelog/PR description, and ideally a custom deserializer or a serde `alias` / migration note so operators aren't surprised.

How can I resolve this? If you propose a fix, please make it concise.


/// Buffer configuration for batched writes.
#[serde(default)]
Expand All @@ -777,7 +790,7 @@ impl Default for UsageConfig {
fn default() -> Self {
Self {
database: true,
otlp: None,
otlp: Vec::new(),
buffer: UsageBufferConfig::default(),
}
}
Expand All @@ -792,6 +805,11 @@ pub struct UsageOtlpConfig {
#[serde(default = "default_true")]
pub enabled: bool,

/// Human-readable name for this endpoint (used in logs/metrics).
/// Defaults to the endpoint URL if not specified.
#[serde(default)]
pub name: Option<String>,

/// OTLP endpoint URL.
/// If not specified, uses the tracing OTLP endpoint.
#[serde(default)]
Expand Down
31 changes: 23 additions & 8 deletions src/usage_sink.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,16 @@
//! [observability.usage]
//! database = true # Enable database logging (default)
//!
//! [observability.usage.otlp]
//! enabled = true
//! endpoint = "http://localhost:4317" # or inherit from tracing.otlp
//! # Fan out to multiple OTLP endpoints:
//! [[observability.usage.otlp]]
//! name = "grafana"
//! endpoint = "https://otlp-gateway.grafana.net/otlp"
//! headers = { Authorization = "Basic xxx" }
//!
//! [[observability.usage.otlp]]
//! name = "datadog"
//! endpoint = "https://otel.datadoghq.com"
//! headers = { "DD-API-KEY" = "xxx" }
//! ```

use std::sync::Arc;
Expand Down Expand Up @@ -48,7 +55,7 @@ pub trait UsageSink: Send + Sync {
async fn write_batch(&self, entries: &[UsageLogEntry]) -> Result<usize, UsageSinkError>;

/// Get the sink name for logging/metrics.
fn name(&self) -> &'static str;
fn name(&self) -> &str;
}

/// Errors from usage sinks.
Expand Down Expand Up @@ -138,7 +145,7 @@ impl UsageSink for DatabaseSink {
}
}

fn name(&self) -> &'static str {
fn name(&self) -> &str {
"database"
}
}
Expand All @@ -160,6 +167,7 @@ impl UsageSink for DatabaseSink {
/// Requires the `otlp` feature.
#[cfg(feature = "otlp")]
pub struct OtlpSink {
name: String,
logger_provider: opentelemetry_sdk::logs::SdkLoggerProvider,
logger: opentelemetry_sdk::logs::SdkLogger,
}
Expand Down Expand Up @@ -203,7 +211,14 @@ impl OtlpSink {

let logger = provider.logger("hadrian.usage");

let name = config
.name
.clone()
.or_else(|| config.endpoint.clone())
.unwrap_or_else(|| "otlp".to_string());

Ok(Self {
name,
logger_provider: provider,
logger,
})
Expand Down Expand Up @@ -484,8 +499,8 @@ impl UsageSink for OtlpSink {
Ok(success_count)
}

fn name(&self) -> &'static str {
"otlp"
fn name(&self) -> &str {
&self.name
}
}

Expand Down Expand Up @@ -554,7 +569,7 @@ impl UsageSink for CompositeSink {
}
}

fn name(&self) -> &'static str {
fn name(&self) -> &str {
"composite"
}
}
Expand Down
Loading