-
Notifications
You must be signed in to change notification settings - Fork 70
feat: Add opentelemetry initial declarative configuration #487
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
base: main
Are you sure you want to change the base?
Conversation
05acef2 to
85505ee
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #487 +/- ##
=======================================
+ Coverage 54.0% 56.3% +2.2%
=======================================
Files 71 78 +7
Lines 11782 12532 +750
=======================================
+ Hits 6370 7058 +688
- Misses 5412 5474 +62 ☔ 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.
There is a lot of unnecessary nesting of structs. Looking at the code, we have:
MetricsConfigurator contains ReaderConfigurator
ReaderConfigurator contains PeriodicReaderConfigurator
PeriodicReaderConfigurator contains PeriodicExporterConfigurator
Also separate PullReaderConfigurator contains PullExporterConfigurator
Could we simplify this?
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 is consistent with the standard definition.
It just aligns with the standard definition. See https://github.com/open-telemetry/opentelemetry-configuration/blob/main/examples/kitchen-sink.yaml.
It can be adjusted in alignment with changes to the standard.
| Self {} | ||
| } | ||
|
|
||
| pub fn register_into(configurators_manager: &mut opentelemetry_config::ConfiguratorManager) { |
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 method seems odd. This way of registering components does not enforce an implementor to have the registration logic. I was hoping to see something similar to what @lalitb had suggested here: open-telemetry/opentelemetry-rust#3226 (comment)
Use a trait and force the implementors to provide the registration logic.
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.
That was my first attempt, but it did not work well with existing definitions, they need concrete implementations (impl) instead of dyn references.
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.
@andborja - Can you add a working example of an external crate adding a custom exporter (i.e, new exporter different from otlp/console to the YAML). I still have doubts on how this register pattern will work for external crates.
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 added two examples, one with a mock exporter, and the one with stdout.
As long as the structure (definition) is created/validated in advance, externals crates can provide implementations for it.
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 might be missing some context, but MockConsoleExporter doesn’t seem to demonstrate external custom exporter extensibility - it looks like an alternative implementation of the existing PeriodicExporterConsole type. That’s an implementation replacement rather than a type extension.
The registry pattern is interesting, but if it currently can’t support external crates adding new exporter types (as mentioned here), its value might be limited. In that case, it may be cleaner to merge the OTLP and stdout implementations directly into the main opentelemetry-config crate and simplify the design instead of maintaining a registry layer.
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 this approach is scalable. Under the current design, every external exporter requires modifying the PeriodicExporter struct in opentelemetry-config. This makes us the gatekeeper for all exporter types. For a Geneva exporter, Jaeger exporter, or any other custom exporter, teams would need to:
- Submit a PR adding their config struct to
opentelemetry-config. - Wait for review and merge.
- Have their schema maintained in this crate.
External crates should be self-contained - they should define their config types, implement the configurator trait, and work without touching opentelemetry-config source code.
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 completely. If the configuration is "standard", it will be available for usage and implementation. If it deviates from the standard, it should be approved by reviewers.
In the end it depends on the preferred strategy:
- You want more consistency and alignment with the standards. The structure is known and validated for consistency when parsing it.
- You want complete flexibility for defining the configuration externally. (with potentially different definitions for different implementations of the same element).
I prefer #1, but I'm open to discuss and get consensus on the preferred strategy.
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.
Updated the implementation so the model is extensible also.
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 current approach still seems a bit over-engineered with the introduction of new traits, type erasure using dyn Any and unnecessary public APIs such as register_into. We want to simplify the user experience for a custom exporter author.
Here's what I expect from the implementation:
- Load config from YAML/JSON → Parse into structured config
- Instantiate exporters using the standard SDK traits (e.g.,
PushMetricExporter,LogExporteretc.) instead of creating its own - Wire them up to the SDK providers
The opentelemetry-config crate should:
- Accept factory functions that take config and return
Box<dyn PushMetricExporter> - Not require custom provider traits
- Not expose
MeterProviderBuildermanipulation
opentelemetry-config crate should have something like this:
/// Type alias for metric exporter factory functions
pub type MetricExporterFactory = dyn Fn(&Value) -> Result<Box<dyn PushMetricExporter>, Error> + Send + Sync;
pub struct ConfigurationRegistry {
metric_exporter_factories: HashMap<String, Box<MetricExporterFactory>,
}
impl ConfigurationRegistry {
pub fn register_metric_exporter<F>(&mut self, name: &str, factory: F)
where
F: Fn(&Value) -> Result<Box<dyn PushMetricExporter>, Error> + 'static
{
self.metric_exporter_factories.insert(name.to_string(), Box::new(factory));
}
pub fn build_from_config(&self, config: &str) -> Result<TelemetryProviders, Error> {
// Parse YAML
// Look up factories by name
// Instantiate exporters using the SDK's standard traits
// Build and return providers
}
}opentelemetry-config-stdout or a custom exporter config crate should only need to register a factory. For example, opentelemetry-config-stdout could have something like this:
/// Configuration for the console exporter
#[derive(Deserialize, Clone, Debug)]
#[serde(deny_unknown_fields)]
pub struct ConsoleExporterConfig {
#[serde(default)]
pub temporality: Option<Temporality>,
}
#[derive(Deserialize, Clone, Debug)]
#[serde(rename_all = "lowercase")]
pub enum Temporality {
Delta,
Cumulative,
}
pub fn create_exporter(
config: &serde_yaml::Value,
) -> Result<Box<dyn PushMetricExporter>, ProviderError> {
// Deserialize the configuration
let exporter_config = serde_yaml::from_value::<ConsoleExporterConfig>(config.clone())
.map_err(|e| {
ProviderError::InvalidConfiguration(format!(
"Failed to deserialize console exporter configuration: {}",
e
))
})?;
// Build the exporter with the configured temporality
let mut exporter_builder = opentelemetry_stdout::MetricExporter::builder();
if let Some(temporality) = exporter_config.temporality {
let sdk_temporality = match temporality {
Temporality::Delta => opentelemetry_sdk::metrics::Temporality::Delta,
Temporality::Cumulative => opentelemetry_sdk::metrics::Temporality::Cumulative,
};
exporter_builder = exporter_builder.with_temporality(sdk_temporality);
}
let exporter = exporter_builder.build();
Ok(Box::new(exporter))
}Final user experience should look something like:
// Register exporters
let mut registry = ConfigurationProvidersRegistry::new();
registry.register_metric_exporter("console", opentelemetry_config_stdout::create_exporter);
// registry.register_metric_exporter("otlp", opentelemetry_config_otlp::create_exporter);
// Generate providers from config - single static call
let providers = TelemetryProvider::build_from_yaml_file(®istry, "config.yaml")?;
if let Some(meter_provider) = providers.meter_provider {
opentelemetry::global::set_meter_provider(meter_provider);
}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 suggestion look good. One comment - use typed configs (e.g., ConsoleExporterConfig) for built-in exporters rather than Value. This catches implementation bugs at compile time while maintaining the self-contained architecture - each exporter still stays in its own crate, while external customer plugins continue using dynamic validation.
Really like the direction this is heading!
| pub fn meter_provider(&self) -> Option<&SdkMeterProvider> { | ||
| self.meter_provider.as_ref() | ||
| } | ||
|
|
||
| pub fn traces_provider(&self) -> Option<&SdkTracerProvider> { | ||
| self.traces_provider.as_ref() | ||
| } | ||
|
|
||
| pub fn logs_provider(&self) -> Option<&SdkLoggerProvider> { | ||
| self.logs_provider.as_ref() | ||
| } |
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.
Please remove methods that aren't really needed to keep the PR diff smaller. These methods only seem to be used by the tests which can access the fields directly.
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 is only those.. I feel it is not a lot, and they show how it will look for the other elements. I prefer keeping them.
opentelemetry-config/README.md
Outdated
| fn configure( | ||
| &self, | ||
| meter_provider_builder: MeterProviderBuilder, | ||
| config: &dyn std::any::Any, |
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.
Do we need to rely on type erasure here - can we try with associated types or generics?
pub trait MetricsReaderPeriodicExporterConfigurator {
type Config;
fn configure(&self, builder: MeterProviderBuilder, config: &Self::Config) -> MeterProviderBuilder;
}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 try this approach before (that was the reason of the staled traits removed at the latest commit).
The problem of this approach is that we cannot use the map as a registry, as it requires specific types at that point.
We could have specific field names in the registry for every object configured instead of the Map, but I'm not sure if that is cleaner.
Changes
Adds initial declarative configuration support.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes