Feature #17841
closed[costanalyzer] add container duration to totals
Added by Ward Vandewege about 3 years ago. Updated almost 3 years ago.
Updated by Ward Vandewege about 3 years ago
- Status changed from New to In Progress
Updated by Ward Vandewege about 3 years ago
ready for review in 0153e8ec303e63ff75a1a2ce52a05ff910706029 on branch 17841-add-duration. Tests are running at developer-run-tests: #2555
Updated by Tom Clegg about 3 years ago
addContainerLine() could also return a containerInfo struct instead of two floats
In costAnalyzer(), crCsv seems like a weird name since it's not really a csv, maybe it should be called crInfo?
Should generateCrCsv() have a "totalDuration += tmpTotalDuration" alongside "totalCost += tmpTotalCost"?
It seems like cost and duration are always retrieved the same way and added to other containerInfo field values at the same time, which might be a hint that all costs and durations should be passed around in this form, e.g., totalCost and totalDuration could be total.cost and total.duration where total is a containerInfo... and perhaps containerInfo is really "consumption" or something like that, since it doesn't necessarily refer to just a single container. Using a (*containerInfo)Add(containerInfo) method instead of a +=
statement for each field would help avoid future bugs like the missing addition to totalDuration, if indeed that's a bug.
Updated by Ward Vandewege about 3 years ago
Tom Clegg wrote:
addContainerLine() could also return a containerInfo struct instead of two floats
Done!
In costAnalyzer(), crCsv seems like a weird name since it's not really a csv, maybe it should be called crInfo?
Done!
Should generateCrCsv() have a "totalDuration += tmpTotalDuration" alongside "totalCost += tmpTotalCost"?
Yeah, fixed, good find.
It seems like cost and duration are always retrieved the same way and added to other containerInfo field values at the same time, which might be a hint that all costs and durations should be passed around in this form, e.g., totalCost and totalDuration could be total.cost and total.duration where total is a containerInfo...
Yes! Changed.
and perhaps containerInfo is really "consumption" or something like that, since it doesn't necessarily refer to just a single container. Using a (*containerInfo)Add(containerInfo) method instead of a
+=
statement for each field would help avoid future bugs like the missing addition to totalDuration, if indeed that's a bug.
Yes! Good idea, changed.
I also fixed an offset problem with the TOTAL line in the per-CR csv files, the column positions for duration and cost were off by one.
Ready for another look at 98bdab8fca735201efe2a785b6c20003e1d9058f on branch 17841-add-duration. Tests are running at developer-run-tests: #2559
Updated by Ward Vandewege about 3 years ago
Tom Clegg wrote:
98bdab8fc LGTM, thanks!
Thanks, I updated the doc page (I had forgotten to do that) and merged.
Updated by Ward Vandewege about 3 years ago
- % Done changed from 0 to 100
- Status changed from In Progress to Resolved
Applied in changeset arvados|c137248849c4b4ad38513f3faebd1782037add09.