Skip to content

Commit 2f252cf

Browse files
authored
Merge target_metrics uniquely (#1615)
This commit resolves a bug in PR #1614 spotted by @GeorgeHahn. If configs are merged that contain multiple target metrics this is not a crash-worthy state but we do not want to parse the same target metrics multiple times. Signed-off-by: Brian L. Troutwine <[email protected]>
1 parent c11a60f commit 2f252cf

File tree

1 file changed

+40
-4
lines changed

1 file changed

+40
-4
lines changed

lading/src/config.rs

Lines changed: 40 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -294,10 +294,14 @@ impl Config {
294294
base.blackhole.extend(overlay.blackhole);
295295
check_duplicate_blackhole_ids(&base.blackhole)?;
296296

297-
// Merge target_metrics
297+
// Merge target_metrics, deduplicating entries
298298
if let Some(overlay_metrics) = overlay.target_metrics {
299299
if let Some(ref mut existing) = base.target_metrics {
300-
existing.extend(overlay_metrics);
300+
for metric in overlay_metrics {
301+
if !existing.contains(&metric) {
302+
existing.push(metric);
303+
}
304+
}
301305
} else {
302306
base.target_metrics = Some(overlay_metrics);
303307
}
@@ -1102,7 +1106,7 @@ generator:
11021106
}
11031107

11041108
#[test]
1105-
fn target_metrics_merge_concatenates(
1109+
fn target_metrics_merge_unique_entries(
11061110
num_a in 1_usize..4,
11071111
num_b in 1_usize..4,
11081112
) {
@@ -1120,11 +1124,43 @@ generator:
11201124
let merged = Config::merge_partial(partial_a, partial_b)
11211125
.expect("merge should succeed");
11221126

1123-
// target_metrics should be concatenated
1127+
// Unique target_metrics should all be present
11241128
let target_metrics = merged.target_metrics.expect("should have target_metrics");
11251129
prop_assert_eq!(target_metrics.len(), num_a + num_b);
11261130
}
11271131

1132+
#[test]
1133+
fn target_metrics_merge_deduplicates(
1134+
num_unique in 1_usize..4,
1135+
num_duplicates in 1_usize..4,
1136+
) {
1137+
// Create metrics that exist in both partials (duplicates)
1138+
let shared_metrics: Vec<_> = (0..num_unique)
1139+
.map(|i| make_prometheus_target_metrics(&format!("http://localhost:{}/metrics", 9000 + i)))
1140+
.collect();
1141+
1142+
// partial_a has the shared metrics
1143+
let partial_a = make_partial_with_target_metrics(shared_metrics.clone());
1144+
1145+
// partial_b has duplicates of some shared metrics
1146+
let duplicate_metrics: Vec<_> = (0..num_duplicates)
1147+
.map(|i| make_prometheus_target_metrics(&format!("http://localhost:{}/metrics", 9000 + (i % num_unique))))
1148+
.collect();
1149+
let partial_b = make_partial_with_target_metrics(duplicate_metrics);
1150+
1151+
let merged = Config::merge_partial(partial_a, partial_b)
1152+
.expect("merge should succeed");
1153+
1154+
// Should only have unique metrics, duplicates removed
1155+
let target_metrics = merged.target_metrics.expect("should have target_metrics");
1156+
prop_assert_eq!(target_metrics.len(), num_unique);
1157+
1158+
// Verify all original metrics are present
1159+
for metric in &shared_metrics {
1160+
prop_assert!(target_metrics.contains(metric));
1161+
}
1162+
}
1163+
11281164
#[test]
11291165
fn none_id_generators_can_merge(
11301166
num_a in 1_usize..5,

0 commit comments

Comments
 (0)