feat: add OTLP log exporter#333
Conversation
davidB
left a comment
There was a problem hiding this comment.
Thank you
Can you add, as an example, a test, or just documentation in the README of init-tracing-opentelemetry, or just the sample you included in the PR's description?
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces OTLP log exporting capabilities by adding a logs feature, implementing log provider initialization, and integrating a logging layer into the tracing subscriber. Review feedback identifies that the maybe_endpoint environment variable is currently ignored during exporter setup and that the logs_enabled configuration flag is not checked before adding the logging layer to the subscriber.
| let exporter: Option<LogExporter> = match protocol.as_deref() { | ||
| Some("http/protobuf") => Some(LogExporter::builder().with_http().build()?), | ||
| #[cfg(feature = "tls")] | ||
| Some("grpc/tls") => Some( | ||
| LogExporter::builder() | ||
| .with_tonic() | ||
| .with_tls_config(ClientTlsConfig::new().with_enabled_roots()) | ||
| .build()?, | ||
| ), | ||
| Some("grpc") => Some(LogExporter::builder().with_tonic().build()?), | ||
| Some(x) => { | ||
| tracing::warn!( | ||
| "unknown '{x}' env var set or infered for OTEL_EXPORTER_OTLP_LOGS_PROTOCOL or OTEL_EXPORTER_OTLP_PROTOCOL; no log exporter will be created" | ||
| ); | ||
| None | ||
| } | ||
| None => { | ||
| tracing::warn!( | ||
| "no env var set or infered for OTEL_EXPORTER_OTLP_LOGS_PROTOCOL or OTEL_EXPORTER_OTLP_PROTOCOL; no log exporter will be created" | ||
| ); | ||
| None | ||
| } | ||
| }; |
There was a problem hiding this comment.
The maybe_endpoint retrieved from environment variables is never used to configure the LogExporter. This means the exporter will always use default endpoints (e.g., http://localhost:4318/v1/logs for HTTP or http://localhost:4317 for gRPC), ignoring the OTEL_EXPORTER_OTLP_LOGS_ENDPOINT or OTEL_EXPORTER_OTLP_ENDPOINT settings.
let exporter: Option<LogExporter> = match protocol.as_deref() {
Some("http/protobuf") => {
let mut builder = LogExporter::builder().with_http();
if let Some(endpoint) = maybe_endpoint {
builder = builder.with_endpoint(endpoint);
}
Some(builder.build()?)
}
#[cfg(feature = "tls")]
Some("grpc/tls") => {
let mut builder = LogExporter::builder()
.with_tonic()
.with_tls_config(ClientTlsConfig::new().with_enabled_roots());
if let Some(endpoint) = maybe_endpoint {
builder = builder.with_endpoint(endpoint);
}
Some(builder.build()?)
}
Some("grpc") => {
let mut builder = LogExporter::builder().with_tonic();
if let Some(endpoint) = maybe_endpoint {
builder = builder.with_endpoint(endpoint);
}
Some(builder.build()?)
}
Some(x) => {
tracing::warn!(
"unknown '{x}' env var set or infered for OTEL_EXPORTER_OTLP_LOGS_PROTOCOL or OTEL_EXPORTER_OTLP_PROTOCOL; no log exporter will be created"
);
None
}
None => {
tracing::warn!(
"no env var set or infered for OTEL_EXPORTER_OTLP_LOGS_PROTOCOL or OTEL_EXPORTER_OTLP_PROTOCOL; no log exporter will be created"
);
None
}
};There was a problem hiding this comment.
Similar logic is used for tracing code, I pretty much copied the implementation from traces. Can fix that though.
| build_tracer_layer_with_resource_and_name(otel_rsrc, self.tracer_name.clone())?; | ||
| let subscriber = subscriber.with(trace_layer); | ||
| #[cfg(feature = "logs")] | ||
| let subscriber = subscriber.with(logs_layer); |
There was a problem hiding this comment.
The logs_enabled field in OtelConfig is currently ignored. The log layer is added to the subscriber whenever the logs feature is enabled, regardless of the runtime configuration. It should be conditionally added based on self.otel_config.logs_enabled.
| let subscriber = subscriber.with(logs_layer); | |
| let subscriber = subscriber.with(self.otel_config.logs_enabled.then_some(logs_layer)); |
There was a problem hiding this comment.
Same logic exists for metrics. Copied it from there. I can fix this one or both. Up to you.
Sure, will do when I'm back home later |
|
Sorry for the delay, I was nearly offline last week. Thank you for the correction. I'll take care of aligning the 3 implementations and the CI issue related to the dependency update. |
I have added a functional log exporter interface that more or less mirrors the setup of the tracing setup. I ran all required mise tasks and tested it in a short code example.
Traces are automatically attached to logs as well, so if the logs are queried in Grafana (for example), the trace automatically links to the log line.
Minimal code setup I used to test (running an LGTM stack in a Docker Compose with OTLP ports exposed):