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

feat: Fix Cost Calculation for AWS CloudFront Distributions #1083

Conversation

bishal7679
Copy link
Contributor

Problem #877

Currently cost for AWS cloudfront is full static and showing wrong.

Solution

This PR is fixing that issue by adding more metrics for lambda-edge-duration, lambda-edge-requests, function-invocations with pricemap fetching.

Changes Made

  • Fetching price info using AWS SDK price module using AmazonCloudFront service code and appropriate filters.
  • getting price map using util function GetPriceMap from utils.go
  • fetching distribution objects using cloudfront client
  • also fetching lambda-edge-duration, lambda-edge-requests, function-invocations metrics using cloudwatch
  • Finally added dynamic calculation as per all the metrics

Checklist

  • Code follows the contributing guidelines
  • Changes have been thoroughly tested
  • Documentation has been updated, if necessary
  • Any dependencies have been added to the project, if necessary

Pricingoutput :-
pricing.json

Signed-off-by: bishal7679 <[email protected]>
@bishal7679
Copy link
Contributor Author

/cc @AvineshTripathi @Azanul

Comment on lines +28 to +33
func ConvertBytesToTerabytes(bytes int64) float64 {
return float64(bytes) / 1099511627776
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't we convert to GB instead? We do the same by multiplying by 1024 to the result

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I thought it would be more efficient not to multiply inline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok i got it

Comment on lines +239 to +285
dataTransferToInternetCost := awsUtils.GetCost(priceMap["CloudFront-DataTransfer-In-Bytes"], sizeInTBUpload*1024)

dataTransferToOriginCost := awsUtils.GetCost(priceMap["CloudFront-DataTransfer-Out-Bytes"], sizeInTBDownload*1024)

requestsCost := awsUtils.GetCost(priceMap["CloudFront-Requests"], requests/10000)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where did you get the keys for priceMap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we keep it static like previous???

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let me see if I can find the correct keys


// calculate requests cost
requestsCost := requests * 0.000001
functionInvocationsCost := awsUtils.GetCost(priceMap["AWS-CloudFront-FunctionInvocation"], functionInvocation)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where did you get this key for priceMap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! updated the code with a function. Searched about it but was not getting the appropriate key. Give your suggestion on the getRate function now

@bishal7679 bishal7679 force-pushed the feature/correct-cost-for-cloudfront branch from f1c8da8 to a96976f Compare October 12, 2023 05:01
@mlabouardy mlabouardy requested a review from Azanul October 12, 2023 09:29
@bishal7679
Copy link
Contributor Author

@Azanul any update??

@Azanul
Copy link
Collaborator

Azanul commented Oct 13, 2023

@Azanul any update??

Yes, I looked into costing of CloudFront. I found out that there are only 3 components to CloudFront cost: CF distributions, CF Functions & CF Origin Shield requests. ref: https://aws.amazon.com/cloudfront/pricing/
Other costs that are being calculated here don't belong here, they belong with their own services such as AWS Lambda.
I'd suggest you look into what a CloudFront distribution is and their review it's cost calculation

@bishal7679
Copy link
Contributor Author

@Azanul any update??

Yes, I looked into costing of CloudFront. I found out that there are only 3 components to CloudFront cost: CF distributions, CF Functions & CF Origin Shield requests. ref: https://aws.amazon.com/cloudfront/pricing/ Other costs that are being calculated here don't belong here, they belong with their own services such as AWS Lambda. I'd suggest you look into what a CloudFront distribution is and their review it's cost calculation

Yes actually I went through it already

@bishal7679
Copy link
Contributor Author

bishal7679 commented Oct 13, 2023

I am including lambda-edge as our pricingoutput is giving those
and what the issue with it? if someone use lambda as edge with cloudfront distribution then they will be charged by some amount otherwise it will remain $0
isnt it?

@Azanul
Copy link
Collaborator

Azanul commented Oct 13, 2023

I am including lambda-edge as our pricingoutput is giving those and what the issue with it? if someone use lambda as edge with cloudfront distribution then they will be charged by some amount otherwise it will remain $0 isnt it?

The issue that you're trying to resolve is for CloudFront Distributions, the file that you're making changes to is for CloudFront Distributions not CloudFront Functions or Origin Shield requests.

if someone use lambda as edge with cloudfront distribution then they will be charged by some amount

That'll be accounted for in it's own resource

@Azanul
Copy link
Collaborator

Azanul commented Oct 13, 2023

I think the respective issue description is misleading as it says we do not have the calculation whereas the calculation is there. The only thing off about the calculation seems to be the hard-coded prices.
cc: @mlabouardy @AvineshTripathi @ShubhamPalriwala

@bishal7679
Copy link
Contributor Author

bishal7679 commented Oct 13, 2023

I think the respective issue description is misleading as it says we do not have the calculation whereas the calculation is there. The only thing off about the calculation seems to be the hard-coded prices. cc: @mlabouardy @AvineshTripathi @ShubhamPalriwala

Yeah I noticed when I took this issue! So should we include all the charges distribution + function. and resolve with this issue only? Otherwise we have to create different issue for those.
WDYT only distribution charges are enough or adding others with it would make it much more efficient @Azanul @mlabouardy @AvineshTripathi

@bishal7679
Copy link
Contributor Author

@jakepage91 Can we add hold label for this PR

@mlabouardy mlabouardy added the aws label Oct 15, 2023
@mlabouardy mlabouardy added this to the v3.1.2 milestone Oct 15, 2023
@mlabouardy
Copy link
Collaborator

mlabouardy commented Oct 19, 2023

@jakepage91 Can we add hold label for this PR

Hey @bishal7679 sorry for jumping late to the thread, is there something I can help with to move forward with your PR?

@AvineshTripathi
Copy link
Collaborator

@mlabouardy we have intentionally kept this on hold to dicuss the changes on cost calculation to avoid repeatative work!

@bishal7679
Copy link
Contributor Author

closing this PR as awsutils recent changes are not being rendered onto my feature/correct-cost-for-cloudformation branch locally [PS:- did git pull & sync]
and raising a new PR

@bishal7679 bishal7679 closed this Oct 21, 2023
@bishal7679 bishal7679 deleted the feature/correct-cost-for-cloudfront branch October 21, 2023 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants