batch: add quarantine stats to batch and generate graph#100
Merged
Conversation
includes trend lines for each of the SIGs to track how each SIG handles quarantined tests Signed-off-by: Brian Carey <bcarey@redhat.com>
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Refactor the curve setup in gatherPlotData to use a data-driven mapping for colors and titles per metric instead of nested if/switch blocks, which will improve readability and make adding new metrics easier.
- In quarantineProcessor you’re re-parsing results.EndDate and only appending a single DataPoint for the total series—consider using the actual DataPoint.Date (or one per SIG) so that each quarantine curve shares the correct timestamp in sync with other metrics.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Refactor the curve setup in gatherPlotData to use a data-driven mapping for colors and titles per metric instead of nested if/switch blocks, which will improve readability and make adding new metrics easier.
- In quarantineProcessor you’re re-parsing results.EndDate and only appending a single DataPoint for the total series—consider using the actual DataPoint.Date (or one per SIG) so that each quarantine curve shares the correct timestamp in sync with other metrics.
## Individual Comments
### Comment 1
<location> `pkg/runner/batch.go:125` </location>
<code_context>
func gatherPlotData(basePath string, metric types.Metric, startDate string) ([]types.Curve, error) {
totalCurves := 2
+ if metric == types.QuarantineMetric {
+ totalCurves = 5 // Total + 4 SIGs
+ }
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring repeated switch and append logic into a loop over a curve-definition slice for cleaner and more maintainable code.
You can collapse the two big `switch`/`if` blocks and the 5× repeated `append` calls into two small loops over a “curve‐definition” slice. For example:
```go
// 1) Define your per‐curve metadata in one place:
var quarantineDefs = []struct {
Title string
Color color.RGBA
// extractor ignores DataPoint here, but we keep the same type-signature:
Value func(data types.MetricData, dp types.DataPoint) float64
}{
{
Title: "Total Quarantined",
Color: color.RGBA{255, 0, 0, 255},
Value: func(d types.MetricData, _ types.DataPoint) float64 { return d.QuarantineTotal },
},
{
Title: "SIG Compute",
Color: color.RGBA{0, 0, 255, 255},
Value: func(d types.MetricData, _ types.DataPoint) float64 { return d.QuarantineSigCompute },
},
// ... SIG Storage, SIG Network, SIG Monitoring
}
```
```go
// 2) In gatherPlotData(), pick the right defs and build curves in one loop:
func gatherPlotData(… ) ([]types.Curve, error) {
var defs []struct{Title string; Color color.RGBA; Value func(types.MetricData, types.DataPoint) float64}
if metric == types.QuarantineMetric {
defs = quarantineDefs
} else {
defs = []struct {
Title string
Color color.RGBA
Value func(types.MetricData, types.DataPoint) float64
}{
{
Title: metric.ResultsName(),
Color: color.RGBA{0, 255, 0, 255},
Value: func(d types.MetricData, dp types.DataPoint) float64 { return dp.Value },
},
{
Title: metric.ResultsName() + " (avg)",
Color: color.RGBA{0, 0, 255, 255},
Value: func(d types.MetricData, dp types.DataPoint) float64 { return d.Avg },
},
}
}
curves := make([]types.Curve, len(defs))
for i, def := range defs {
curves[i] = types.Curve{
Title: def.Title,
Color: def.Color,
X: []string{},
Y: []float64{},
}
}
// … after JSON.Unmarshal, dispatch:
switch metricName {
case constants.QuarantineStats:
addGenericResults(results, curves, metricName, defs)
// other cases…
}
return curves, nil
}
```
```go
// 3) One generic adder instead of hard‐coding 5 appends:
func addGenericResults(
results types.Results,
curves []types.Curve,
metricName string,
defs []struct {
Title string
Color color.RGBA
Value func(types.MetricData, types.DataPoint) float64
},
) {
data := results.Data[metricName]
for _, dp := range data.DataPoints {
if dp.Date == nil {
continue
}
dateStr := dp.Date.Format(constants.DateFormat)
for i, def := range defs {
curves[i].X = append(curves[i].X, dateStr)
curves[i].Y = append(curves[i].Y, def.Value(data, dp))
}
}
}
```
This:
- Removes the nested `switch/i==0` in `gatherPlotData`
- Collapses 5× repeated `append` calls into a single `for i, def := range defs`
- Keeps *exactly* the same functionality, but all the “what-to-draw” lives in one small slice of defs.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
/cc @dhiller
Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
Release note: