Skip to content
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

AggregationSelector is not needed anymore #2085

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions opentelemetry-otlp/src/exporter/http/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@
/// ```
/// # #[cfg(feature="metrics")]
/// use opentelemetry_sdk::metrics::reader::{
/// DefaultAggregationSelector, DefaultTemporalitySelector,
/// DefaultTemporalitySelector,
/// };
///
/// # fn main() -> Result<(), Box<dyn std::error::Error>> {
Expand All @@ -91,7 +91,6 @@
/// let metrics_exporter = opentelemetry_otlp::new_exporter()
/// .http()
/// .build_metrics_exporter(
/// Box::new(DefaultAggregationSelector::new()),
/// Box::new(DefaultTemporalitySelector::new()),
/// )?;
///
Expand Down Expand Up @@ -252,7 +251,6 @@
#[cfg(feature = "metrics")]
pub fn build_metrics_exporter(
mut self,
aggregation_selector: Box<dyn opentelemetry_sdk::metrics::reader::AggregationSelector>,
temporality_selector: Box<dyn opentelemetry_sdk::metrics::reader::TemporalitySelector>,
) -> opentelemetry::metrics::Result<crate::MetricsExporter> {
use crate::{
Expand All @@ -267,11 +265,7 @@
OTEL_EXPORTER_OTLP_METRICS_HEADERS,
)?;

Ok(crate::MetricsExporter::new(
client,
temporality_selector,
aggregation_selector,
))
Ok(crate::MetricsExporter::new(client, temporality_selector))

Check warning on line 268 in opentelemetry-otlp/src/exporter/http/mod.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-otlp/src/exporter/http/mod.rs#L268

Added line #L268 was not covered by tests
}
}

Expand Down
10 changes: 2 additions & 8 deletions opentelemetry-otlp/src/exporter/tonic/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@
/// ```no_run
/// # #[cfg(feature="metrics")]
/// use opentelemetry_sdk::metrics::reader::{
/// DefaultAggregationSelector, DefaultTemporalitySelector,
/// DefaultTemporalitySelector,
/// };
///
/// # fn main() -> Result<(), Box<dyn std::error::Error>> {
Expand All @@ -110,7 +110,6 @@
/// let metrics_exporter = opentelemetry_otlp::new_exporter()
/// .tonic()
/// .build_metrics_exporter(
/// Box::new(DefaultAggregationSelector::new()),
/// Box::new(DefaultTemporalitySelector::new()),
/// )?;
///
Expand Down Expand Up @@ -332,7 +331,6 @@
#[cfg(feature = "metrics")]
pub fn build_metrics_exporter(
self,
aggregation_selector: Box<dyn opentelemetry_sdk::metrics::reader::AggregationSelector>,
temporality_selector: Box<dyn opentelemetry_sdk::metrics::reader::TemporalitySelector>,
) -> opentelemetry::metrics::Result<crate::MetricsExporter> {
use crate::MetricsExporter;
Expand All @@ -347,11 +345,7 @@

let client = TonicMetricsClient::new(channel, interceptor, compression);

Ok(MetricsExporter::new(
client,
temporality_selector,
aggregation_selector,
))
Ok(MetricsExporter::new(client, temporality_selector))

Check warning on line 348 in opentelemetry-otlp/src/exporter/tonic/mod.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-otlp/src/exporter/tonic/mod.rs#L348

Added line #L348 was not covered by tests
}

/// Build a new tonic span exporter
Expand Down
3 changes: 1 addition & 2 deletions opentelemetry-otlp/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@
//! use opentelemetry::{global, KeyValue, trace::Tracer};
//! use opentelemetry_sdk::{trace::{self, RandomIdGenerator, Sampler}, Resource};
//! # #[cfg(feature = "metrics")]
//! use opentelemetry_sdk::metrics::reader::{DefaultAggregationSelector, DefaultTemporalitySelector};
//! use opentelemetry_sdk::metrics::reader::DefaultTemporalitySelector;
//! use opentelemetry_otlp::{Protocol, WithExportConfig, ExportConfig};
//! use std::time::Duration;
//! # #[cfg(feature = "grpc-tonic")]
Expand Down Expand Up @@ -184,7 +184,6 @@
//! .with_resource(Resource::new(vec![KeyValue::new("service.name", "example")]))
//! .with_period(Duration::from_secs(3))
//! .with_timeout(Duration::from_secs(10))
//! .with_aggregation_selector(DefaultAggregationSelector::new())
//! .with_temporality_selector(DefaultTemporalitySelector::new())
//! .build();
//! # }
Expand Down
35 changes: 4 additions & 31 deletions opentelemetry-otlp/src/metric.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,8 @@
metrics::{
data::{ResourceMetrics, Temporality},
exporter::PushMetricsExporter,
reader::{
AggregationSelector, DefaultAggregationSelector, DefaultTemporalitySelector,
TemporalitySelector,
},
Aggregation, InstrumentKind, PeriodicReader, SdkMeterProvider,
reader::{DefaultTemporalitySelector, TemporalitySelector},
InstrumentKind, PeriodicReader, SdkMeterProvider,
},
runtime::Runtime,
Resource,
Expand Down Expand Up @@ -50,7 +47,6 @@
{
OtlpMetricPipeline {
rt,
aggregator_selector: None,
temporality_selector: None,
exporter_pipeline: NoExporterConfig(()),
resource: None,
Expand Down Expand Up @@ -82,21 +78,19 @@
pub fn build_metrics_exporter(
self,
temporality_selector: Box<dyn TemporalitySelector>,
aggregation_selector: Box<dyn AggregationSelector>,
) -> Result<MetricsExporter> {
match self {
#[cfg(feature = "grpc-tonic")]
MetricsExporterBuilder::Tonic(builder) => {
builder.build_metrics_exporter(aggregation_selector, temporality_selector)
builder.build_metrics_exporter(temporality_selector)

Check warning on line 85 in opentelemetry-otlp/src/metric.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-otlp/src/metric.rs#L85

Added line #L85 was not covered by tests
}
#[cfg(feature = "http-proto")]
MetricsExporterBuilder::Http(builder) => {
builder.build_metrics_exporter(aggregation_selector, temporality_selector)
builder.build_metrics_exporter(temporality_selector)

Check warning on line 89 in opentelemetry-otlp/src/metric.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-otlp/src/metric.rs#L89

Added line #L89 was not covered by tests
}
#[cfg(not(any(feature = "http-proto", feature = "grpc-tonic")))]
MetricsExporterBuilder::Unconfigured => {
drop(temporality_selector);
drop(aggregation_selector);
Err(opentelemetry::metrics::MetricsError::Other(
"no configured metrics exporter, enable `http-proto` or `grpc-tonic` feature to configure a metrics exporter".into(),
))
Expand Down Expand Up @@ -125,7 +119,6 @@
/// runtime.
pub struct OtlpMetricPipeline<RT, EB> {
rt: RT,
aggregator_selector: Option<Box<dyn AggregationSelector>>,
temporality_selector: Option<Box<dyn TemporalitySelector>>,
exporter_pipeline: EB,
resource: Option<Resource>,
Expand Down Expand Up @@ -178,14 +171,6 @@
pub fn with_delta_temporality(self) -> Self {
self.with_temporality_selector(DeltaTemporalitySelector)
}

/// Build with the given aggregation selector
pub fn with_aggregation_selector<T: AggregationSelector + 'static>(self, selector: T) -> Self {
OtlpMetricPipeline {
aggregator_selector: Some(Box::new(selector)),
..self
}
}
}

impl<RT> OtlpMetricPipeline<RT, NoExporterConfig>
Expand All @@ -200,7 +185,6 @@
OtlpMetricPipeline {
exporter_pipeline: pipeline.into(),
rt: self.rt,
aggregator_selector: self.aggregator_selector,
temporality_selector: self.temporality_selector,
resource: self.resource,
period: self.period,
Expand All @@ -218,8 +202,6 @@
let exporter = self.exporter_pipeline.build_metrics_exporter(
self.temporality_selector
.unwrap_or_else(|| Box::new(DefaultTemporalitySelector::new())),
self.aggregator_selector
.unwrap_or_else(|| Box::new(DefaultAggregationSelector::new())),
)?;

let mut builder = PeriodicReader::builder(exporter, self.rt);
Expand Down Expand Up @@ -295,7 +277,6 @@
pub struct MetricsExporter {
client: Box<dyn MetricsClient>,
temporality_selector: Box<dyn TemporalitySelector>,
aggregation_selector: Box<dyn AggregationSelector>,
}

impl Debug for MetricsExporter {
Expand All @@ -310,12 +291,6 @@
}
}

impl AggregationSelector for MetricsExporter {
fn aggregation(&self, kind: InstrumentKind) -> Aggregation {
self.aggregation_selector.aggregation(kind)
}
}

#[async_trait]
impl PushMetricsExporter for MetricsExporter {
async fn export(&self, metrics: &mut ResourceMetrics) -> Result<()> {
Expand All @@ -337,12 +312,10 @@
pub fn new(
client: impl MetricsClient,
temporality_selector: Box<dyn TemporalitySelector>,
aggregation_selector: Box<dyn AggregationSelector>,
) -> MetricsExporter {
MetricsExporter {
client: Box::new(client),
temporality_selector,
aggregation_selector,
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,12 @@
"value": {
"intValue": "100"
}
},
{
"key": "number/int",
"value": {
"intValue": "100"
}
}
],
"droppedAttributesCount": 0
Expand Down
8 changes: 1 addition & 7 deletions opentelemetry-sdk/benches/metric.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use opentelemetry_sdk::{
metrics::{
data::{ResourceMetrics, Temporality},
new_view,
reader::{AggregationSelector, MetricReader, TemporalitySelector},
reader::{MetricReader, TemporalitySelector},
Aggregation, Instrument, InstrumentKind, ManualReader, Pipeline, SdkMeterProvider, Stream,
View,
},
Expand All @@ -26,12 +26,6 @@ impl TemporalitySelector for SharedReader {
}
}

impl AggregationSelector for SharedReader {
fn aggregation(&self, kind: InstrumentKind) -> Aggregation {
self.0.aggregation(kind)
}
}

impl MetricReader for SharedReader {
fn register_pipeline(&self, pipeline: Weak<Pipeline>) {
self.0.register_pipeline(pipeline)
Expand Down
4 changes: 2 additions & 2 deletions opentelemetry-sdk/src/metrics/aggregation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ pub enum Aggregation {
/// instrument kind that differs from the default. This aggregation ensures the
/// default is used.
///
/// See the [DefaultAggregationSelector] for information about the default
/// See the [the spec] for information about the default
/// instrument kind selection mapping.
///
/// [DefaultAggregationSelector]: crate::metrics::reader::DefaultAggregationSelector
/// [the spec]: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.19.0/specification/metrics/sdk.md#default-aggregation
Default,

/// An aggregation that summarizes a set of measurements as their arithmetic
Expand Down
9 changes: 2 additions & 7 deletions opentelemetry-sdk/src/metrics/exporter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,13 @@ use async_trait::async_trait;

use opentelemetry::metrics::Result;

use crate::metrics::{
data::ResourceMetrics,
reader::{AggregationSelector, TemporalitySelector},
};
use crate::metrics::{data::ResourceMetrics, reader::TemporalitySelector};

/// Exporter handles the delivery of metric data to external receivers.
///
/// This is the final component in the metric push pipeline.
#[async_trait]
pub trait PushMetricsExporter:
AggregationSelector + TemporalitySelector + Send + Sync + 'static
{
pub trait PushMetricsExporter: TemporalitySelector + Send + Sync + 'static {
/// Export serializes and transmits metric data to a receiver.
///
/// All retry logic must be contained in this function. The SDK does not
Expand Down
34 changes: 2 additions & 32 deletions opentelemetry-sdk/src/metrics/manual_reader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,7 @@
instrument::InstrumentKind,
pipeline::Pipeline,
reader::{
AggregationSelector, DefaultAggregationSelector, DefaultTemporalitySelector,
MetricProducer, MetricReader, SdkProducer, TemporalitySelector,
DefaultTemporalitySelector, MetricProducer, MetricReader, SdkProducer, TemporalitySelector,
},
};

Expand All @@ -34,7 +33,6 @@
pub struct ManualReader {
inner: Box<Mutex<ManualReaderInner>>,
temporality_selector: Box<dyn TemporalitySelector>,
aggregation_selector: Box<dyn AggregationSelector>,
}

impl Default for ManualReader {
Expand Down Expand Up @@ -65,7 +63,6 @@
/// A [MetricReader] which is directly called to collect metrics.
pub(crate) fn new(
temporality_selector: Box<dyn TemporalitySelector>,
aggregation_selector: Box<dyn AggregationSelector>,
producers: Vec<Box<dyn MetricProducer>>,
) -> Self {
ManualReader {
Expand All @@ -75,7 +72,6 @@
external_producers: producers,
})),
temporality_selector,
aggregation_selector,
}
}
}
Expand All @@ -86,12 +82,6 @@
}
}

impl AggregationSelector for ManualReader {
fn aggregation(&self, kind: InstrumentKind) -> super::aggregation::Aggregation {
self.aggregation_selector.aggregation(kind)
}
}

impl MetricReader for ManualReader {
/// Register a pipeline which enables the caller to read metrics from the SDK
/// on demand.
Expand Down Expand Up @@ -159,7 +149,6 @@
/// Configuration for a [ManualReader]
pub struct ManualReaderBuilder {
temporality_selector: Box<dyn TemporalitySelector>,
aggregation_selector: Box<dyn AggregationSelector>,
producers: Vec<Box<dyn MetricProducer>>,
}

Expand All @@ -173,7 +162,6 @@
fn default() -> Self {
ManualReaderBuilder {
temporality_selector: Box::new(DefaultTemporalitySelector { _private: () }),
aggregation_selector: Box::new(DefaultAggregationSelector { _private: () }),
producers: vec![],
}
}
Expand All @@ -196,20 +184,6 @@
self
}

/// Sets the [AggregationSelector] a reader will use to determine the
/// aggregation to use for an instrument based on its kind.
///
/// If this option is not used, the reader will use the default aggregation
/// selector or the aggregation explicitly passed for a view matching an
/// instrument.
pub fn with_aggregation_selector(
mut self,
aggregation_selector: impl AggregationSelector + 'static,
) -> Self {
self.aggregation_selector = Box::new(aggregation_selector);
self
}

/// Registers a an external [MetricProducer] with this reader.
///
/// The producer is used as a source of aggregated metric data which is
Expand All @@ -221,10 +195,6 @@

/// Create a new [ManualReader] from this configuration.
pub fn build(self) -> ManualReader {
ManualReader::new(
self.temporality_selector,
self.aggregation_selector,
self.producers,
)
ManualReader::new(self.temporality_selector, self.producers)

Check warning on line 198 in opentelemetry-sdk/src/metrics/manual_reader.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-sdk/src/metrics/manual_reader.rs#L198

Added line #L198 was not covered by tests
}
}
Loading
Loading