Project

General

Profile

Actions

Feature #17717

closed

[costanalyzer] should be able to do its thing for all computation between 2 timestamps

Added by Ward Vandewege almost 3 years ago. Updated over 2 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Story points:
-
Release relationship:
Auto

Subtasks 1 (0 open1 closed)

Task #17729: review 17717-costanalyzer-date-modeResolvedWard Vandewege05/26/2021Actions
Actions #1

Updated by Ward Vandewege almost 3 years ago

  • Status changed from New to In Progress
Actions #2

Updated by Ward Vandewege almost 3 years ago

Ready for review in 05607eae460bab7efc61dd33d1e88b31139a97c7 on branch 17717-costanalyzer-date-mode

Actions #3

Updated by Ward Vandewege almost 3 years ago

  • Target version changed from 2021-05-26 sprint to 2021-06-09 sprint
Actions #4

Updated by Tom Clegg almost 3 years ago

Usage message: "cost for all supplied uuid." → "...uuids." (or UUIDs?)

Usage message: the explanation of begin/end seems a bit more verbose than necessary, and doesn't mention the somewhat subtle point that only top-level containers' finish times are considered - would this be better? "...it will calculate the cost for all top-level container requests whose container finished during the specified interval."

Nit: "2006-01-02T15:04:05" is repeated several times, should probably be a const

Nit: as a habit, putting "defer close(uuidChannel)" at the top of the go func() (instead of close() at the bottom) makes it less likely that someone will create a deadlock bug in future by inserting an early "return".

Creeping scope of review: IDK if it's worth refactoring right now, but the long list of return values from parseFlags is getting a bit awkward. This could be rearranged so the parameters are fields in the "command" struct, and (*command)parseFlags() modifies them in place. Another existing debt thing is that we seem to create a *config.Loader and pass it around but never actually use it ... and this is a client-side command so we probably never will use it.

Some sort of progress indicator would be good, if we can get one for cheap. Maybe when getting a date range, sort by finished_at, and mention that timestamp in one of the Considering/Processing/Collecting log messages? Come to think of it, merely sorting by uuid would provide a pretty useful progress indicator.

Actions #5

Updated by Ward Vandewege over 2 years ago

Tom Clegg wrote:

Usage message: "cost for all supplied uuid." → "...uuids." (or UUIDs?)

Fixed, and I capitalized UUID everywhere in the help text.

Usage message: the explanation of begin/end seems a bit more verbose than necessary, and doesn't mention the somewhat subtle point that only top-level containers' finish times are considered - would this be better? "...it will calculate the cost for all top-level container requests whose container finished during the specified interval."

Thanks, yes, replaced.

Nit: "2006-01-02T15:04:05" is repeated several times, should probably be a const

Fixed.

Nit: as a habit, putting "defer close(uuidChannel)" at the top of the go func() (instead of close() at the bottom) makes it less likely that someone will create a deadlock bug in future by inserting an early "return".

Ah, yes, fixed.

Creeping scope of review: IDK if it's worth refactoring right now, but the long list of return values from parseFlags is getting a bit awkward. This could be rearranged so the parameters are fields in the "command" struct, and (*command)parseFlags() modifies them in place.

Done!

Another existing debt thing is that we seem to create a *config.Loader and pass it around but never actually use it ... and this is a client-side command so we probably never will use it.

Oh, right. Removed.

Some sort of progress indicator would be good, if we can get one for cheap. Maybe when getting a date range, sort by finished_at, and mention that timestamp in one of the Considering/Processing/Collecting log messages? Come to think of it, merely sorting by uuid would provide a pretty useful progress indicator.

Good idea. Sorting by container.finished_at would be ideal, but apparently ordering by a field on the container table is when requesting a list of container_requests. Too bad; it works for filtering. So I did the next best thing, sorting by 'started_at', and I added the timestamp to the 'Collecting child container requests' output line.

Ready for another look at 41e0df276eaaa548e692e44cd8f3c27c4692375e on branch 17717-costanalyzer-date-mode. Tests running at developer-run-tests: #2512

Actions #6

Updated by Tom Clegg over 2 years ago

This should probably be logger.Debugf():

+       fmt.Printf("UUIDS: %s\n", c.uuids)

As an alternative to the SetUpTest func, you could set var Command = command{} and using a value receiver instead of pointer receiver for RunCommand(). Then RunCommand() wouldn't modify the global variable.

Rest LGTM.

Actions #7

Updated by Ward Vandewege over 2 years ago

Tom Clegg wrote:

This should probably be logger.Debugf():

[...]

I've just removed it, it was leftover from debugging.

As an alternative to the SetUpTest func, you could set var Command = command{} and using a value receiver instead of pointer receiver for RunCommand(). Then RunCommand() wouldn't modify the global variable.

Rest LGTM.

Done! I'll merge it.

Actions #8

Updated by Ward Vandewege over 2 years ago

  • Status changed from In Progress to Resolved
Actions #9

Updated by Peter Amstutz over 2 years ago

  • Release set to 42
Actions

Also available in: Atom PDF