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

COST-1075: implement improvements to ranking logic. #2708

Merged
merged 8 commits into from
Mar 25, 2021
Merged

Conversation

blentz
Copy link
Contributor

@blentz blentz commented Mar 5, 2021

This PR fixes a bug that is present for queries that use resolution=daily, as described in COST-1075.

Previously, the behavior of filter[limit]=LIMIT&group_by[GROUP]=* did not constrain the number of groups in the whole response. There would be LIMIT number of elements in each per-day list, but the total number of groups in the response could exceed the LIMIT.

With this change, the response now ensures that only LIMIT number of groups are present in the whole response being returned.

Testing

Azure

Example query: LIMIT=5; curl -s -g "http://localhost:8000/api/cost-management/v1/reports/azure/costs/?filter[limit]=$LIMIT&filter[offset]=0&group_by[subscription_guid]=*&order_by[cost]=desc&end_date=2021-02-28&start_date=2021-02-01&filter[resolution]=daily" | jq '.'

Example output:
azure_query.txt

AWS

Example query: LIMIT=5; curl -s -g "http://localhost:8000/api/cost-management/v1/reports/azure/costs/?filter[limit]=$LIMIT&filter[offset]=0&group_by[region]=*&order_by[cost]=desc&end_date=2021-02-28&start_date=2021-02-01&filter[resolution]=daily" | jq '.'

Example output:
aws_query.txt

OCP + AWS

Example query: LIMIT=5; curl -s -g "http://localhost:8000/api/cost-management/v1/reports/openshift/infrastructures/aws/costs/?filter[limit]=$LIMIT&filter[offset]=0&group_by[project]=*&order_by[cost]=desc&end_date=2021-02-28&start_date=2021-02-01&filter[resolution]=daily" | jq '.'

Example output:
ocp_aws_query.txt

OCP

Test Data: 9 projects

Functional testing:

$ for LIMIT in $(seq 1 10); do curl -s -g "http://localhost:8000/api/cost-management/v1/reports/openshift/costs/?filter[limit]=$LIMIT&filter[offset]=0&group_by[project]=*&order_by[cost]=desc&end_date=2021-02-28&start_date=2021-02-01&filter[resolution]=daily" | jq '[.data[].projects[].project] | unique | length'; done
1
2
3
4
5
6
7
8
9
9

Example query: $ LIMIT=5; curl -s -g "http://localhost:8000/api/cost-management/v1/reports/openshift/costs/?filter[limit]=$LIMIT&filter[offset]=0&group_by[project]=*&order_by[cost]=desc&end_date=2021-02-28&start_date=2021-02-01&filter[resolution]=daily" | jq '.'

Example response:
query_json.txt

@blentz blentz requested a review from a team March 5, 2021 14:45
@blentz blentz self-assigned this Mar 5, 2021
@blentz blentz added the bug Something isn't working label Mar 5, 2021
@codecov
Copy link

codecov bot commented Mar 5, 2021

Codecov Report

Merging #2708 (9ac421f) into master (da715dc) will decrease coverage by 0.0%.
The diff coverage is 95.5%.

@@           Coverage Diff            @@
##           master   #2708     +/-   ##
========================================
- Coverage    94.6%   94.6%   -0.0%     
========================================
  Files         289     289             
  Lines       22484   22490      +6     
  Branches     2571    2583     +12     
========================================
+ Hits        21278   21280      +2     
- Misses        732     733      +1     
- Partials      474     477      +3     

@blentz blentz force-pushed the blentz-wip branch 8 times, most recently from 718795d to 00fed35 Compare March 11, 2021 23:07
@blentz blentz force-pushed the blentz-wip branch 4 times, most recently from 2ef5353 to 6b2af4b Compare March 16, 2021 15:57
@blentz blentz marked this pull request as ready for review March 16, 2021 16:15
dlabrecq
dlabrecq previously approved these changes Mar 16, 2021
Copy link
Contributor

@dlabrecq dlabrecq left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@adberglund adberglund left a comment

Choose a reason for hiding this comment

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

We have a handful of smoke tests failing on this. Essentially when we're returning daily data, and a particular day doesn't have an entry for one of the ranks, we need to add in a zeroed out or blank entry for it. This will be important for the table in the cost explorer so we always have an entry, even if it is 0. Happy to chat about this too.

koku/api/report/queries.py Outdated Show resolved Hide resolved
@blentz blentz force-pushed the blentz-wip branch 3 times, most recently from 8adce38 to fd76f28 Compare March 17, 2021 20:26
@dccurtis
Copy link
Contributor

If everything is set for this one. Lets run smokes locally one more time before merging.

@blentz blentz force-pushed the blentz-wip branch 2 times, most recently from c4c09b3 to 05e0a8a Compare March 18, 2021 20:34
dlabrecq
dlabrecq previously approved these changes Mar 19, 2021
Copy link
Contributor

@dccurtis dccurtis left a comment

Choose a reason for hiding this comment

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

Thanks for slogging through this one Brett!

@blentz blentz merged commit f58ff32 into master Mar 25, 2021
@blentz blentz deleted the blentz-wip branch March 25, 2021 19:38
maskarb added a commit that referenced this pull request Mar 30, 2021
* COST-1110: forecast should not include negative numbers (#2750)

* [COST-1138] Large Delta Percent (#2723)

* percent delta change to return none if previous rounds to 0 at 2 decimals

* make tests use specific rounding cases

* whoops, fix test expected value

* Add ibm.account to RESOURCE_TYPES in RBAC (#2747)

* [COST-1063][COST-1165] More priority cost model updates (#2754)

* Add auto-ingest to docker compose sources
* Use prio queue for all cost model updates
* Refresh costs when deleting a cost model
* Test updates

* Update GCP storage mat views (#2756)

* Misc fixes (#2757)

* try/except refersh view task too
* Update jvm config

* Tail the check migration command (#2759)

* COST-1075: implement improvements to ranking logic. (#2708)

* COST-1075: implement window function changes. fix delta ordering.

* COST-680: Move gcp tag enablement out of development. (#2753)

* COST-680: Move gcp tag enablement out of development.

* Reverse report processing month order (#2755)

* End last month on the end of the month (#2770)

Co-authored-by: Brett Lentz <[email protected]>
Co-authored-by: Corey Goodfred <[email protected]>
Co-authored-by: boaz0 <[email protected]>
Co-authored-by: Andrew Berglund <[email protected]>
Co-authored-by: Douglas Curtis <[email protected]>
Co-authored-by: Cody Myers <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants