Skip to content

Commit c7e9c95

Browse files
prabhaksparmesant
andauthored
fix(otel): sync OTEL_METRICS_KNOWN_FIELD_LIST with emitted keys (#1723) (#1724)
compute_series_hash treats any top-level data-point key not in OTEL_METRICS_KNOWN_FIELD_LIST as a series label, so the list must match the keys the flatteners actually emit. It had drifted both ways: - Emitted but missing (fragmented series): `min` and `max` (histogram per-sample stats) and `data_point_quantile_values` (summary quantile array). Their per-sample values leaked into __series_hash, splitting a physical series into many. - Listed but never emitted as top-level keys (dead entries): `quantile` and `value` (only emitted nested inside data_point_quantile_values), plus the never-used data_point_quantile_values_quantile / data_point_quantile_values_value. `value`/`quantile` also risked collisions: a user attribute of that name was silently dropped from series identity, merging distinct series. Add the 3 missing keys, remove the 4 dead ones (length 37 -> 36), and add tests for min/max and quantile-value fragmentation plus the value/quantile collision. fix(otel): prefix histogram min/max keys to avoid shadowing user attributes Bare min/max top-level keys collide with any OTel attribute literally named min or max once added to OTEL_METRICS_KNOWN_FIELD_LIST, wrongly excluding that attribute from the series hash. Renamed to data_point_min/data_point_max, matching every other data-point field's naming convention, and added a regression test for the collision case. --------- Signed-off-by: parmesant <anant.v09@protonmail.com> Co-authored-by: parmesant <anant.v09@protonmail.com>
1 parent cac761e commit c7e9c95

1 file changed

Lines changed: 111 additions & 6 deletions

File tree

‎src/otel/metrics.rs‎

Lines changed: 111 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ use super::otel_utils::{
3939

4040
pub const SERIES_HASH_COLUMN: &str = "__series_hash_u64";
4141

42-
pub const OTEL_METRICS_KNOWN_FIELD_LIST: [&str; 38] = [
42+
pub const OTEL_METRICS_KNOWN_FIELD_LIST: [&str; 39] = [
4343
"metric_name",
4444
"metric_description",
4545
"metric_unit",
@@ -59,8 +59,12 @@ pub const OTEL_METRICS_KNOWN_FIELD_LIST: [&str; 38] = [
5959
"data_point_sum",
6060
"data_point_bucket_counts",
6161
"data_point_explicit_bounds",
62+
"data_point_quantile_values",
6263
"data_point_quantile_values_quantile",
6364
"data_point_quantile_values_value",
65+
// Histogram per-sample min/max statistics.
66+
"data_point_min",
67+
"data_point_max",
6468
"data_point_scale",
6569
"data_point_zero_count",
6670
"data_point_flags",
@@ -69,8 +73,6 @@ pub const OTEL_METRICS_KNOWN_FIELD_LIST: [&str; 38] = [
6973
"positive_bucket_count",
7074
"negative_offset",
7175
"negative_bucket_count",
72-
"quantile",
73-
"value",
7476
"is_monotonic",
7577
"aggregation_temporality",
7678
"aggregation_temporality_description",
@@ -358,8 +360,8 @@ fn flatten_histogram(histogram: &Histogram, flatten_exemplars: bool) -> Vec<Map<
358360
);
359361

360362
insert_data_point_flags(&mut data_point_json, data_point.flags);
361-
insert_number_if_some(&mut data_point_json, "min", &data_point.min);
362-
insert_number_if_some(&mut data_point_json, "max", &data_point.max);
363+
insert_number_if_some(&mut data_point_json, "data_point_min", &data_point.min);
364+
insert_number_if_some(&mut data_point_json, "data_point_max", &data_point.max);
363365
insert_aggregation_temporality(&mut data_point_json, histogram.aggregation_temporality);
364366
data_points_json.push(data_point_json);
365367
}
@@ -1021,9 +1023,112 @@ mod tests {
10211023
assert_eq!(compute_series_hash(&a), compute_series_hash(&b));
10221024
}
10231025

1026+
fn number(value: f64) -> Value {
1027+
serde_json::Number::from_f64(value)
1028+
.map(Value::Number)
1029+
.unwrap_or(Value::Null)
1030+
}
1031+
1032+
#[test]
1033+
fn series_hash_ignores_histogram_min_max() {
1034+
// `data_point_min`/`data_point_max` are per-sample histogram
1035+
// statistics, not series labels. Two samples of the same series with
1036+
// different min/max must hash identically, otherwise histogram
1037+
// series fragment on every scrape.
1038+
let mut a = make_dp();
1039+
a.insert("data_point_min".to_string(), number(1.0));
1040+
a.insert("data_point_max".to_string(), number(9.0));
1041+
let mut b = make_dp();
1042+
b.insert("data_point_min".to_string(), number(2.0));
1043+
b.insert("data_point_max".to_string(), number(8.0));
1044+
assert_eq!(compute_series_hash(&a), compute_series_hash(&b));
1045+
}
1046+
1047+
#[test]
1048+
fn series_hash_ignores_summary_quantile_values() {
1049+
// `data_point_quantile_values` is the nested per-sample quantile
1050+
// array. Differing quantile values must not change series identity.
1051+
let mut a = make_dp();
1052+
a.insert(
1053+
"data_point_quantile_values".to_string(),
1054+
Value::Array(vec![number(0.5), number(0.9)]),
1055+
);
1056+
let mut b = make_dp();
1057+
b.insert(
1058+
"data_point_quantile_values".to_string(),
1059+
Value::Array(vec![number(0.7), number(0.99)]),
1060+
);
1061+
assert_eq!(compute_series_hash(&a), compute_series_hash(&b));
1062+
}
1063+
10241064
#[test]
10251065
fn known_field_list_contains_exemplars() {
1026-
assert_eq!(OTEL_METRICS_KNOWN_FIELD_LIST.len(), 38);
10271066
assert!(OTEL_METRICS_KNOWN_FIELDS.contains("exemplars"));
10281067
}
1068+
1069+
#[test]
1070+
fn series_hash_distinguishes_attribute_named_value_or_quantile() {
1071+
// `value` and `quantile` are only ever emitted nested inside the
1072+
// quantile array, never as top-level fields. They must NOT be
1073+
// treated as known fields: a user attribute literally named
1074+
// `value` or `quantile` is a real label and must affect series
1075+
// identity (previously it was wrongly excluded, colliding series).
1076+
let mut a = make_dp();
1077+
a.insert("value".to_string(), Value::String("x".into()));
1078+
let mut b = make_dp();
1079+
b.insert("value".to_string(), Value::String("y".into()));
1080+
assert_ne!(compute_series_hash(&a), compute_series_hash(&b));
1081+
1082+
let mut c = make_dp();
1083+
c.insert("quantile".to_string(), Value::String("p50".into()));
1084+
let mut d = make_dp();
1085+
d.insert("quantile".to_string(), Value::String("p99".into()));
1086+
assert_ne!(compute_series_hash(&c), compute_series_hash(&d));
1087+
}
1088+
1089+
#[test]
1090+
fn series_hash_distinguishes_attribute_named_min_or_max() {
1091+
// Histogram min/max statistics are emitted under the prefixed
1092+
// `data_point_min`/`data_point_max` keys, not bare `min`/`max`. A
1093+
// user attribute literally named `min` or `max` is a real label and
1094+
// must affect series identity.
1095+
let mut a = make_dp();
1096+
a.insert("min".to_string(), Value::String("x".into()));
1097+
let mut b = make_dp();
1098+
b.insert("min".to_string(), Value::String("y".into()));
1099+
assert_ne!(compute_series_hash(&a), compute_series_hash(&b));
1100+
1101+
let mut c = make_dp();
1102+
c.insert("max".to_string(), Value::String("x".into()));
1103+
let mut d = make_dp();
1104+
d.insert("max".to_string(), Value::String("y".into()));
1105+
assert_ne!(compute_series_hash(&c), compute_series_hash(&d));
1106+
}
1107+
1108+
#[test]
1109+
fn known_field_list_matches_emitted_keys() {
1110+
// Guard the list against drifting from the keys the flatteners
1111+
// actually emit as top-level data-point fields.
1112+
assert_eq!(OTEL_METRICS_KNOWN_FIELD_LIST.len(), 39);
1113+
for field in [
1114+
"data_point_min",
1115+
"data_point_max",
1116+
"data_point_quantile_values",
1117+
"data_point_quantile_values_quantile",
1118+
"data_point_quantile_values_value",
1119+
"exemplars",
1120+
] {
1121+
assert!(
1122+
OTEL_METRICS_KNOWN_FIELDS.contains(field),
1123+
"expected `{field}` to be a known field"
1124+
);
1125+
}
1126+
// Removed dead entries that were never emitted as top-level keys.
1127+
for field in ["quantile", "value", "min", "max"] {
1128+
assert!(
1129+
!OTEL_METRICS_KNOWN_FIELDS.contains(field),
1130+
"`{field}` must not be a known field"
1131+
);
1132+
}
1133+
}
10291134
}

0 commit comments

Comments
 (0)