From 985867385ee08896dfb8df06e65d723da5760f42 Mon Sep 17 00:00:00 2001 From: Lance Austin Date: Thu, 29 Jul 2021 10:23:25 -0500 Subject: [PATCH] addressed feedback in favor of impl `Default` over `from_env` --- opentelemetry-prometheus/CHANGELOG.md | 6 +- opentelemetry-prometheus/src/lib.rs | 97 ++++++++++++++----- .../tests/integration_test.rs | 30 ------ 3 files changed, 76 insertions(+), 57 deletions(-) diff --git a/opentelemetry-prometheus/CHANGELOG.md b/opentelemetry-prometheus/CHANGELOG.md index 903cbb1191..4d503a5ff9 100644 --- a/opentelemetry-prometheus/CHANGELOG.md +++ b/opentelemetry-prometheus/CHANGELOG.md @@ -4,7 +4,11 @@ ### Added -- Add `from_env` method to prometheus exporter based on semantic environment vars +- Adds `Default` implementation to `ExporterBuilder` based on the otel specification environment variables #242 + +### Deprecated + +- `PrometheusExporter::new()` is deprecated in favor of using `ExporterBuilder` ## v0.8.0 diff --git a/opentelemetry-prometheus/src/lib.rs b/opentelemetry-prometheus/src/lib.rs index 8d8630dc1c..65c11cf075 100644 --- a/opentelemetry-prometheus/src/lib.rs +++ b/opentelemetry-prometheus/src/lib.rs @@ -93,13 +93,25 @@ const DEFAULT_CACHE_PERIOD: Duration = Duration::from_secs(0); const EXPORT_KIND_SELECTOR: ExportKindSelector = ExportKindSelector::Cumulative; +/// Default host used by the Prometheus Exporter when env variable not found +const DEFAULT_EXPORTER_HOST: &str = "0.0.0.0"; + +/// Default port used by the Prometheus Exporter when env variable not found +const DEFAULT_EXPORTER_PORT: &str = "9464"; + +/// The hostname for the Promtheus Exporter +const ENV_EXPORTER_HOST: &str = "OTEL_EXPORTER_PROMETHEUS_HOST"; + +/// The port for the Prometheus Exporter +const ENV_EXPORTER_PORT: &str = "OTEL_EXPORTER_PROMETHEUS_PORT"; + /// Create a new prometheus exporter builder. pub fn exporter() -> ExporterBuilder { ExporterBuilder::default() } /// Configuration for the prometheus exporter. -#[derive(Debug, Default)] +#[derive(Debug)] pub struct ExporterBuilder { /// The OpenTelemetry `Resource` associated with all Meters /// created by the pull controller. @@ -135,30 +147,21 @@ pub struct ExporterBuilder { port: Option, } -impl ExporterBuilder { - /// Set builder fields for host and port from the OS environment variables - /// - /// Fields set by developer takes precedent and the OS environment variables will be ignored - pub fn from_env() -> Self { - let mut builder = ExporterBuilder::default(); - - if let Some(host) = env::var("OTEL_EXPORTER_PROMETHEUS_HOST") - .ok() - .filter(|s| !s.is_empty()) - { - builder = builder.with_host(host) - } - - if let Some(port) = env::var("OTEL_EXPORTER_PROMETHEUS_PORT") - .ok() - .filter(|s| !s.is_empty()) - { - builder = builder.with_port(port); +impl Default for ExporterBuilder { + fn default() -> Self { + ExporterBuilder { + resource: None, + cache_period: None, + default_histogram_boundaries: None, + default_summary_quantiles: None, + registry: None, + host: env::var(ENV_EXPORTER_HOST).ok().filter(|s| !s.is_empty()), + port: env::var(ENV_EXPORTER_PORT).ok().filter(|s| !s.is_empty()), } - - builder } +} +impl ExporterBuilder { /// Set the resource to be associated with all `Meter`s for this exporter pub fn with_resource(self, resource: Resource) -> Self { ExporterBuilder { @@ -239,17 +242,27 @@ impl ExporterBuilder { global::set_meter_provider(controller.provider()); - let host = self.host.unwrap_or_else(|| "0.0.0.0".to_string()); - let port = self.port.unwrap_or_else(|| "9464".to_string()); + let host = self + .host + .unwrap_or_else(|| DEFAULT_EXPORTER_HOST.to_string()); + let port = self + .port + .unwrap_or_else(|| DEFAULT_EXPORTER_PORT.to_string()); + + let controller = Arc::new(Mutex::new(controller)); + let collector = Collector::with_controller(controller.clone()); + registry + .register(Box::new(collector)) + .map_err(|e| MetricsError::Other(e.to_string()))?; - PrometheusExporter::new( + Ok(PrometheusExporter { registry, controller, default_summary_quantiles, default_histogram_boundaries, host, port, - ) + }) } /// Sets up a complete export pipeline with the recommended setup, using the @@ -278,6 +291,10 @@ pub struct PrometheusExporter { } impl PrometheusExporter { + #[deprecated( + since = "0.9.0", + note = "Please use the ExporterBuilder to initialize a PrometheusExporter" + )] /// Create a new prometheus exporter pub fn new( registry: prometheus::Registry, @@ -529,3 +546,31 @@ fn get_metric_desc(record: &Record<'_>) -> PrometheusMetricDesc { .unwrap_or_else(|| desc.name().to_string()); PrometheusMetricDesc { name, help } } + +#[cfg(test)] +mod tests { + use std::env; + + use super::*; + + #[test] + fn test_exporter_builder_default() { + env::remove_var(ENV_EXPORTER_HOST); + env::remove_var(ENV_EXPORTER_PORT); + let exporter = ExporterBuilder::default().init(); + assert_eq!(exporter.host(), "0.0.0.0"); + assert_eq!(exporter.port(), "9464"); + + env::set_var(ENV_EXPORTER_HOST, "prometheus-test"); + env::set_var(ENV_EXPORTER_PORT, "9000"); + let exporter = ExporterBuilder::default().init(); + assert_eq!(exporter.host(), "prometheus-test"); + assert_eq!(exporter.port(), "9000"); + + env::set_var(ENV_EXPORTER_HOST, ""); + env::set_var(ENV_EXPORTER_PORT, ""); + let exporter = ExporterBuilder::default().init(); + assert_eq!(exporter.host(), "0.0.0.0"); + assert_eq!(exporter.port(), "9464"); + } +} diff --git a/opentelemetry-prometheus/tests/integration_test.rs b/opentelemetry-prometheus/tests/integration_test.rs index d175e66a47..ec32f912b1 100644 --- a/opentelemetry-prometheus/tests/integration_test.rs +++ b/opentelemetry-prometheus/tests/integration_test.rs @@ -1,5 +1,3 @@ -use std::env; - use opentelemetry::sdk::Resource; use opentelemetry::{ metrics::{BatchObserverResult, MeterProvider, ObserverResult}, @@ -161,31 +159,3 @@ fn compare_export(exporter: &PrometheusExporter, mut expected: Vec<&'static str> assert_eq!(expected.join("\n"), metrics_only.join("\n")) } - -#[test] -fn test_from_env() { - let otel_exporter_prometheus_host = "OTEL_EXPORTER_PROMETHEUS_HOST"; - let otel_exporter_prometheus_port = "OTEL_EXPORTER_PROMETHEUS_PORT"; - - // environment variables do not exist - env::remove_var(otel_exporter_prometheus_host); - env::remove_var(otel_exporter_prometheus_port); - let exporter = opentelemetry_prometheus::ExporterBuilder::from_env().init(); - assert_eq!(exporter.host(), "0.0.0.0"); - assert_eq!(exporter.port(), "9464"); - - // environment variables are available and non-empty strings - env::set_var(otel_exporter_prometheus_host, "prometheus-test"); - env::set_var(otel_exporter_prometheus_port, "9000"); - - let exporter = opentelemetry_prometheus::ExporterBuilder::from_env().init(); - assert_eq!(exporter.host(), "prometheus-test"); - assert_eq!(exporter.port(), "9000"); - - // environment variables are available and empty - env::set_var(otel_exporter_prometheus_host, ""); - env::set_var(otel_exporter_prometheus_port, ""); - let exporter = opentelemetry_prometheus::ExporterBuilder::from_env().init(); - assert_eq!(exporter.host(), "0.0.0.0"); - assert_eq!(exporter.port(), "9464"); -}