Feature #17841

[costanalyzer] add container duration to totals

Added by Ward Vandewege 4 months ago. Updated 4 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
07/01/2021
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-

Subtasks

Task #17842: review 17841-add-durationResolvedWard Vandewege

Associated revisions

Revision c1372488
Added by Ward Vandewege 4 months ago

Merge branch '17841-add-duration'

closes #17841

Arvados-DCO-1.1-Signed-off-by: Ward Vandewege <>

History

#1 Updated by Ward Vandewege 4 months ago

  • Status changed from New to In Progress

#2 Updated by Ward Vandewege 4 months ago

ready for review in 0153e8ec303e63ff75a1a2ce52a05ff910706029 on branch 17841-add-duration. Tests are running at https://ci.arvados.org/view/Developer/job/developer-run-tests/2555/

#3 Updated by Tom Clegg 4 months 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.

#4 Updated by Ward Vandewege 4 months 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 https://ci.arvados.org/view/Developer/job/developer-run-tests/2559/

#5 Updated by Tom Clegg 4 months ago

98bdab8fc LGTM, thanks!

#6 Updated by Ward Vandewege 4 months ago

Tom Clegg wrote:

98bdab8fc LGTM, thanks!

Thanks, I updated the doc page (I had forgotten to do that) and merged.

#7 Updated by Ward Vandewege 4 months ago

  • % Done changed from 0 to 100
  • Status changed from In Progress to Resolved

Also available in: Atom PDF