Report: Add infrastructure for Perf Stat rules #244
Conversation
|
I recommend thinking about how to do this such that it doesn't require explicit on-boarding for each data type to support analytics rules. The data they output should be self-describing enough that aggregates can be formed generically. |
I prefer things are explicit. It will help when others are looking to contribute rules. |
| @@ -70,4 +70,5 @@ function is_unique(values_array) { | |||
| let all_rules: Rules[] = [ | |||
| system_info_rules, | |||
| cpu_utilization_rules, | |||
| perf_stat_rules, | |||
There was a problem hiding this comment.
Are we planning to have all the rules defined in the code or give also the user the possibility to add/configure/change them?
There was a problem hiding this comment.
Currently we want all the rules to be in code.
There was a problem hiding this comment.
It'd be beneficial if this was somehow discoverable instead of needing it to be added explicitly for every data source.
| name: "ipc", | ||
| per_run_rule: function* (ruleOpts: RuleOpts) : Generator<Finding, void, any> { | ||
| let diff = percent_difference(ruleOpts.base_run_data, ruleOpts.this_run_data); | ||
| if (diff > 10) { |
There was a problem hiding this comment.
do we want this % to be fixed?
There was a problem hiding this comment.
Currently yes. Maybe once folks use it and get back with details about other units we can change it.
| @@ -199,3 +199,7 @@ function get_data_key(data_type, key) { | |||
| } | |||
| return key_value_map; | |||
| } | |||
|
|
|||
| function percent_difference(v1, v2) { | |||
There was a problem hiding this comment.
v1 and v2 should be typed as number correct? This function should also return a number.
| @@ -433,11 +434,12 @@ struct EndPerfData { | |||
| pub metadata: GraphMetadata, | |||
| } | |||
|
|
|||
| fn get_values(values: Vec<PerfStat>, key: String) -> Result<String> { | |||
| fn get_values(values: Vec<PerfStat>, key: String, metrics: &mut DataMetrics) -> Result<String> { | |||
There was a problem hiding this comment.
Since every data source is going to implement something like this, shouldn't we make a rust trait to hold some of the assumed common functionality every data source needs?
| @@ -70,4 +70,5 @@ function is_unique(values_array) { | |||
| let all_rules: Rules[] = [ | |||
| system_info_rules, | |||
| cpu_utilization_rules, | |||
| perf_stat_rules, | |||
There was a problem hiding this comment.
It'd be beneficial if this was somehow discoverable instead of needing it to be added explicitly for every data source.
This will be followed by infrastructure for other data types.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.