feat: allow OTLP exporting by extending existing logger configs#835
feat: allow OTLP exporting by extending existing logger configs#835ZanCorDX merged 4 commits intoflashbots:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds OTLP (OpenTelemetry Protocol) distributed tracing support by extending the existing logger configuration infrastructure. The changes introduce OtlpConfig and TracingConfig types that wrap the existing LoggerConfig, allowing applications to optionally enable OTLP exporting alongside local logging.
Key Changes:
- New
OtlpConfigandTracingConfigtypes for unified logging and tracing configuration - Updated initialization code across multiple binaries to use the new tracing configuration
- Added OTLP dependencies to the rbuilder-config crate
Reviewed changes
Copilot reviewed 9 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/rbuilder-config/src/otlp.rs | Implements OTLP configuration and guard for managing the tracer provider lifecycle |
| crates/rbuilder-config/src/tracing.rs | Defines unified tracing configuration that combines logging and OTLP |
| crates/rbuilder-config/src/logger.rs | Refactors logger initialization and deprecates direct init_tracing method |
| crates/rbuilder-config/src/lib.rs | Exports new OTLP and tracing modules |
| crates/rbuilder-config/Cargo.toml | Adds OpenTelemetry dependencies |
| crates/rbuilder/src/live_builder/base_config.rs | Updates to use TracingConfig with optional OTLP support |
| crates/test-relay/src/main.rs | Migrates CLI to use TracingConfig and adds OTLP environment name option |
| crates/rbuilder-rebalancer/src/config.rs | Renames logger field to tracing and updates to use TracingConfig |
| crates/bid-scraper/src/config.rs | Adds otlp_env_name field to configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// # Panics | ||
| /// | ||
| /// If `self.log_json` is true. | ||
| pub(crate) fn ansi(&self) -> eyre::Result<SubscriberBuilder<DefaultFields, Format, EnvFilter>> { |
There was a problem hiding this comment.
The return type 'Format' is ambiguous and should be fully qualified as 'Format<Full, SystemTime>' to match the pattern used in the 'builder()' method.
| pub(crate) fn ansi(&self) -> eyre::Result<SubscriberBuilder<DefaultFields, Format, EnvFilter>> { | |
| pub(crate) fn ansi(&self) -> eyre::Result<SubscriberBuilder<DefaultFields, Format<Full, SystemTime>, EnvFilter>> { |
There was a problem hiding this comment.
i go the other way. including default type args you don't have to is overly verbose
| $registry.with(fmt).try_init() | ||
| }}; | ||
| (log @ $registry:ident, $filter:ident) => {{ | ||
| let fmt = tracing_subscriber::fmt::layer().with_filter($filter); |
There was a problem hiding this comment.
did you forget the ansi() ?
There was a problem hiding this comment.
ya, it needs to be propagated through.
incidentally, the preferred config method for ANSI is NO_COLOR. As documented here, ansi is enabled by default when the crate "ansi" feature is enabled. If this is set to true and the flag is NOT enabled, then with_ansi panics
this is a roundabout way of saying that probably log_color should be ignored configs, and the recommendation to use NO_COLOR should be documented
18c0421 to
9ddf802
Compare
| let exporter = opentelemetry_otlp::SpanExporter::builder() | ||
| .with_http() | ||
| .build() | ||
| .unwrap(); |
There was a problem hiding this comment.
The unwrap() call here will panic if the OTLP exporter fails to build. This could happen for various reasons such as invalid configuration or network issues. Consider returning a Result from the provider() method and propagating the error, or at minimum provide a more descriptive error message explaining why the exporter failed to build.
| .unwrap(); | |
| .expect("failed to build OTLP span exporter"); |
📝 Summary
Add simple OTLP exporting by extending existing logger configs.
The goal is to allow rbuilder to export spans, events, and other trace information to tools like zipkin or groundcover
💡 Motivation and Context
Distributed tracing is important for production systems :)
✅ I have completed the following steps:
make lintmake test