Fixes #RHIROS-1401 - Dropping csv records with missing resource usage… #144
Conversation
internal/utils/aggregator.go
Outdated
| @@ -40,6 +42,33 @@ func Aggregate_data(df dataframe.DataFrame) dataframe.DataFrame { | |||
|
|
|||
| df = df.Mutate(s.Col("X0")).Rename("k8s_object_type", "X0") | |||
| df = df.Mutate(s.Col("X1")).Rename("k8s_object_name", "X1") | |||
|
|
|||
| // filter out only valid workload type | |||
| df = df.Filter( | |||
There was a problem hiding this comment.
Is it possible to get this out as separate function? I understand its just a call to Filter however the Aggregate_data is already lengthy. With that can we have tests then? So looking at those one would understand what is invalid?
There was a problem hiding this comment.
Added test cases
| // Validation to check if metrics for cpuUsage, memoryUsage and memoryRSS are missing | ||
| df, no_of_dropped_records := filter_valid_csv_records(df) | ||
| if no_of_dropped_records != 0 { | ||
| invalidDataPoints.Add(float64(no_of_dropped_records)) |
There was a problem hiding this comment.
If I understand then we are never going to return float from filter_valid_csv_records, is it prometheus which expects float value?
There was a problem hiding this comment.
yes, prometheus Add method requires the float value.
internal/utils/metrics.go
Outdated
| Name: "rosocp_invalid_datapoints_total", | ||
| Help: "The total number of invalid datapoints(rows) found in CSVs recevied", |
There was a problem hiding this comment.
nit:
| Name: "rosocp_invalid_datapoints_total", | |
| Help: "The total number of invalid datapoints(rows) found in CSVs recevied", | |
| Name: "rosocp_total_invalid_datapoints", | |
| Help: "The total number of invalid datapoints(rows) found in received CSVs", |
There was a problem hiding this comment.
According to prometheus metric naming convention - https://prometheus.io/docs/practices/naming/ suffix should describe the unit.
| df, no_of_dropped_records := filter_valid_csv_records(df) | ||
| if no_of_dropped_records != 0 { | ||
| invalidDataPoints.Add(float64(no_of_dropped_records)) | ||
| log.Infof("Invalid records in CSV - %v", no_of_dropped_records) |
There was a problem hiding this comment.
Does this need to be at Info level? and do you think we should also print more about owner_name and workload?
There was a problem hiding this comment.
Yes, good to have at info level so that using request_id we can check in kibana how many rows where dropped for particular request/CSV. Our logging system by default logs all the request related info - https://github.com/RedHatInsights/ros-ocp-backend/blob/main/internal/logging/logging.go#L66-L71
There was a problem hiding this comment.
@patilsuraj767 The Aggregate_data function seems quite long.
Suggestion: Could you split the function into 2, one could just filter the records and another aggregates?
|
/retest |
1 similar comment
|
/retest |
|
/retest |
… metrics