-
Notifications
You must be signed in to change notification settings - Fork 1
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.
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
🔭 [SDS-247] Add metric regarding multi-pass v0 #40
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a few comments
sds/src/scanner/metrics.rs
Outdated
impl Default for Metrics { | ||
fn default() -> Self { | ||
Metrics::new(&NO_LABEL) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it's needed, maybe we don't need a default here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sds/src/scanner/metrics.rs
Outdated
Metrics { | ||
false_positive_excluded_attributes: counter!( | ||
"false_positive.excluded_attributes", | ||
labels.clone_with_labels(Labels::new(&[(TYPE, "excluded_attributes".to_string())])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, no need to add the type:excluded_attributes
tag here as the same of the metric is already pretty explicit.
Also, I believe we can find a name a bit more explicit for this metric. What do you think of false_positive.multipass.excluded_match
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sds/src/scanner/mod.rs
Outdated
@@ -88,6 +91,7 @@ impl Scanner { | |||
rules: rules.clone(), | |||
scoped_ruleset, | |||
cache_pool: CachePool::new(rules), | |||
metrics: Metrics::new(&scanner_labels), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logs.sds.false_positives.v0
is tagged by scrubbing_rule_name
. With the current implementation, we will lose this tag. Would it be possible to keep this tag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I re-implemented this to include the tags of the rule: Create metric for each compiled rule
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but I think you will have some conflicts with the main branch. Let me know when they are fixed, I will take another look
99043f2
to
5b63ed6
Compare
sds/src/scanner/mod.rs
Outdated
assert_eq!(snapshot.len(), 3); | ||
|
||
let metric_name = "false_positive.multipass.excluded_match"; | ||
let metric_pos = snapshot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend converting the snapshot
into a HashMap so you can directly lookup by key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, didn't see it was possible, it will be much better indeed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 small suggestion, otherwise looks good
Description
Add a metric to measure the multi pass scanning v0 false positives generated.