Skip to content
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 extra and gifted execution minutes #1361

Merged
merged 34 commits into from
Dec 7, 2023
Merged

Conversation

tw4l
Copy link
Member

@tw4l tw4l commented Nov 8, 2023

Fixes #1358

Could use a good bit of additional manual testing, edge cases/fuzz testing welcomed (e.g. this has not been tested across months yet)! And very open to any suggestions to simplify the implementation on backend and/or frontend.

Overview

  • Adds extraExecMinutes and giftedExecMinutes org quotas, which are not reset monthly but a one-time update-able bucket
  • Adds quotaUpdate field to Organization to track when quotas were updated with timestamp
  • Adds extraExecMinutesAvailable and giftedExecMinutesAvailable fields to Organization to help with tracking available time left (includes tested migration to initialize these to 0)
  • Modifies org backend to track time across multiple categories, using monthly exec time first, then gifted, then extra
  • Updates Dashboard crawling meter to include all types of execution time if extraExecMinutes and/or giftedExecMinutes are set above 0
  • Updates Dashboard Usage History table to include all types of execution time
  • Adds backend nightly test to check handling of quotas and execution time

Screenshots

Only monthly quota set, no execution time used yet

Screen Shot 2023-11-15 at 1 20 50 PM

Only monthly quota set, with some execution time used

Screen Shot 2023-11-15 at 1 24 15 PM

All three quotas set, no execution time used yet

Screen Shot 2023-11-15 at 2 05 32 PM

All three quotas set, some execution time used

Screen Shot 2023-11-15 at 2 27 24 PM Screen Shot 2023-11-15 at 2 43 30 PM Screen Shot 2023-11-15 at 2 47 22 PM Screen Shot 2023-11-15 at 2 59 35 PM

Tooltips

Screen Shot 2023-11-15 at 2 27 36 PM Screen Shot 2023-11-15 at 2 27 39 PM

Quota reached

Screen Shot 2023-11-15 at 3 04 20 PM

With storage quota also enabled

Screen Shot 2023-11-15 at 3 05 36 PM

@tw4l tw4l force-pushed the issue-1358-additional-minutes branch 3 times, most recently from e5807ba to 7bd5ab8 Compare November 15, 2023 16:41
@tw4l tw4l marked this pull request as ready for review November 15, 2023 21:14
@tw4l tw4l requested review from ikreymer, SuaYoo and emma-sg November 15, 2023 21:14
@tw4l tw4l changed the title Add free and billable rollover execution minutes Add extra and gifted execution minutes Nov 15, 2023
backend/btrixcloud/orgs.py Outdated Show resolved Hide resolved
backend/btrixcloud/orgs.py Outdated Show resolved Hide resolved
@Shrinks99
Copy link
Member

Getting a runtime error on this branch, ran yarn to make sure I was up to date but still no success:

ERROR
Expected a finite number
module.exports@webpack-internal:///./node_modules/pretty-ms/index.js:10:9
Dashboard/this.formatExecutionSeconds/<@webpack-internal:///./src/pages/org/dashboard.ts:58:196
when@webpack-internal:///./node_modules/lit-html/development/directives/when.js:11:24
Dashboard/this.formatExecutionSeconds@webpack-internal:///./src/pages/org/dashboard.ts:58:69
renderCrawlingMeter/<@webpack-internal:///./src/pages/org/dashboard.ts:452:28
when@webpack-internal:///./node_modules/lit-html/development/directives/when.js:11:91
renderCrawlingMeter@webpack-internal:///./src/pages/org/dashboard.ts:441:71
render/<@webpack-internal:///./src/pages/org/dashboard.ts:235:22
renderCard/<@webpack-internal:///./src/pages/org/dashboard.ts:503:107
when@webpack-internal:///./node_modules/lit-html/development/directives/when.js:11:24
renderCard@webpack-internal:///./src/pages/org/dashboard.ts:503:73
render@webpack-internal:///./src/pages/org/dashboard.ts:234:18
update@webpack-internal:///./node_modules/lit-element/development/lit-element.js:138:28
performUpdate@webpack-internal:///./node_modules/@lit/reactive-element/development/reactive-element.js:831:22
scheduleUpdate@webpack-internal:///./node_modules/@lit/reactive-element/development/reactive-element.js:766:21
__enqueueUpdate@webpack-internal:///./node_modules/@lit/reactive-element/development/reactive-element.js:739:29

In the screenshots the inner shadow which helps the user discern the difference between the filled and unfilled part of the bar graph appears to be missing from the coloured backgrounds. Out of context, a completely unfilled graph might look more filled than we want it to without this!

Screenshot 2023-11-16 at 1 20 12 PM

For the tooltips, it looks like you've made discrete tooltips for each section (filled and unfilled) whereas in the mockups I had them as one so users could easily compare how many minutes in each section they had used? Open to other opinions if you have a reason this method is better.

I think I should have made a comment about this in the Figma, sorry :(

Screenshot 2023-11-16 at 1 40 09 PM

@tw4l
Copy link
Member Author

tw4l commented Nov 16, 2023

Getting a runtime error on this branch, ran yarn to make sure I was up to date but still no success: ...

Looking into it!

In the screenshots the inner shadow which helps the user discern the difference between the filled and unfilled part of the bar graph appears to be missing from the coloured backgrounds. Out of context, a completely unfilled graph might look more filled than we want it to without this!

Ah, I missed that and you're entirely right! Will add in a follow-up commit.

For the tooltips, it looks like you've made discrete tooltips for each section (filled and unfilled) whereas in the mockups I had them as one so users could easily compare how many minutes in each section they had used? Open to other opinions if you have a reason this method is better.

I think I should have made a comment about this in the Figma, sorry :(

No reason to be sorry! This was just a holdover from a previous iteration and I hadn't noticed the difference in the mockups! Happy to change

@tw4l
Copy link
Member Author

tw4l commented Nov 20, 2023

Blocked on #1386, which refactors a helper used extensively in this PR. Will wait for that to be merged first.

Update: merged and rebased

frontend/src/pages/org/dashboard.ts Outdated Show resolved Hide resolved
@tw4l tw4l force-pushed the issue-1358-additional-minutes branch 2 times, most recently from 21b2a0d to f86abe0 Compare November 22, 2023 18:05
@tw4l
Copy link
Member Author

tw4l commented Nov 22, 2023

@Shrinks99 @emma-sg Branch has been updated to resolve review comments, including:

  • Rebase on latest humanizeExecutionSeconds work
  • Add box shadow for remaining time bars
  • Fix yarn runtime error
  • Display single tooltip for used/remaining sections of each type of execution time when either extra or gifted execution minute quotas are enabled

Updated screenshots

No extra/gifted execution minute quotas (similar behavior as before)

Screen Shot 2023-11-22 at 1 19 01 PM Screen Shot 2023-11-22 at 1 19 06 PM Screen Shot 2023-11-22 at 1 19 11 PM

Extra/gifted execution minute quotas added

Screen Shot 2023-11-22 at 1 19 26 PM

(Single tooltip per type of execution minutes, displays if cursor is over used or remaining, centering on used if present)

Screen Shot 2023-11-22 at 1 19 30 PM Screen Shot 2023-11-22 at 1 19 35 PM Screen Shot 2023-11-22 at 1 19 41 PM Screen Shot 2023-11-22 at 1 34 29 PM Screen Shot 2023-11-22 at 1 44 15 PM

@tw4l tw4l requested a review from emma-sg November 22, 2023 18:40
@tw4l
Copy link
Member Author

tw4l commented Nov 22, 2023

And one additional tweak to add right border-radius for execution time used bars if there is still remaining time in that category:

Screen Shot 2023-11-22 at 2 08 39 PM Screen Shot 2023-11-22 at 1 53 08 PM

@tw4l tw4l force-pushed the issue-1358-additional-minutes branch from 4de5eed to f2dc7d6 Compare November 22, 2023 20:52
@tw4l
Copy link
Member Author

tw4l commented Nov 22, 2023

Right border-radius now fixed so that the quota bar shows underneath without any white pixels:

Screen Shot 2023-11-22 at 3 46 26 PM Screen Shot 2023-11-22 at 3 51 26 PM

Close-up:

Screen Shot 2023-11-22 at 3 54 06 PM

This also made it easier to always center the tooltip on the execution type, rather than just on the used portion, e.g.:

Screen Shot 2023-11-22 at 3 55 31 PM

Copy link
Member

@emma-sg emma-sg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet! Nicely done, all works well as far as I can tell with my poking around

@ikreymer
Copy link
Member

Nice! Looks good overall, but noticed one issue:
When changing the quota for the current month, I noticed that the bar resets to the entire monthly quota being used.
See attachment:
Screenshot 2023-11-27 at 10 30 53 PM

The actual usage is 3m 2s but the meter shows that all 20 mins have been used

tw4l and others added 20 commits December 6, 2023 11:57
Combines used and remaining bars for each type into one
component with a shared tooltip.

Uses traditional meter-bar instead if no extra or gifted time has
been set.
and show short units when style is short
- Explicitly set giftedExecSecondsAvailable to 0 if appropriate
- Use min of seconds still over quota or available extra seconds
for calculating extraExecSeconds usage
- Make crawlExecSeconds total
- Add monthlyExecSeconds
- Add migration to copy previous crawlExecSeconds to monthly, since
other categories didn't exist before now
- Don't let quota updates result in negative time available, set to
0 instead when applicable
@tw4l tw4l force-pushed the issue-1358-additional-minutes branch from cc16635 to d666680 Compare December 6, 2023 17:00
@tw4l
Copy link
Member Author

tw4l commented Dec 6, 2023

Let's update the logic so that monthlyExecSeconds <= monthly quota. That means if there's no monthly quota, it is not incremented, and otherwise only incremented up to the quota, eg. if there's a quota of 6000 seconds but actual shutdown takes 6030 seconds, crawlExecSeconds can go up to 6030 while monthlyExecSeconds will top at 6000.

Done!

@ikreymer ikreymer self-requested a review December 7, 2023 08:09
Copy link
Member

@ikreymer ikreymer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can finally merge this! (@tw4l just want to make sure the PR message is still accurate, if any tweaks should be made before merging)

@tw4l tw4l merged commit be41c48 into main Dec 7, 2023
6 checks passed
@tw4l tw4l deleted the issue-1358-additional-minutes branch December 7, 2023 19:34
tw4l added a commit that referenced this pull request Dec 12, 2023
Fixes #1358 

- Adds `extraExecMinutes` and `giftedExecMinutes` org quotas, which are
not reset monthly but are updateable amounts that carry across months
- Adds `quotaUpdate` field to `Organization` to track when quotas were
updated with timestamp
- Adds `extraExecMinutesAvailable` and `giftedExecMinutesAvailable`
fields to `Organization` to help with tracking available time left
(includes tested migration to initialize these to 0)
- Modifies org backend to track time across multiple categories, using
monthlyExecSeconds, then giftedExecSeconds, then extraExecSeconds.
All time is also written into crawlExecSeconds, which is now the monthly
total and also contains any overage time above the quotas
- Updates Dashboard crawling meter to include all types of execution
time if `extraExecMinutes` and/or `giftedExecMinutes` are set above 0
- Updates Dashboard Usage History table to include all types of
execution time (only displaying columns that have data)
- Adds backend nightly test to check handling of quotas and execution
time
- Includes migration to add new fields and copy crawlExecSeconds to
monthlyExecSeconds for previous months

Co-authored-by: emma <[email protected]>
tw4l added a commit that referenced this pull request Dec 19, 2023
Fixes #1358 

- Adds `extraExecMinutes` and `giftedExecMinutes` org quotas, which are
not reset monthly but are updateable amounts that carry across months
- Adds `quotaUpdate` field to `Organization` to track when quotas were
updated with timestamp
- Adds `extraExecMinutesAvailable` and `giftedExecMinutesAvailable`
fields to `Organization` to help with tracking available time left
(includes tested migration to initialize these to 0)
- Modifies org backend to track time across multiple categories, using
monthlyExecSeconds, then giftedExecSeconds, then extraExecSeconds.
All time is also written into crawlExecSeconds, which is now the monthly
total and also contains any overage time above the quotas
- Updates Dashboard crawling meter to include all types of execution
time if `extraExecMinutes` and/or `giftedExecMinutes` are set above 0
- Updates Dashboard Usage History table to include all types of
execution time (only displaying columns that have data)
- Adds backend nightly test to check handling of quotas and execution
time
- Includes migration to add new fields and copy crawlExecSeconds to
monthlyExecSeconds for previous months

Co-authored-by: emma <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Rollover Minutes Quotas / Additional minutes reporting
4 participants