-
Notifications
You must be signed in to change notification settings - Fork 108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add upload statistics to report #3615
Conversation
Present upload counts in various buckets, including "this year", "this month", "this week", "today" along with per year/month/day-of-month/hour just for fun.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great (although, the headers for the buckets could be more descriptive -- I fell into the trap of thinking that the "by day" buckets were for each of the last 31 days and that the "by hour" were for the last 24 hours, as opposed to, e.g., "the second hour of any day"...unfortunately, I don't have any good suggestions off the top of my head).
I do have a pointed question and a small 1+ for your consideration.
lib/pbench/cli/server/report.py
Outdated
click.echo(f" {this_month:,d} uploads this month ({month:%B, %Y})") | ||
click.echo(f" {this_week:,d} uploads this week ({week:%B %d} to {day:%B %d})") | ||
click.echo(f" {this_day:,d} uploads today ({day:%Y-%m-%d})") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure you want to use %Y-%m-%d
for "today" after using %B
in the previous two lines? (And, do you really want the comma in %B, %Y
?) I would opt for %d %B %Y
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did bounce back and forth on the exact format for several. Basically I was doing this on half the burners in between bouts of wrestling with Horreum and Kiota.
lib/pbench/cli/server/report.py
Outdated
def columnize(items: dict[str, Any], width: int = 80): | ||
line = "" | ||
for item, count in sorted(items.items()): | ||
if len(line) >= width: | ||
click.echo(line) | ||
line = "" | ||
line += f" {item:4d}: {count:>8,d}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the code adds to the line in chunks of 18 characters (I think...), the test at line 314 could produce a line which is "too long" (i.e., 90 characters, by default). [I don't know why this doesn't happen in the same output in the description...did you specify a smaller width for that?]
I would recommend something like this:
line = ""
for item, count in sorted(items.items()):
addition = f" {item:4d}: {count:>8,d}"
if len(line) + len(addition) >= width:
click.echo(line)
line = ""
line += addition
or
line = ""
for item, count in sorted(items.items()):
addition = f" {item:4d}: {count:>8,d}"
if len(line) + len(addition) >= width:
click.echo(line)
line = addition
else:
line += addition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I threw in the columnization at the last minute because the lists were rather long: you're right, it's not entirely accurate as is, but I didn't spend too many cycles worrying about it. However, I guess it's worth cleaning up.
(And, yeah, I think I generated the sample output with --width 70
to make sure GitHub didn't wrap it. 😆 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized after pushing the PR that I'd meant to also touch up the feedback (and add a "show" indexing state) on reindex after our ops review experience, so this is due another pass anyway this weekend... and I'd've maybe felt slightly bad about that if you'd approved without comments.
lib/pbench/cli/server/report.py
Outdated
click.echo(f" {this_month:,d} uploads this month ({month:%B, %Y})") | ||
click.echo(f" {this_week:,d} uploads this week ({week:%B %d} to {day:%B %d})") | ||
click.echo(f" {this_day:,d} uploads today ({day:%Y-%m-%d})") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did bounce back and forth on the exact format for several. Basically I was doing this on half the burners in between bouts of wrestling with Horreum and Kiota.
lib/pbench/cli/server/report.py
Outdated
def columnize(items: dict[str, Any], width: int = 80): | ||
line = "" | ||
for item, count in sorted(items.items()): | ||
if len(line) >= width: | ||
click.echo(line) | ||
line = "" | ||
line += f" {item:4d}: {count:>8,d}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I threw in the columnization at the last minute because the lists were rather long: you're right, it's not entirely accurate as is, but I didn't spend too many cycles worrying about it. However, I guess it's worth cleaning up.
(And, yeah, I think I generated the sample output with --width 70
to make sure GitHub didn't wrap it. 😆 )
On a whim, I split the dataset history statistics to be able to show by
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks excellent.
Present upload counts in various buckets, including "this year", "this month", "this week", "today" along with per year/month/day-of-month/day-of-week/hour-of-day just for fun.