feat(ddm): Add negative outcomes to metric stats in pops #3452
Conversation
| @@ -480,6 +482,7 @@ impl Services { | |||
| test_store: Addr<TestStore>, | |||
| upstream_relay: Addr<UpstreamRelay>, | |||
| global_config: Addr<GlobalConfigManager>, | |||
| metric_stats: MetricStats, | |||
There was a problem hiding this comment.
Not really happy about this, since metric stats it's not accessed as a service. I could put the param top level.
There was a problem hiding this comment.
Not a fan either, maybe should just pass it as a separate arg like config to be consistent?
relay-server/src/metric_stats.rs
Outdated
| @@ -61,27 +59,30 @@ impl MetricStats { | |||
| } | |||
|
|
|||
| /// Tracks the metric volume and outcome for the bucket. | |||
| pub fn track_metric(&self, scoping: Scoping, bucket: Bucket, outcome: Outcome) { | |||
| pub fn track_metric<'a, V>(&self, scoping: Scoping, bucket: V, outcome: Outcome) | |||
There was a problem hiding this comment.
Decided to switch the implementation to take any type that can be converted into a BucketView since we ended up thinking that the semantics of owning the supplied Bucket introduce complications in places where this code is called.
iambriccardo
changed the title
relay-server/src/metric_stats.rs
Outdated
| if !self.is_enabled(scoping) { | ||
| return; | ||
| } | ||
|
|
||
| let Some(volume) = self.to_volume_metric(&bucket, &outcome) else { | ||
| let bucket_view = bucket.into(); |
There was a problem hiding this comment.
I'd just shadow bucket here:
| let bucket_view = bucket.into(); | |
| let bucket = bucket.into(); |
| pub fn reject_metrics<'a, I, V>( | ||
| addr: &Addr<TrackOutcome>, | ||
| quantities: SourceQuantities, | ||
| scoping: Scoping, | ||
| outcome: Outcome, | ||
| metric_stats: Option<&MetricStats>, | ||
| buckets: Option<I>, |
There was a problem hiding this comment.
| pub fn reject_metrics<'a, I, V>( | |
| addr: &Addr<TrackOutcome>, | |
| quantities: SourceQuantities, | |
| scoping: Scoping, | |
| outcome: Outcome, | |
| metric_stats: Option<&MetricStats>, | |
| buckets: Option<I>, | |
| pub fn reject_metrics<'a, T: Into<BucketView<'a>>>( | |
| addr: &Addr<TrackOutcome>, | |
| quantities: SourceQuantities, | |
| scoping: Scoping, | |
| outcome: Outcome, | |
| metric_stats: Option<&MetricStats>, | |
| buckets: impl IntoIterator<Item = T>, |
This should work and means you only have to supply a single generic. The trick is that None implements IntoIterator.
There was a problem hiding this comment.
I am not sure I understand, I was thinking of something like this:
fn foo<T: Into<u32>>(_: impl IntoIterator<Item = T>) {}
fn main() {
foo::<u16>(None);
foo(vec![1u16]);
}| @@ -2886,7 +2886,7 @@ impl UpstreamRequest for SendMetricsRequest { | |||
| // Request did not arrive, we are responsible for outcomes. | |||
| Err(error) if !error.is_received() => { | |||
| for (scoping, quantities) in self.quantities { | |||
| utils::reject_metrics::<Vec<Bucket>>( | |||
| utils::reject_metrics::<Vec<&Bucket>, &Bucket>( | |||
There was a problem hiding this comment.
I can fix her this. Maybe ... In a future refactor ...
relay-server/src/services/project.rs
Outdated
| let metric_stats = MetricStats::new( | ||
| Arc::new(Config::default()), | ||
| GlobalConfigHandle::fixed(GlobalConfig::default()), | ||
| addr, | ||
| ); |
There was a problem hiding this comment.
Worth adding a MetricStats::test() constructor?
| @@ -480,6 +482,7 @@ impl Services { | |||
| test_store: Addr<TestStore>, | |||
| upstream_relay: Addr<UpstreamRelay>, | |||
| global_config: Addr<GlobalConfigManager>, | |||
| metric_stats: MetricStats, | |||
There was a problem hiding this comment.
Not a fan either, maybe should just pass it as a separate arg like config to be consistent?
| &self.outcome_aggregator, | ||
| &self.metric_stats, |
There was a problem hiding this comment.
Right now we propagate the metric_stats but it's not needed since we pass no buckets. This is done because in a follow up ticket we will have to see how we want to propagate upstream the volume of rejected BucketView values. The reason for why this doesn't work right now can be seen in a comment inside reject_metrics.
This PR includes the following changes:
#[cfg(feature = "processing")]since now we are also using metric stats in pops.BucketViewfor more flexibility.