Aggregate contract metrics when no contract or host filter is provided #802
Conversation
ChrisSchinnerl
force-pushed
the
chris/contract-metrics-no-filter
branch
from
8a25820
to
3f5dbd8
Compare
ChrisSchinnerl
force-pushed
the
chris/contract-metrics-no-filter
branch
from
3f5dbd8
to
3f63ebf
Compare
stores/metrics.go
Outdated
| } | ||
| currentPeriod := int64(math.MinInt64) | ||
| err := tx.Raw(` | ||
| SELECT * FROM contracts |
There was a problem hiding this comment.
maybe reformat this raw query block to remove leading tabs/spaces and make the inner select more clear
There was a problem hiding this comment.
I added some indentation to make it look nicer but I'm not sure how to get rid of tabs while at the same time making it more clear by itself and with the surrounding code.
stores/metrics.go
Outdated
| metrics = append(metrics, m.Metric) | ||
| currentPeriod = m.Period | ||
| } else { | ||
| metrics[len(metrics)-1] = metrics[len(metrics)-1].Aggregate(m.Metric) |
There was a problem hiding this comment.
I'm not too sure but often with logic like this you have to perform this block outside of the loop to cover the last batch, is this one of those cases? e.g. do you need to call Aggregate on the last metric here after the loop on metricsWithPeriod?
There was a problem hiding this comment.
I think this is fine since every metric in the loop is either appended or aggregated. Looking at the test I also don't think there is anything missing at the end.
ChrisSchinnerl
force-pushed
the
chris/contract-metrics-no-filter
branch
from
1bba83a
to
2d04ba3
Compare
When no filter for either contract id or host is provided, the contract metrics returned by the metrics endpoint will aggregate the spending within a period using the first metric for any given contract within the period.