Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃敪 [SDS-247] Add metric regarding multi-pass v0 #40

Merged
merged 8 commits into from May 13, 2024

Conversation

artslidd
Copy link
Contributor

@artslidd artslidd commented May 2, 2024

Description

Add a metric to measure the multi pass scanning v0 false positives generated.

@artslidd artslidd marked this pull request as ready for review May 2, 2024 10:14
Copy link
Contributor

@vinckama vinckama left a 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

Comment on lines 21 to 24
impl Default for Metrics {
fn default() -> Self {
Metrics::new(&NO_LABEL)
}
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics {
false_positive_excluded_attributes: counter!(
"false_positive.excluded_attributes",
labels.clone_with_labels(Labels::new(&[(TYPE, "excluded_attributes".to_string())]))
Copy link
Contributor

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -88,6 +91,7 @@ impl Scanner {
rules: rules.clone(),
scoped_ruleset,
cache_pool: CachePool::new(rules),
metrics: Metrics::new(&scanner_labels),
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

@vinckama vinckama left a 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

@artslidd artslidd force-pushed the artslidd/sds-247-add-obsv branch from 99043f2 to 5b63ed6 Compare May 7, 2024 13:19
assert_eq!(snapshot.len(), 3);

let metric_name = "false_positive.multipass.excluded_match";
let metric_pos = snapshot
Copy link
Contributor

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.

Copy link
Contributor Author

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!

Copy link
Contributor Author

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 Show resolved Hide resolved
@artslidd artslidd requested a review from fuchsnj May 13, 2024 11:49
Copy link
Contributor

@fuchsnj fuchsnj left a 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

@artslidd artslidd merged commit 220c98b into main May 13, 2024
4 checks passed
@artslidd artslidd deleted the artslidd/sds-247-add-obsv branch May 13, 2024 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants