Skip to content

[usage] List cost centers with expired billing time #14229

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 27, 2022
Merged

[usage] List cost centers with expired billing time #14229

merged 1 commit into from
Oct 27, 2022

Conversation

easyCZ
Copy link
Member

@easyCZ easyCZ commented Oct 27, 2022 •
edited
Loading

Description

  • Implements ResetUsage to initially only report how many Cost centers it finds with expired NextBillingTime
  • Implements underlying SQL search
    • Tested SQL query against prod. On the full CostCenter dataset this takes ~4 secs.
    • We can add an index on NextBillingTime to speed this up
  • This RPC doesn't yet get invoked by the ResetUsage Job, to test, we'll invoke it manually through grpcurl.

Related Issue(s)

How to test

Release Notes

NONE

Documentation

Werft options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide

@easyCZ easyCZ marked this pull request as ready for review October 27, 2022 11:37
@easyCZ easyCZ requested a review from a team October 27, 2022 11:37
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Oct 27, 2022
svenefftinge
svenefftinge reviewed Oct 27, 2022
View reviewed changes
components/usage/pkg/db/cost_center.go Outdated
Where("billingStrategy = ?", strategy).
Group("id").
Where("nextBillingTime != ?", "").
Where("nextBillingTime < ?", TimeToISO8601(billingTimeBefore))
Copy link
Member

Choose a reason for hiding this comment

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

All conditions need to be applied to the main query, otherwise we wouldn't necessarily get the latest cost center per attributionid.

Copy link
Member Author

@easyCZ easyCZ Oct 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We do the join ON (id, creationTime) so we should get only the most recent one, due to the MAX(createdTime). The unit test in this PR also looks to work.

Copy link
Member

Choose a reason for hiding this comment

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

I think the subselect doesn't fetch the latest version of each cost center, but the latest one fulfilling the conditions. So if say I change to stripe it would pick the outdated version where my cost center was other. Likewise if you test with firstCreation.Add(12 * time.Hour), that is a point between first and second, your query would return the outdated costcenter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will add a test for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for flagging, you indeed spotted this correctly. I've now added a test to validate this as well.

@easyCZ
Copy link
Member Author

easyCZ commented Oct 27, 2022

/hold for adding a test and adjusting Query

@easyCZ easyCZ force-pushed the mp/usage-list-expired-cost-centers branch from 2cebdd6 to 4578dae Compare October 27, 2022 14:33
@easyCZ easyCZ force-pushed the mp/usage-list-expired-cost-centers branch from 4578dae to c4795b3 Compare October 27, 2022 14:38
@easyCZ
Copy link
Member Author

easyCZ commented Oct 27, 2022

Query adjusted and unit test added
/unhold

@easyCZ easyCZ requested a review from a team October 27, 2022 14:44
@roboquat roboquat merged commit 2ec4546 into main Oct 27, 2022
@roboquat roboquat deleted the mp/usage-list-expired-cost-centers branch October 27, 2022 14:56
@easyCZ easyCZ mentioned this pull request Oct 27, 2022
Closed
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@svenefftinge svenefftinge svenefftinge approved these changes

Assignees
No one assigned
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/L team: webapp Issue belongs to the WebApp team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@easyCZ @svenefftinge @roboquat