https://dev.arvados.org/https://dev.arvados.org/favicon.ico?15576888422021-06-29T17:25:51ZArvadosArvados - Feature #17841: [costanalyzer] add container duration to totalshttps://dev.arvados.org/issues/17841?journal_id=939612021-06-29T17:25:51ZWard Vandewegeward@curii.com
<ul><li><strong>Status</strong> changed from <i>New</i> to <i>In Progress</i></li></ul> Arvados - Feature #17841: [costanalyzer] add container duration to totalshttps://dev.arvados.org/issues/17841?journal_id=939622021-06-29T17:39:06ZWard Vandewegeward@curii.com
<ul></ul><p>ready for review in <a class="changeset" title="17841: add container duration to the totals in the csv files. Reduce cost precision in tot..." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/0153e8ec303e63ff75a1a2ce52a05ff910706029">0153e8ec303e63ff75a1a2ce52a05ff910706029</a> on branch 17841-add-duration. Tests are running at <a class="external" href="https://ci.arvados.org/view/Developer/job/developer-run-tests/2555/"<a href="https://ci.arvados.org/view/Developer/job/developer-run-tests/2555/">developer-run-tests: #2555 <img src="https://ci.arvados.org/buildStatus/icon?job=developer-run-tests&build=2555" alt="" /></a></a></p> Arvados - Feature #17841: [costanalyzer] add container duration to totalshttps://dev.arvados.org/issues/17841?journal_id=939632021-06-29T18:40:49ZTom Cleggtom@curii.com
<ul></ul><p>addContainerLine() could also return a containerInfo struct instead of two floats</p>
<p>In costAnalyzer(), crCsv seems like a weird name since it's not really a csv, maybe it should be called crInfo?</p>
<p>Should generateCrCsv() have a "totalDuration += tmpTotalDuration" alongside "totalCost += tmpTotalCost"?</p>
<p>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 <code>+=</code> statement for each field would help avoid future bugs like the missing addition to totalDuration, if indeed that's a bug.</p> Arvados - Feature #17841: [costanalyzer] add container duration to totalshttps://dev.arvados.org/issues/17841?journal_id=939662021-06-29T21:38:47ZWard Vandewegeward@curii.com
<ul></ul><p>Tom Clegg wrote:</p>
<blockquote>
<p>addContainerLine() could also return a containerInfo struct instead of two floats</p>
</blockquote>
<p>Done!</p>
<blockquote>
<p>In costAnalyzer(), crCsv seems like a weird name since it's not really a csv, maybe it should be called crInfo?</p>
</blockquote>
<p>Done!</p>
<blockquote>
<p>Should generateCrCsv() have a "totalDuration += tmpTotalDuration" alongside "totalCost += tmpTotalCost"?</p>
</blockquote>
<p>Yeah, fixed, good find.</p>
<blockquote>
<p>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...</p>
</blockquote>
<p>Yes! Changed.</p>
<blockquote>
<p>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 <code>+=</code> statement for each field would help avoid future bugs like the missing addition to totalDuration, if indeed that's a bug.</p>
</blockquote>
<p>Yes! Good idea, changed.</p>
<p>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.</p>
<p>Ready for another look at <a class="changeset" title="17841: address review comments. Arvados-DCO-1.1-Signed-off-by: Ward Vandewege <ward@curii.com>" href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/98bdab8fca735201efe2a785b6c20003e1d9058f">98bdab8fca735201efe2a785b6c20003e1d9058f</a> on branch 17841-add-duration. Tests are running at <a class="external" href="https://ci.arvados.org/view/Developer/job/developer-run-tests/2559/"<a href="https://ci.arvados.org/view/Developer/job/developer-run-tests/2559/">developer-run-tests: #2559 <img src="https://ci.arvados.org/buildStatus/icon?job=developer-run-tests&build=2559" alt="" /></a></a></p> Arvados - Feature #17841: [costanalyzer] add container duration to totalshttps://dev.arvados.org/issues/17841?journal_id=941252021-07-06T15:35:10ZTom Cleggtom@curii.com
<ul></ul><p><a class="changeset" title="17841: address review comments. Arvados-DCO-1.1-Signed-off-by: Ward Vandewege <ward@curii.com>" href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/98bdab8fca735201efe2a785b6c20003e1d9058f">98bdab8fc</a> LGTM, thanks!</p> Arvados - Feature #17841: [costanalyzer] add container duration to totalshttps://dev.arvados.org/issues/17841?journal_id=944992021-07-06T21:38:13ZWard Vandewegeward@curii.com
<ul></ul><p>Tom Clegg wrote:</p>
<blockquote>
<p><a class="changeset" title="17841: address review comments. Arvados-DCO-1.1-Signed-off-by: Ward Vandewege <ward@curii.com>" href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/98bdab8fca735201efe2a785b6c20003e1d9058f">98bdab8fc</a> LGTM, thanks!</p>
</blockquote>
<p>Thanks, I updated the doc page (I had forgotten to do that) and merged.</p> Arvados - Feature #17841: [costanalyzer] add container duration to totalshttps://dev.arvados.org/issues/17841?journal_id=945002021-07-06T21:38:18ZWard Vandewegeward@curii.com
<ul><li><strong>% Done</strong> changed from <i>0</i> to <i>100</i></li><li><strong>Status</strong> changed from <i>In Progress</i> to <i>Resolved</i></li></ul><p>Applied in changeset <a class="changeset" title="Merge branch '17841-add-duration' closes #17841 Arvados-DCO-1.1-Signed-off-by: Ward Vandewege <..." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/c137248849c4b4ad38513f3faebd1782037add09">arvados|c137248849c4b4ad38513f3faebd1782037add09</a>.</p> Arvados - Feature #17841: [costanalyzer] add container duration to totalshttps://dev.arvados.org/issues/17841?journal_id=985742021-11-16T16:28:11ZPeter Amstutzpeter.amstutz@curii.com
<ul><li><strong>Release</strong> set to <i>42</i></li></ul>