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-5728] switch OCP on AWS endpoints to use amortized costs #5393

Merged
merged 6 commits into from
Nov 22, 2024

Conversation

lcouzens
Copy link
Contributor

@lcouzens lcouzens commented Nov 21, 2024

Jira Ticket

COST-5728

Description

This change will switch to amortised cost for the OCP back populate and OCP on ALL tables instead of a coalesce on unblended and savings plans. This is because when we insert into the trino temp tables we normalise all the savings plan and usage costs into a single line. The problem here is we then lose the additional unblended cost from the total value.

lineitem_usagestartdate | coalecse_cost | savingplan_cost | unblended_cost | blended_cost | calculated_amortized_cost | lineitem_lineitemtype
-------------------------+---------------+-----------------+----------------+--------------+---------------------------+-------------------------
2024-11-21 00:00:00.000 | 20.0 | 20.0 | 0.0 | 0.0 | 20.0 | SavingsPlanCoveredUsage
2024-11-21 00:00:00.000 | 10.0 | 0.0 | 10.0 | 10.0 | 10.0 | Usage
(2 rows)

Query 20241121_225310_00541_pvfbe, FINISHED, 1 node
Splits: 1 total, 1 done (100.00%)
0.25 [2 rows, 28.2KiB] [8 rows/s, 114KiB/s]

trino:org1234567> select usage_amount, unblended_cost, savingsplan_effective_cost, calculated_amortized_cost from aws_openshift_daily_resource_matched_temp;
usage_amount | unblended_cost | savingsplan_effective_cost | calculated_amortized_cost
--------------+----------------+----------------------------+---------------------------
11.0 | 10.0 | 20.0 | 30.0
(1 row)

Essentially if we did the coalesce here like sum(coalesce(nullif(savingsplan_effective_cost, 0), unblended_cost)) as unblended_cost this would only work if these lineitemtypes stayed intact. But since we condense them before getting to this sql the coalesce only collects the savings plan cost meaning we DONT account for the additional $10 unblended cost which we should. Switching to amortised here works because in the same trino sql we correctly account for both unblended and savings plan costs before those lines are condensed into a single row.

This is found when you have usage above your savings plan covered usage for the same resource id in a particular day.

Testing

  1. Checkout Branch
  2. Restart Koku
  3. Run regression testing. (Eva also has a reproducing smoke test branch for this too)

Release Notes

  • proposed release note
* [COST-5728](https://issues.redhat.com/browse/COST-5728) Fix savings plan cost using amortised.

@lcouzens lcouzens added the smoke-tests pr_check will build the image and run minimal required smokes label Nov 21, 2024
@lcouzens lcouzens requested review from a team as code owners November 21, 2024 23:04
Copy link

codecov bot commented Nov 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.1%. Comparing base (ad93105) to head (0939e07).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #5393     +/-   ##
=======================================
- Coverage   94.1%   94.1%   -0.0%     
=======================================
  Files        371     371             
  Lines      31549   31549             
  Branches    3385    3385             
=======================================
- Hits       29689   29683      -6     
- Misses      1204    1208      +4     
- Partials     656     658      +2     
---- 🚨 Try these New Features:

@esebesto
Copy link
Contributor

/retest

@esebesto
Copy link
Contributor

I tested this PR and the fix looks good to me!

@lcouzens lcouzens enabled auto-merge (squash) November 22, 2024 08:59
@lcouzens lcouzens disabled auto-merge November 22, 2024 09:11
@lcouzens lcouzens force-pushed the fix_OCP_AWS_calc_amortized_cost branch from 1cab137 to 80bca7b Compare November 22, 2024 09:16
@lcouzens lcouzens enabled auto-merge (squash) November 22, 2024 09:31
@esebesto
Copy link
Contributor

/retest

@esebesto esebesto added full-run-smoke-tests pr_check will build the image and run all smoke tests and removed smoke-tests pr_check will build the image and run minimal required smokes labels Nov 22, 2024
@esebesto
Copy link
Contributor

/retest

@esebesto esebesto added smoke-tests pr_check will build the image and run minimal required smokes and removed full-run-smoke-tests pr_check will build the image and run all smoke tests labels Nov 22, 2024
@esebesto
Copy link
Contributor

/retest

1 similar comment
@esebesto
Copy link
Contributor

/retest

@lcouzens lcouzens merged commit ae819d3 into main Nov 22, 2024
14 checks passed
@lcouzens lcouzens deleted the fix_OCP_AWS_calc_amortized_cost branch November 22, 2024 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
smoke-tests pr_check will build the image and run minimal required smokes smokes-required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants