Skip to content

client stats: do not panic if a metric changes type #183

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 27, 2025
Merged

client stats: do not panic if a metric changes type #183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 5 additions & 2 deletions bd-buffer/src/ring_buffer_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use bd_client_stats_store::test::StatsHelper;
use bd_client_stats_store::{Collector, Counter};
use bd_proto::protos::config::v1::config::buffer_config::BufferSizes;
use bd_proto::protos::config::v1::config::{BufferConfig, BufferConfigList, buffer_config};
use bd_stats_common::{NameType, labels};
use bd_stats_common::{MetricType, NameType, labels};
use std::path::{Path, PathBuf};

fn fake_counter() -> Counter {
Expand Down Expand Up @@ -258,7 +258,10 @@ async fn ring_buffer_stats() {
assert!(
collector
.find_counter(
&NameType::Global("ring_buffer:volatile_overwrite".to_string()),
&NameType::Global(
MetricType::Counter,
"ring_buffer:volatile_overwrite".to_string()
),
&labels! { "buffer_id" => "some-buffer" },
)
.unwrap()
Expand Down
63 changes: 41 additions & 22 deletions bd-client-stats-store/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use bd_proto::protos::client::metric::{
DDSketchHistogram,
InlineHistogramValues,
};
use bd_stats_common::{DynCounter, NameType, make_client_sketch};
use bd_stats_common::{DynCounter, MetricType, NameType, make_client_sketch};
use parking_lot::Mutex;
use sketches_rust::DDSketch;
use std::collections::hash_map::Entry;
Expand Down Expand Up @@ -49,9 +49,6 @@ const MAX_INLINE_HISTOGRAM_VALUES: usize = 5;

#[derive(thiserror::Error, Debug)]
pub enum Error {
#[error("Changed type")]
ChangedType,

#[error("Overflow")]
Overflow,
}
Expand Down Expand Up @@ -285,12 +282,13 @@ impl Scope {
fn get_common_global(
inner: &mut CollectorInner,
name: String,
metric_type: MetricType,
labels: BTreeMap<String, String>,
constructor: impl FnOnce() -> MetricData,
) -> &mut MetricData {
let by_name = inner
.metrics_by_name
.entry(NameType::Global(name))
.entry(NameType::Global(metric_type, name))
.or_default();

match by_name.entry(labels) {
Expand All @@ -302,11 +300,17 @@ impl Scope {
#[must_use]
pub fn counter_with_labels(&self, name: &str, labels: BTreeMap<String, String>) -> Counter {
let mut inner = self.collector.inner.lock();
match Self::get_common_global(&mut inner, self.metric_name(name), labels, || {
MetricData::Counter(Counter {
value: Arc::new(AtomicU64::new(0)),
})
}) {
match Self::get_common_global(
&mut inner,
self.metric_name(name),
MetricType::Counter,
labels,
|| {
MetricData::Counter(Counter {
value: Arc::new(AtomicU64::new(0)),
})
},
) {
MetricData::Counter(counter) => counter.clone(),
MetricData::Histogram(_) => unreachable!(),
}
Expand All @@ -325,9 +329,13 @@ impl Scope {
#[must_use]
pub fn histogram_with_labels(&self, name: &str, labels: BTreeMap<String, String>) -> Histogram {
let mut inner = self.collector.inner.lock();
match Self::get_common_global(&mut inner, self.metric_name(name), labels, || {
MetricData::Histogram(Histogram::default())
}) {
match Self::get_common_global(
&mut inner,
self.metric_name(name),
MetricType::Histogram,
labels,
|| MetricData::Histogram(Histogram::default()),
) {
MetricData::Histogram(histogram) => histogram.clone(),
MetricData::Counter(_) => unreachable!(),
}
Expand Down Expand Up @@ -533,12 +541,13 @@ impl Collector {
fn get_common_workflow(
inner: &mut CollectorInner,
action_id: String,
metric_type: MetricType,
labels: BTreeMap<String, String>,
constructor: impl FnOnce() -> MetricData,
) -> Result<&mut MetricData> {
let by_name = inner
.metrics_by_name
.entry(NameType::ActionId(action_id))
.entry(NameType::ActionId(metric_type, action_id))
.or_default();

let by_name_len = by_name.len();
Expand All @@ -560,21 +569,31 @@ impl Collector {

pub fn dynamic_counter(&self, tags: BTreeMap<String, String>, id: &str) -> Result<Counter> {
let mut inner = self.inner.lock();
match Self::get_common_workflow(&mut inner, id.to_string(), tags, || {
MetricData::Counter(Counter {
value: Arc::new(AtomicU64::new(0)),
})
})? {
match Self::get_common_workflow(
&mut inner,
id.to_string(),
MetricType::Counter,
tags,
|| {
MetricData::Counter(Counter {
value: Arc::new(AtomicU64::new(0)),
})
},
)? {
MetricData::Counter(counter) => Ok(counter.clone()),
MetricData::Histogram(_) => unreachable!(),
}
}

pub fn dynamic_histogram(&self, tags: BTreeMap<String, String>, id: &str) -> Result<Histogram> {
let mut inner = self.inner.lock();
match Self::get_common_workflow(&mut inner, id.to_string(), tags, || {
MetricData::Histogram(Histogram::default())
})? {
match Self::get_common_workflow(
&mut inner,
id.to_string(),
MetricType::Histogram,
tags,
|| MetricData::Histogram(Histogram::default()),
)? {
MetricData::Histogram(histogram) => Ok(histogram.clone()),
MetricData::Counter(_) => unreachable!(),
}
Expand Down
40 changes: 33 additions & 7 deletions bd-client-stats-store/src/lib_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,15 @@
// https://polyformproject.org/wp-content/uploads/2020/06/PolyForm-Shield-1.0.0.txt

use crate::{Collector, MetricData, NameType};
use bd_stats_common::labels;
use bd_stats_common::{MetricType, labels};
use std::collections::BTreeMap;

#[test]
fn dual_type() {
let collector = Collector::default();
let _counter = collector.dynamic_counter(BTreeMap::new(), "same_id");
let _histogram = collector.dynamic_histogram(BTreeMap::new(), "same_id");
}

#[test]
fn retain() {
Expand All @@ -15,12 +23,18 @@ fn retain() {
let h = collector.scope("").histogram("h");
assert!(
collector
.find_counter(&NameType::Global("c".to_string()), &labels!())
.find_counter(
&NameType::Global(MetricType::Counter, "c".to_string()),
&labels!()
)
.is_some()
);
assert!(
collector
.find_histogram(&NameType::Global("h".to_string()), &labels!())
.find_histogram(
&NameType::Global(MetricType::Histogram, "h".to_string()),
&labels!()
)
.is_some()
);

Expand All @@ -29,12 +43,18 @@ fn retain() {
drop(h);
assert!(
collector
.find_counter(&NameType::Global("c".to_string()), &labels!())
.find_counter(
&NameType::Global(MetricType::Counter, "c".to_string()),
&labels!()
)
.is_some()
);
assert!(
collector
.find_histogram(&NameType::Global("h".to_string()), &labels!())
.find_histogram(
&NameType::Global(MetricType::Histogram, "h".to_string()),
&labels!()
)
.is_some()
);

Expand All @@ -45,12 +65,18 @@ fn retain() {

assert!(
collector
.find_counter(&NameType::Global("c".to_string()), &labels!())
.find_counter(
&NameType::Global(MetricType::Counter, "c".to_string()),
&labels!()
)
.is_none()
);
assert!(
collector
.find_histogram(&NameType::Global("h".to_string()), &labels!())
.find_histogram(
&NameType::Global(MetricType::Histogram, "h".to_string()),
&labels!()
)
.is_none()
);
}
16 changes: 13 additions & 3 deletions bd-client-stats-store/src/test/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
// https://polyformproject.org/wp-content/uploads/2020/06/PolyForm-Shield-1.0.0.txt

use crate::{Collector, HistogramInner, MetricData, NameType};
use bd_stats_common::MetricType;
use std::collections::BTreeMap;

pub trait StatsHelper {
Expand All @@ -30,7 +31,10 @@ impl StatsHelper for Collector {
assert_eq!(
value,
self
.find_counter(&NameType::Global(name.to_string()), &labels)
.find_counter(
&NameType::Global(MetricType::Counter, name.to_string()),
&labels
)
.unwrap_or_else(|| panic!("Counter not found: {name} {labels:?}"))
.get()
);
Expand All @@ -46,7 +50,10 @@ impl StatsHelper for Collector {
assert_eq!(
value,
self
.find_counter(&NameType::ActionId(action_id.to_string()), &labels)
.find_counter(
&NameType::ActionId(MetricType::Counter, action_id.to_string()),
&labels
)
.unwrap_or_else(|| panic!("Counter not found: {action_id} {labels:?}"))
.get()
);
Expand All @@ -59,7 +66,10 @@ impl StatsHelper for Collector {
labels: BTreeMap<String, String>,
) {
let histogram_values = match self
.find_histogram(&NameType::ActionId(action_id.to_string()), &labels)
.find_histogram(
&NameType::ActionId(MetricType::Histogram, action_id.to_string()),
&labels,
)
.unwrap_or_else(|| panic!("Histogram not found: {action_id} {labels:?}"))
.snap()
.unwrap()
Expand Down
14 changes: 0 additions & 14 deletions bd-client-stats/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,7 @@ pub mod stats;
pub mod test;

use crate::stats::Flusher;
use anyhow::anyhow;
use bd_api::DataUpload;
use bd_client_common::error::handle_unexpected;
use bd_client_common::file_system::RealFileSystem;
use bd_client_stats_store::{Collector, Error as StatsError};
use bd_runtime::runtime::ConfigLoader;
Expand Down Expand Up @@ -162,12 +160,6 @@ impl Stats {
pub fn record_dynamic_counter(&self, tags: BTreeMap<String, String>, id: &str, value: u64) {
match self.collector.dynamic_counter(tags, id) {
Ok(counter) => counter.inc_by(value),
Err(StatsError::ChangedType) => {
handle_unexpected::<(), anyhow::Error>(
Err(anyhow!("change in dynamic metric type")),
"dynamic counter type change",
);
},
Err(StatsError::Overflow) => {
self.handle_overflow(id);
},
Expand All @@ -177,12 +169,6 @@ impl Stats {
pub fn record_dynamic_histogram(&self, tags: BTreeMap<String, String>, id: &str, value: f64) {
match self.collector.dynamic_histogram(tags, id) {
Ok(histogram) => histogram.observe(value),
Err(StatsError::ChangedType) => {
handle_unexpected::<(), anyhow::Error>(
Err(anyhow!("change in dynamic metric type")),
"dynamic histogram type change",
);
},
Err(StatsError::Overflow) => {
self.handle_overflow(id);
},
Expand Down
31 changes: 18 additions & 13 deletions bd-client-stats/src/stats.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use bd_proto::protos::client::api::stats_upload_request::snapshot::Snapshot_type
use bd_proto::protos::client::metric::metric::Metric_name_type;
use bd_proto::protos::client::metric::{Metric as ProtoMetric, MetricsList};
use bd_shutdown::ComponentShutdown;
use bd_stats_common::NameType;
use bd_stats_common::{MetricType, NameType};
use bd_time::TimeDurationExt;
#[cfg(test)]
use stats_test::{TestHooks, TestHooksReceiver};
Expand Down Expand Up @@ -503,16 +503,21 @@ impl SnapshotHelper {
};

let mut new_metrics: MetricsByName = HashMap::new();
for metric in metrics.metric {
let name = match metric.metric_name_type {
Some(Metric_name_type::Name(name)) => NameType::Global(name),
Some(Metric_name_type::MetricId(id)) => NameType::ActionId(id),
None => continue,
};

let tags = metric.tags.into_iter().collect();
if let Some(data) = metric.data {
for proto_metric in metrics.metric {
let tags = proto_metric.tags.into_iter().collect();
if let Some(data) = proto_metric.data {
if let Some(metric) = MetricData::from_proto(data) {
let metric_type = match metric {
MetricData::Counter(_) => MetricType::Counter,
MetricData::Histogram(_) => MetricType::Histogram,
};

let name = match proto_metric.metric_name_type {
Some(Metric_name_type::Name(name)) => NameType::Global(metric_type, name),
Some(Metric_name_type::MetricId(id)) => NameType::ActionId(metric_type, id),
None => continue,
};

let existing = new_metrics.entry(name).or_default().insert(tags, metric);
debug_assert!(existing.is_none());
}
Expand All @@ -534,7 +539,7 @@ impl SnapshotHelper {
}

fn add_metric(&mut self, name: NameType, labels: BTreeMap<String, String>, metric: MetricData) {
let maybe_limit = if matches!(name, NameType::ActionId(_)) {
let maybe_limit = if matches!(name, NameType::ActionId(..)) {
self.limit
} else {
None
Expand Down Expand Up @@ -566,8 +571,8 @@ impl SnapshotHelper {
.into_iter()
.map(move |(labels, metric)| ProtoMetric {
metric_name_type: Some(match name.clone() {
NameType::Global(name) => Metric_name_type::Name(name),
NameType::ActionId(id) => Metric_name_type::MetricId(id),
NameType::Global(_, name) => Metric_name_type::Name(name),
NameType::ActionId(_, id) => Metric_name_type::MetricId(id),
}),
tags: labels.into_iter().collect(),
data: Some(metric.to_proto()),
Expand Down
Loading
Loading