-
Notifications
You must be signed in to change notification settings - Fork 592
Add declarative configuration initial version #3226
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.
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
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3226 +/- ##
=======================================
+ Coverage 80.6% 80.8% +0.2%
=======================================
Files 128 138 +10
Lines 23198 23852 +654
=======================================
+ Hits 18719 19295 +576
- Misses 4479 4557 +78 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new opentelemetry-declarative-config crate that provides declarative YAML-based configuration for OpenTelemetry SDKs. The implementation supports configuring metrics and logs telemetry with various exporters (OTLP, stdout, and Prometheus).
Key Changes:
- New declarative configuration crate with YAML-based setup for OpenTelemetry telemetry
- Support for metrics readers (periodic and pull) and logs processors (batch)
- Factory pattern implementation for creating exporters (OTLP, stdout, Prometheus)
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| opentelemetry-declarative-config/src/lib.rs | Main configurator implementation with telemetry provider management |
| opentelemetry-declarative-config/src/telemetry_config.rs | Core telemetry configuration structure with YAML parsing support |
| opentelemetry-declarative-config/src/metrics.rs | Metrics configuration with factory enums for periodic and pull exporters |
| opentelemetry-declarative-config/src/logs.rs | Logs configuration with batch exporter factory implementation |
| opentelemetry-declarative-config/src/metrics/reader_config.rs | Configuration structures for metrics readers |
| opentelemetry-declarative-config/src/logs/processor_config.rs | Configuration structures for log processors |
| opentelemetry-declarative-config/src/metrics/exporters.rs | Shared utilities for metrics exporters including temporality deserialization |
| opentelemetry-declarative-config/src/logs/exporters.rs | Module declaration for logs exporters |
| opentelemetry-declarative-config/src/metrics/exporters/otlp_periodic_exporter.rs | OTLP metrics periodic exporter factory with protocol configuration |
| opentelemetry-declarative-config/src/metrics/exporters/stdout_periodic_exporter.rs | Stdout metrics periodic exporter factory with temporality configuration |
| opentelemetry-declarative-config/src/metrics/exporters/prometheus_pull_exporter.rs | Mock Prometheus pull exporter (currently not maintained) |
| opentelemetry-declarative-config/src/logs/exporters/otlp_batch_exporter.rs | OTLP logs batch exporter factory (configuration parsing incomplete) |
| opentelemetry-declarative-config/src/logs/exporters/stdout_batch_exporter.rs | Stdout logs batch exporter factory |
| opentelemetry-declarative-config/tests/telemetry_config_tests.rs | Tests for telemetry configuration parsing from YAML |
| opentelemetry-declarative-config/tests/configurator_tests.rs | Integration tests for configurator with various scenarios |
| opentelemetry-declarative-config/tests/sample1.yaml | Sample YAML configuration file for testing |
| opentelemetry-declarative-config/tests/non_maintained_exporter.yaml | Test file for non-maintained Prometheus exporter |
| opentelemetry-declarative-config/tests/extra_field.yaml | Test file for extra field validation |
| opentelemetry-declarative-config/examples/declarative_basic.rs | Example demonstrating basic declarative configuration usage |
| opentelemetry-declarative-config/Cargo.toml | Package configuration with dependencies and features |
| opentelemetry-declarative-config/README.md | Package documentation and overview |
| opentelemetry-declarative-config/CHANGELOG.md | Initial changelog for v0.1.0 |
| Cargo.toml | Workspace member addition for the new crate |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
opentelemetry-declarative-config/src/logs/exporters/otlp_batch_exporter.rs
Outdated
Show resolved
Hide resolved
opentelemetry-declarative-config/tests/telemetry_config_tests.rs
Outdated
Show resolved
Hide resolved
opentelemetry-declarative-config/src/logs/exporters/otlp_batch_exporter.rs
Outdated
Show resolved
Hide resolved
opentelemetry-declarative-config/src/logs/exporters/otlp_batch_exporter.rs
Outdated
Show resolved
Hide resolved
opentelemetry-declarative-config/src/logs/exporters/otlp_batch_exporter.rs
Outdated
Show resolved
Hide resolved
a70baa9 to
a142280
Compare
|
Thanks @andborja for the PR - great to see contribution coming in for the opentelemetry-configuration implementation. This isn’t specific to the PR content itself, but more of a broader question with other maintainers/approvers - The OTel Configuration spec is currently at v1.0.0-rc.2 (stable target seems to be March 2026). Should this implementation start in |
@lalitb That's a good point. I think it's better to have it in the contrib repo. Apart from readiness/stability, I also think the release cadence for this crate would not align with the rest of the core components. It looks like Go SIG also put this in their contrib repo: https://github.com/open-telemetry/opentelemetry-go-contrib/tree/main/otelconf |
|
|
||
| Declarative configuration for applications instrumented with [`OpenTelemetry`]. | ||
|
|
||
| ## OpenTelemetry Overview |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This README should talk about OpenTelemetry Declarative Configuration and not general OpenTelemetry overview.
| opentelemetry-http = { workspace = true, optional = true, default-features = false } | ||
| serde = { workspace = true, features = ["derive"] } | ||
| reqwest = { workspace = true, optional = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think opentelemetry-http and reqwest need to be added as direct dependencies. You already have opentelemetry-otlp dependency which should transitively pull these two dependencies.
Once you remove these two, you can also remove the cargo-machete section as well.
| /// Example of configuring OpenTelemetry telemetry using declarative YAML configuration. | ||
|
|
||
| #[tokio::main] | ||
| pub async fn main() -> Result<(), Box<dyn std::error::Error>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this a regular main method. We don't need this to be async.
- That would clearly convey that this crate does not have any asynchronous APIs
- You can also remove the dependency on
tokio.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the OTLP implementation requires it. Let's chat on how to avoid it.
| let config_yaml = r#" | ||
| metrics: | ||
| readers: | ||
| - periodic: | ||
| exporter: | ||
| otlp: | ||
| protocol: http/protobuf | ||
| endpoint: https://backend:4318 | ||
| stdout: | ||
| temporality: cumulative | ||
| logs: | ||
| processors: | ||
| - batch: | ||
| exporter: | ||
| stdout: | ||
| otlp: | ||
| protocol: http/protobuf | ||
| endpoint: https://backend:4318 | ||
| resource: | ||
| service.name: sample-service | ||
| service.version: "1.0.0" | ||
| "#; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we move this to an actual .yaml instead of using raw string literal? That would make the example more helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this will be embedded most likely in other configurations, the most common use case is actually a string or the object composition itself. I'll add another one with a file for more coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The examples are for our users. Wouldn't the common use case for users be to provide the config in a file? For our tests, we can always use a string.
| pub pull: Option<PullReaderConfig>, | ||
| pub periodic: Option<PeriodicReaderConfig>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to use an enum here. A MetricReader would either be periodic (push-based) or pull-based. It can't be both.
Good point. I'll move it to that repo. |
|
|
||
| /// Resource attributes to be associated with all telemetry data | ||
| #[serde(default)] | ||
| pub resource: HashMap<String, String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HashMap<String, String> restricts resource attributes to string values only. According to the specs, resource attributes must support multiple data types including integers, booleans, doubles, and arrays. If we plan to fix this in future iteration, lets add a TODO, as this would be critical requirement.
| } | ||
|
|
||
| let provider = provider_builder.build(); | ||
| global::set_meter_provider(provider.clone()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be left to the user - the configurator should just build providers, not register them globally.
| pub fn configure_telemetry( | ||
| &self, | ||
| telemetry_config: TelemetryConfig, | ||
| ) -> Result<TelemetryProviders, Box<dyn std::error::Error>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for this iteration, but good to see if we can return more structured errors to the caller.
| } | ||
|
|
||
| /// Enum representing different Batch Exporter Factories | ||
| pub enum BatchExporterFactory { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be better to separate processor and exporter.
| /// Configuration for Batch Log Processor | ||
| #[derive(Deserialize)] | ||
| pub struct ProcessorBatchConfig { | ||
| pub exporter: Value, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more generic comment for all the config's - Storing the exporter/processor/reader as serde_yaml::Value isn’t ideal - it bypasses all type validation and defers errors to runtime. This should use a typed struct or enum so schema and type issues are caught during parsing, not during exporter creation.
| pub enum PeriodicExporterFactory { | ||
| Stdout(StdoutMetricsPeriodicExporterFactory), | ||
| Otlp(OtlpMetricsPeriodicExporterFactory), | ||
| //TODO: Add other variants as needed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not very flexible. The external custom crates can't be added as new variants to the enum without adding them as dependency to otel-declarative-config crate. This should be more flexible to be extended at runtime.
One out of other approaches could be to replace the enums with trait + registry pattern:
pub trait MetricsExporterFactory: Send + Sync {
fn create_exporter(&self, config: &Value)
-> Result<Box<dyn PushMetricExporter>, Error>;
}
pub struct Configurator {
exporters: HashMap<String, Box<dyn MetricsExporterFactory>>,
}
impl Configurator {
pub fn new() -> Self {
let mut exporters = HashMap::new();
// Built-in exporters
exporters.insert("stdout".into(), Box::new(StdoutFactory));
exporters.insert("otlp".into(), Box::new(OtlpFactory));
Self { exporters }
}
// Allow external registration
pub fn register_exporter(&mut self, name: String, factory: Box<dyn MetricsExporterFactory>) {
self.exporters.insert(name, factory);
}
}|
Closing this PR in favor of open-telemetry/opentelemetry-rust-contrib#487 |
Changes
Add declarative configuration initial version.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes