Skip to content

Commit 1e7cf97

Browse files
mattklein123mattklein123
authored
drop: fix over eager matching for exact matches (#93)
Universally using regex was causing a prefix match. Signed-off-by: Matt Klein <[email protected]>
1 parent 3cb09d2 commit 1e7cf97

File tree

2 files changed

+177
-93
lines changed

2 files changed

+177
-93
lines changed

‎pulse-metrics/src/pipeline/processor/drop/mod.rs‎

Lines changed: 39 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -34,29 +34,50 @@ use std::sync::Arc;
3434
// 1) If all name matches are exact, we can use an FST.
3535
// 2) Otherwise we can use a RegexSet and perform a single match across all names.
3636

37+
//
38+
// TranslatedStringMatchCondition
39+
//
40+
41+
enum TranslatedStringMatchCondition {
42+
Exact(String),
43+
Regex(Regex),
44+
}
45+
46+
impl TranslatedStringMatchCondition {
47+
fn is_match(&self, value: &[u8]) -> bool {
48+
match self {
49+
Self::Exact(exact) => exact.as_bytes() == value,
50+
Self::Regex(regex) => regex.is_match(value),
51+
}
52+
}
53+
}
54+
3755
//
3856
// TranslatedDropCondition
3957
//
4058

4159
enum TranslatedDropCondition {
42-
MetricName(Regex),
60+
MetricName(TranslatedStringMatchCondition),
4361
TagMatch {
4462
name: String,
45-
value_regex: Option<Regex>,
63+
value_match: Option<TranslatedStringMatchCondition>,
4664
},
4765
ValueMatch(ValueMatch),
4866
AndMatch(Vec<TranslatedDropCondition>),
4967
NotMatch(Box<TranslatedDropCondition>),
5068
}
5169

5270
impl TranslatedDropCondition {
53-
fn string_match_to_regex(string_match: &StringMatch) -> anyhow::Result<Regex> {
54-
// TODO(mattklein123): For simplicity we use regex for all matching currently.
71+
fn string_match_to_regex(
72+
string_match: &StringMatch,
73+
) -> anyhow::Result<TranslatedStringMatchCondition> {
5574
Ok(
5675
match string_match.string_match_type.as_ref().expect("pgv") {
57-
String_match_type::Exact(exact) => Regex::new(exact),
58-
String_match_type::Regex(regex) => Regex::new(regex),
59-
}?,
76+
String_match_type::Exact(exact) => TranslatedStringMatchCondition::Exact(exact.to_string()),
77+
String_match_type::Regex(regex) => {
78+
TranslatedStringMatchCondition::Regex(Regex::new(regex)?)
79+
},
80+
},
6081
)
6182
}
6283

@@ -67,7 +88,7 @@ impl TranslatedDropCondition {
6788
},
6889
Condition_type::TagMatch(tag_match) => Ok(Self::TagMatch {
6990
name: tag_match.tag_name.to_string(),
70-
value_regex: tag_match
91+
value_match: tag_match
7192
.tag_value
7293
.as_ref()
7394
.map(Self::string_match_to_regex)
@@ -108,15 +129,15 @@ impl TranslatedDropCondition {
108129

109130
fn drop_sample(&self, sample: &ParsedMetric) -> bool {
110131
match self {
111-
Self::MetricName(regex) => regex.is_match(sample.metric().get_id().name()),
112-
Self::TagMatch { name, value_regex } => sample
132+
Self::MetricName(value_match) => value_match.is_match(sample.metric().get_id().name()),
133+
Self::TagMatch { name, value_match } => sample
113134
.metric()
114135
.get_id()
115136
.tag(name.as_str())
116137
.is_some_and(|tag_value| {
117-
value_regex
138+
value_match
118139
.as_ref()
119-
.is_none_or(|value_regex| value_regex.is_match(&tag_value.value))
140+
.is_none_or(|value_match| value_match.is_match(&tag_value.value))
120141
}),
121142
Self::ValueMatch(value_match) => Self::is_value_match(sample, value_match),
122143
Self::AndMatch(conditions) => conditions
@@ -162,6 +183,12 @@ impl TranslatedDropRule {
162183
}
163184

164185
fn drop_sample(&self, sample: &ParsedMetric) -> bool {
186+
log::trace!(
187+
"checking drop rule {} mode {:?} for sample {}",
188+
self.name,
189+
self.mode,
190+
sample.metric()
191+
);
165192
let drop = self
166193
.conditions
167194
.iter()

‎pulse-metrics/src/pipeline/processor/drop/mod_test.rs‎

Lines changed: 138 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,126 @@ use prometheus::labels;
2828
use pulse_protobuf::protos::pulse::config::processor::v1::drop;
2929
use std::sync::Arc;
3030

31+
fn make_exact_match(name: &str) -> DropCondition {
32+
DropCondition {
33+
condition_type: Some(Condition_type::MetricName(StringMatch {
34+
string_match_type: Some(String_match_type::Exact(name.to_string().into())),
35+
..Default::default()
36+
})),
37+
..Default::default()
38+
}
39+
}
40+
41+
fn make_regex_match(name: &str) -> DropCondition {
42+
DropCondition {
43+
condition_type: Some(Condition_type::MetricName(StringMatch {
44+
string_match_type: Some(String_match_type::Regex(name.to_string().into())),
45+
..Default::default()
46+
})),
47+
..Default::default()
48+
}
49+
}
50+
51+
fn make_not_match(condition: DropCondition) -> DropCondition {
52+
DropCondition {
53+
condition_type: Some(Condition_type::NotMatch(Box::new(condition))),
54+
..Default::default()
55+
}
56+
}
57+
58+
fn make_tag_match(name: &str) -> DropCondition {
59+
DropCondition {
60+
condition_type: Some(Condition_type::TagMatch(TagMatch {
61+
tag_name: name.to_string().into(),
62+
..Default::default()
63+
})),
64+
..Default::default()
65+
}
66+
}
67+
68+
fn make_tag_value_exact_match(tag_name: &str, tag_value: &str) -> DropCondition {
69+
DropCondition {
70+
condition_type: Some(Condition_type::TagMatch(TagMatch {
71+
tag_name: tag_name.to_string().into(),
72+
tag_value: Some(StringMatch {
73+
string_match_type: Some(String_match_type::Exact(tag_value.to_string().into())),
74+
..Default::default()
75+
})
76+
.into(),
77+
..Default::default()
78+
})),
79+
..Default::default()
80+
}
81+
}
82+
83+
fn make_tag_value_regex_match(tag_name: &str, tag_value: &str) -> DropCondition {
84+
DropCondition {
85+
condition_type: Some(Condition_type::TagMatch(TagMatch {
86+
tag_name: tag_name.to_string().into(),
87+
tag_value: Some(StringMatch {
88+
string_match_type: Some(String_match_type::Regex(tag_value.to_string().into())),
89+
..Default::default()
90+
})
91+
.into(),
92+
..Default::default()
93+
})),
94+
..Default::default()
95+
}
96+
}
97+
98+
fn make_and_match(conditions: Vec<DropCondition>) -> DropCondition {
99+
DropCondition {
100+
condition_type: Some(Condition_type::AndMatch(AndMatch {
101+
conditions,
102+
..Default::default()
103+
})),
104+
..Default::default()
105+
}
106+
}
107+
108+
#[tokio::test]
109+
async fn regex_vs_exact() {
110+
let (mut helper, context) = processor_factory_context_for_test();
111+
let processor = Arc::new(
112+
DropProcessor::new(
113+
DropProcessorConfig {
114+
config_source: Some(Config_source::Inline(DropConfig {
115+
rules: vec![DropRule {
116+
name: "rule1".into(),
117+
mode: DropMode::ENABLED.into(),
118+
conditions: vec![make_exact_match("exact_name")],
119+
..Default::default()
120+
}],
121+
..Default::default()
122+
})),
123+
..Default::default()
124+
},
125+
context,
126+
)
127+
.await
128+
.unwrap(),
129+
);
130+
131+
make_mut(&mut helper.dispatcher)
132+
.expect_send()
133+
.times(1)
134+
.returning(|metrics| {
135+
assert_eq!(metrics, vec![make_metric("exact_name_with_more", &[], 0),]);
136+
});
137+
processor
138+
.clone()
139+
.recv_samples(vec![
140+
make_metric("exact_name", &[], 0),
141+
make_metric("exact_name_with_more", &[], 0),
142+
])
143+
.await;
144+
helper.stats_helper.assert_counter_eq(
145+
1,
146+
"processor:dropped",
147+
&labels! { "rule_name" => "rule1", "mode" => "enabled" },
148+
);
149+
}
150+
31151
#[tokio::test]
32152
async fn not() {
33153
let (mut helper, context) = processor_factory_context_for_test();
@@ -38,16 +158,7 @@ async fn not() {
38158
rules: vec![DropRule {
39159
name: "rule1".into(),
40160
mode: DropMode::ENABLED.into(),
41-
conditions: vec![DropCondition {
42-
condition_type: Some(Condition_type::NotMatch(Box::new(DropCondition {
43-
condition_type: Some(Condition_type::MetricName(StringMatch {
44-
string_match_type: Some(String_match_type::Exact("exact_name".into())),
45-
..Default::default()
46-
})),
47-
..Default::default()
48-
}))),
49-
..Default::default()
50-
}],
161+
conditions: vec![make_not_match(make_exact_match("exact_name"))],
51162
..Default::default()
52163
}],
53164
..Default::default()
@@ -92,85 +203,31 @@ async fn all() {
92203
name: "rule1".into(),
93204
mode: DropMode::ENABLED.into(),
94205
conditions: vec![
95-
DropCondition {
96-
condition_type: Some(Condition_type::MetricName(StringMatch {
97-
string_match_type: Some(String_match_type::Exact("exact_name".into())),
98-
..Default::default()
99-
})),
100-
..Default::default()
101-
},
102-
DropCondition {
103-
condition_type: Some(Condition_type::MetricName(StringMatch {
104-
string_match_type: Some(String_match_type::Regex("regex_name.*".into())),
105-
..Default::default()
106-
})),
107-
..Default::default()
108-
},
109-
DropCondition {
110-
condition_type: Some(Condition_type::TagMatch(TagMatch {
111-
tag_name: "tag_present".into(),
112-
..Default::default()
113-
})),
114-
..Default::default()
115-
},
116-
DropCondition {
117-
condition_type: Some(Condition_type::TagMatch(TagMatch {
118-
tag_name: "tag_exact".into(),
119-
tag_value: Some(StringMatch {
120-
string_match_type: Some(String_match_type::Exact("exact_value".into())),
121-
..Default::default()
122-
})
123-
.into(),
124-
..Default::default()
125-
})),
126-
..Default::default()
127-
},
128-
DropCondition {
129-
condition_type: Some(Condition_type::TagMatch(TagMatch {
130-
tag_name: "tag_regex".into(),
131-
tag_value: Some(StringMatch {
132-
string_match_type: Some(String_match_type::Exact("regex_value.*".into())),
133-
..Default::default()
134-
})
135-
.into(),
136-
..Default::default()
137-
})),
138-
..Default::default()
139-
},
206+
make_exact_match("exact_name"),
207+
make_regex_match("regex_name.*"),
208+
make_tag_match("tag_present"),
209+
make_tag_value_exact_match("tag_exact", "exact_value"),
210+
make_tag_value_regex_match("tag_regex", "regex_value.*"),
140211
],
141212
..Default::default()
142213
},
143214
DropRule {
144215
name: "rule2".into(),
145216
mode: DropMode::TESTING.into(),
146-
conditions: vec![DropCondition {
147-
condition_type: Some(Condition_type::AndMatch(AndMatch {
148-
conditions: vec![
149-
DropCondition {
150-
condition_type: Some(Condition_type::MetricName(StringMatch {
151-
string_match_type: Some(String_match_type::Exact(
152-
"value_match_name".into(),
153-
)),
154-
..Default::default()
155-
})),
156-
..Default::default()
157-
},
158-
DropCondition {
159-
condition_type: Some(Condition_type::ValueMatch(ValueMatch {
160-
value_match_type: Some(Value_match_type::SimpleValue(SimpleValueMatch {
161-
target: 100.0,
162-
operator: ValueMatchOperator::NOT_EQUAL.into(),
163-
..Default::default()
164-
})),
165-
..Default::default()
166-
})),
217+
conditions: vec![make_and_match(vec![
218+
make_exact_match("value_match_name"),
219+
DropCondition {
220+
condition_type: Some(Condition_type::ValueMatch(ValueMatch {
221+
value_match_type: Some(Value_match_type::SimpleValue(SimpleValueMatch {
222+
target: 100.0,
223+
operator: ValueMatchOperator::NOT_EQUAL.into(),
167224
..Default::default()
168-
},
169-
],
225+
})),
226+
..Default::default()
227+
})),
170228
..Default::default()
171-
})),
172-
..Default::default()
173-
}],
229+
},
230+
])],
174231
..Default::default()
175232
},
176233
],

0 commit comments

Comments
 (0)