feat: Fix Cost Calculation for AWS CloudFront Distributions & Functions #1131

bishal7679 · 2023-10-23T17:13:34Z

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, origin-shield-request 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 & function objects using CloudFront client
also fetching lambda-edge-duration, lambda-edge-requests, origin-shield-request 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

REF :- #1083

Signed-off-by: bishal7679 <[email protected]>

bishal7679 · 2023-10-23T17:25:15Z

@AvineshTripathi @Azanul
PHAL!
Now all checks passed 👍

AvineshTripathi · 2023-10-23T18:44:14Z

providers/aws/cloudfront/distributions.go


-			// calculate region data transfer out to origin
-			dataTransferToOrigin := (bytesDownloaded / 1000000000) * 0.02
+			dataTransferToOriginCost := awsUtils.GetCost(priceMap["AWS-CloudFront-DataTransfer-Out-Bytes"], (float64(bytesDownloaded) / 1099511627776)*1024)


Just a thought it would be better to declare a var for static values for better readability for others who might take ref of your work

providers/aws/cloudfront/functions.go

providers/aws/cloudfront/distributions.go

Azanul

The priceMap keys are incorrect, please fix those. Log the pricing output to find the correct keys.

Cloudfront distribution cost is Data transfer out cost + request cost
Check out the pricing examples in https://aws.amazon.com/cloudfront/pricing/

providers/aws/cloudfront/functions.go

bishal7679 · 2023-10-24T05:20:27Z

The priceMap keys are incorrect, please fix those. Log the pricing output to find the correct keys.

Cloudfront distribution cost is Data transfer out cost + request cost Check out the pricing examples in https://aws.amazon.com/cloudfront/pricing/

But I didn't change anything on this. It was included cost = upload + download + request
Also help me to get the correct keys. I am unable to find bcoz pricing output doesn't fetch those

Kolawole99 · 2023-10-24T08:09:55Z

providers/aws/cloudfront/functions.go

+
+			lambdaEdgeDurationCost := awsUtils.GetCost(priceMap["AWS-Lambda-Edge-Duration"], lambdaEdgeDuration)
+
+			lambdaEdgeRequestsCost := awsUtils.GetCost(priceMap["AWS-Lambda-Edge-Requests"], lambdaEdgeRequests/10000000)


I think you should declare a constant for 10000000 this should be descriptive so other developers would have an idea of what this calculation achieves.

Azanul · 2023-10-24T08:42:22Z

But I didn't change anything on this. It was included cost = upload + download + request Also help me to get the correct keys. I am unable to find bcoz pricing output doesn't fetch those

Try without the region code filter since for cloudfront from which region the data is fetched is also considered. Here's the full pricing output w/o region code filter.
op.json

Signed-off-by: bishal7679 <[email protected]>

bishal7679 · 2023-10-24T14:09:10Z

@Azanul @AvineshTripathi PTAL now!

Azanul

Cloufroint functions looks good to go from me.

Calculation for distributions requires much more calculation as the implementation here would simply take the last value that parser gets in the list of prices. In short, the location needs to be taken into consideration.

bishal7679 · 2023-10-24T15:44:32Z

Cloufroint functions looks good to go from me.

Calculation for distributions requires much more calculation as the implementation here would simply take the last value that parser gets in the list of prices. In short, the location needs to be taken into consideration.

Could you pls elaborate more
Location what do you mean and do I have to write another metric??

Azanul · 2023-10-24T17:56:37Z

Calculation for distributions requires much more calculation as the implementation here would simply take the last value that parser gets in the list of prices. In short, the location needs to be taken into consideration.

Could you pls elaborate more Location what do you mean and do I have to write another metric??

If you see the pricing output or the cloudfront pricing page, you'll notice that price is location wise. The location is (probably) from location, the location/region of the origin from the data is fetched. To get this I think we'll probably have to map the aws regions to the listed cloudfront regions and calculate accordingly. Someone with more AWS expertise might be able to confirm.
cc: @mlabouardy @jakepage91

jolo-dev · 2023-10-25T20:27:29Z

providers/aws/cloudfront/functions.go

+				StartTime:  aws.Time(utils.BeginningOfMonth(time.Now())),
+				EndTime:    aws.Time(time.Now()),
+				MetricName: aws.String("Duration"),
+				Namespace:  aws.String("AWS/LambdaEdge"),


Is that not aws.String("AWS/Cloudfront")?

@Azanul WDYT??
AWS/Cloudfront or AWS/Lambda would be correct??

AWS/CloudFront
ref: https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/programming-cloudwatch-metrics.html#cloudfront-metrics-global-values

jolo-dev · 2023-10-25T20:28:29Z

providers/aws/cloudfront/functions.go

+				StartTime:  aws.Time(utils.BeginningOfMonth(time.Now())),
+				EndTime:    aws.Time(time.Now()),
+				MetricName: aws.String("Requests"),
+				Namespace:  aws.String("AWS/LambdaEdge"),


Isn't it AWS/Cloudfront?

jolo-dev · 2023-10-25T20:29:13Z

providers/aws/cloudfront/functions.go

+			metricsLambdaEdgeDurationOutput, err := cloudwatchClient.GetMetricStatistics(ctx, &cloudwatch.GetMetricStatisticsInput{
+				StartTime:  aws.Time(utils.BeginningOfMonth(time.Now())),
+				EndTime:    aws.Time(time.Now()),
+				MetricName: aws.String("Duration"),


Does that work? If yes, that would be great 👍

jolo-dev · 2023-10-25T20:29:56Z

providers/aws/cloudfront/functions.go

+			resources = append(resources, Resource{
+				Provider:   "AWS",
+				Account:    client.Name,
+				Service:    "CloudFront",


Shouldn't it be Cloudfront Functions?

Cloudfront functions isn't a service, it's a resource/sub-resource of Cloudfront service

You're right.

providers/aws/cloudfront/functions.go

Azanul · 2023-10-26T09:44:46Z

@bishal7679 I recently uncovered an oversight here. Cloudfront functions and Lambda edge functions are different, the current PR mixes them both. I'm looking for the Lambda edge metrics API, when found, we could use those metrics instead of cloudfront function invocations, change the file name and it'll work out.
Cloudfront functions won't need a pricing client either since the price is static globally.

bishal7679 · 2023-10-26T09:59:30Z

@bishal7679 I recently uncovered an oversight here. Cloudfront functions and Lambda edge functions are different, the current PR mixes them both. I'm looking for the Lambda edge metrics API, when found, we could use those metrics instead of cloudfront function invocations, change the file name and it'll work out. Cloudfront functions won't need a pricing client either since the price is static globally.

Found Lambda@Edge is similar to cloudfront means the functions running on cloudfront edge regionally. Thats might be the reason pricing output is giving Lambda edge results for CloudFront

Azanul · 2023-10-26T10:20:35Z

Found Lambda@Edge is similar to cloudfront means the functions running on cloudfront edge regionally. Thats might be the reason pricing output is giving Lambda edge results for CloudFront

Lambda edge is a feature of cloudfront only but is different from cloudfront functions

mlabouardy · 2023-10-27T09:18:46Z

Found Lambda@Edge is similar to cloudfront means the functions running on cloudfront edge regionally. Thats might be the reason pricing output is giving Lambda edge results for CloudFront

Lambda edge is a feature of cloudfront only but is different from cloudfront functions

I agree, Lambda@Edge is just a Lambda function that's triggered on a CloudFront edge

jolo-dev · 2023-10-27T16:27:39Z

Found Lambda@Edge is similar to cloudfront means the functions running on cloudfront edge regionally. Thats might be the reason pricing output is giving Lambda edge results for CloudFront

Lambda edge is a feature of cloudfront only but is different from cloudfront functions

I agree, Lambda@Edge is just a Lambda function that's triggered on a CloudFront edge

So meaning my PR #1127 is not completely useless? :D

bishal7679 · 2023-10-27T17:41:11Z

Found Lambda@Edge is similar to cloudfront means the functions running on cloudfront edge regionally. Thats might be the reason pricing output is giving Lambda edge results for CloudFront

Lambda edge is a feature of cloudfront only but is different from cloudfront functions

I agree, Lambda@Edge is just a Lambda function that's triggered on a CloudFront edge

So meaning my PR #1127 is not completely useless? :D

How it'd be useless??
@Azanul @AvineshTripathi What to do with #1131 as its getting long convo

Azanul · 2023-10-30T23:49:33Z

I don't think we need to iterate through all the regions every time, we'd need to map only the current region from aws client to the region CF classification.
Let's kill the clutter and take one step at a time. Let's drop the changes in CF distributions till someone has more bandwidth to test it out and check whether our assumptions are correct or not. Instead, let's focus on lambdaedge, it has less room for error.

If you see carefully the region, location loop is not iterating every time Its iterating one region and under that region if only one edge location is matched then it will break the loop and fetch the unit price and again go to another region And you suggested this above comment Since we fetch all resources one region at a time, we can map them to the pricing regions

The location loop is the anti-pattern here. Regions are not handled in every service's file, they're handled in provider's file i.e., aws in this case.

bishal7679 · 2023-10-31T04:34:05Z

I don't think we need to iterate through all the regions every time, we'd need to map only the current region from aws client to the region CF classification.
Let's kill the clutter and take one step at a time. Let's drop the changes in CF distributions till someone has more bandwidth to test it out and check whether our assumptions are correct or not. Instead, let's focus on lambdaedge, it has less room for error.

If you see carefully the region, location loop is not iterating every time Its iterating one region and under that region if only one edge location is matched then it will break the loop and fetch the unit price and again go to another region And you suggested this above comment Since we fetch all resources one region at a time, we can map them to the pricing regions

The location loop is the anti-pattern here. Regions are not handled in every service's file, they're handled in provider's file i.e., aws in this case.

since all corresponding edge location of every region of aws.go getRegion func is not present in pricing output how to map all as map[string]string???
Then it'd not be possible to take the region from aws.go thats why it is taken in the service file itself

Azanul · 2023-10-31T04:44:17Z

If you see carefully the region, location loop is not iterating every time Its iterating one region and under that region if only one edge location is matched then it will break the loop and fetch the unit price and again go to another region And you suggested this above comment Since we fetch all resources one region at a time, we can map them to the pricing regions

The location loop is the anti-pattern here. Regions are not handled in every service's file, they're handled in provider's file i.e., aws in this case.

since all corresponding edge location of every region of aws.go getRegion func is not present in pricing output how to map all as map[string]string??? Then it'd not be possible to take the region from aws.go thats why it is taken in the service file itself

try to increase max results for the pricing output

bishal7679 · 2023-10-31T04:52:44Z

If you see carefully the region, location loop is not iterating every time Its iterating one region and under that region if only one edge location is matched then it will break the loop and fetch the unit price and again go to another region And you suggested this above comment Since we fetch all resources one region at a time, we can map them to the pricing regions

The location loop is the anti-pattern here. Regions are not handled in every service's file, they're handled in provider's file i.e., aws in this case.

since all corresponding edge location of every region of aws.go getRegion func is not present in pricing output how to map all as map[string]string??? Then it'd not be possible to take the region from aws.go thats why it is taken in the service file itself

try to increase max results for the pricing output

Could you try once either? I've removed all the attribute only using serviceCode.

Azanul · 2023-10-31T05:16:25Z

since all corresponding edge location of every region of aws.go getRegion func is not present in pricing output how to map all as map[string]string??? Then it'd not be possible to take the region from aws.go thats why it is taken in the service file itself

try to increase max results for the pricing output

Could you try once either? I've removed all the attribute only using serviceCode.

When setting MaxResults to 100
op.json

bishal7679 · 2023-10-31T05:17:59Z

since all corresponding edge location of every region of aws.go getRegion func is not present in pricing output how to map all as map[string]string??? Then it'd not be possible to take the region from aws.go thats why it is taken in the service file itself

try to increase max results for the pricing output

Could you try once either? I've removed all the attribute only using serviceCode.

When setting MaxResults to 100 op.json

I've taken all the results from your given op.json previously 😃

Azanul · 2023-10-31T05:35:56Z

Could you try once either? I've removed all the attribute only using serviceCode.

When setting MaxResults to 100 op.json

I've taken all the results from your given op.json previously 😃

This is what you get when printing the full paginated result
op.json

bishal7679 · 2023-10-31T07:37:23Z

Could you try once either? I've removed all the attribute only using serviceCode.

When setting MaxResults to 100 op.json

I've taken all the results from your given op.json previously 😃

This is what you get when printing the full paginated result op.json

OK! It is giving 2 extra region. suggest modification in my full distribution code and in the location code as well. Would be happy to apply commit suggestion and get this PR ready to merge!

Azanul · 2023-10-31T08:05:44Z

I've taken all the results from your given op.json previously 😃

This is what you get when printing the full paginated result op.json

OK! It is giving 2 extra region. suggest modification in my full distribution code and in the location code as well. Would be happy to apply commit suggestion and get this PR ready to merge!

#1131 (comment)
It is certainly possible

bishal7679 · 2023-10-31T13:39:01Z

I've taken all the results from your given op.json previously 😃

This is what you get when printing the full paginated result op.json

OK! It is giving 2 extra region. suggest modification in my full distribution code and in the location code as well. Would be happy to apply commit suggestion and get this PR ready to merge!

#1131 (comment) It is certainly possible

Well moving out the entire code out of the two loop (region, location) and Will I be able to get for which region currently fetching distributions by using client.AWSClient.Region
and compare that to the map which I created?
Does it sounds good?

Signed-off-by: bishal7679 <[email protected]>

bishal7679 · 2023-10-31T18:31:58Z

@Azanul Now everything is working nicely!

All region is being taken from provider aws.go and being matched with the map I created and fetching the unit price
Roughly tested it
Now only let me know how you got this op.json max result

pricingOutput, err := pricingClient.GetProducts(ctx, &pricing.GetProductsInput{
		ServiceCode: aws.String("AmazonCloudFront"),
	})

will only by ServiceCode work or need to add another attributes?

bishal7679 · 2023-11-01T15:23:48Z

PTAL!
/cc @Azanul @mlabouardy @AvineshTripathi
@jakepage91

Kolawole99 · 2023-11-01T15:58:35Z

providers/aws/cloudfront/distributions.go

-
-			// calculate region data transfer out to origin
-			dataTransferToOrigin := (bytesDownloaded / 1000000000) * 0.02
+			dataTransferToOriginCost := awsUtils.GetCost(priceMapForDataTransfer[EdgeLocation], (float64(bytesDownloaded)/1099511627776)*1024)


It would be better to use the freeTierUpload variable so anyone can tell by the name what this calculation should do You might also create a variable for 1024 as well.

Kolawole99 · 2023-11-01T16:00:50Z

providers/aws/cloudfront/distributions.go

+
+}
+
+func getRegionMapping() map[string]string {


I think it will be better to have this region mapping in an AWS util file so it can be reused by any AWS service needing to map regions.

Also, we should cross-verify if this does not exist (or in a different format) in the codebase as of now

Yeap you are right but this mapping is cloudfront specific
I'm not sure same edge location will be needed for any other service and same edge locations might not be present in their pricingoutput
WDYT??

Kolawole99 · 2023-11-01T16:06:33Z

@bishal7679 good work you have done with the CloudFront cost calculation

Azanul · 2023-11-02T11:53:19Z

Now only let me know how you got this op.json max result
pricingOutput, err := pricingClient.GetProducts(ctx, &pricing.GetProductsInput{
		ServiceCode: aws.String("AmazonCloudFront"),
	})
will only by ServiceCode work or need to add another attributes?

The result is paginated anyway, so I looped through all the pages (used NextToken) and merged the results into one.

bishal7679 · 2023-11-02T13:54:52Z

Now only let me know how you got this op.json max result
pricingOutput, err := pricingClient.GetProducts(ctx, &pricing.GetProductsInput{
		ServiceCode: aws.String("AmazonCloudFront"),
	})
will only by ServiceCode work or need to add another attributes?
The result is paginated anyway, so I looped through all the pages (used NextToken) and merged the results into one.

Yeah! I see
What to do then??

Azanul · 2023-11-02T15:26:32Z

Yeah! I see What to do then??

Let's use upcoming Komiser weekly to have an exhaustive discussion on AWS Cloudfront and our approach for calculating it's cost.

cc: @mlabouardy @jakepage91

bishal7679 · 2023-11-06T15:06:09Z

@Azanul what's the update on it

Azanul · 2023-11-08T15:36:45Z

@Azanul what's the update on it

Will soon give an update on this, we might pivot from our current cost approach for AWS support

Azanul · 2023-11-16T05:20:54Z

@bishal7679 We're going to go with cost explorer API integration. It's safe to close this PR

bishal7679 · 2023-11-16T06:13:42Z

@bishal7679 We're going to go with cost explorer API integration. It's safe to close this PR

Its been three times closing same PR after a lot of consuming time
But no issue 😐
get it done by cost explorer 👍

Azanul · 2023-11-16T06:16:02Z

@bishal7679 We're going to go with cost explorer API integration. It's safe to close this PR

Its been three times closing same PR after a lot of consuming time But no issue 😐 get it done by cost explorer 👍

Another day in a software developer's life

fixed cost of cloudfront by distribution and function

265b209

Signed-off-by: bishal7679 <[email protected]>

bishal7679 requested review from jakepage91, mlabouardy, ShubhamPalriwala, AvineshTripathi, greghub and Traxmaxx as code owners October 23, 2023 17:13

AvineshTripathi reviewed Oct 23, 2023

View reviewed changes

providers/aws/cloudfront/functions.go Outdated Show resolved Hide resolved

Azanul reviewed Oct 24, 2023

View reviewed changes

providers/aws/cloudfront/distributions.go Outdated Show resolved Hide resolved

Azanul requested changes Oct 24, 2023

View reviewed changes

providers/aws/cloudfront/functions.go Outdated Show resolved Hide resolved

Kolawole99 suggested changes Oct 24, 2023

View reviewed changes

modified pricemap key

ad9362d

Signed-off-by: bishal7679 <[email protected]>

bishal7679 force-pushed the feature/correct-cost-for-cloudfront branch from 4b1ba27 to ad9362d Compare October 24, 2023 14:05

Azanul requested changes Oct 24, 2023

View reviewed changes

bishal7679 mentioned this pull request Oct 25, 2023

(feature/0457-support-amazon-cloudfront-function): Support Amazon Clo… #1127

Merged

4 tasks

jolo-dev reviewed Oct 25, 2023

View reviewed changes

mlabouardy added this to the v3.1.3 milestone Oct 27, 2023

updated cloudfront distribution cost based on locations

8fcfcc7

Signed-off-by: bishal7679 <[email protected]>

bishal7679 force-pushed the feature/correct-cost-for-cloudfront branch from 8f151f1 to 8fcfcc7 Compare October 31, 2023 18:16

bishal7679 and others added 2 commits October 31, 2023 23:50

deleted println created for testing purpose for region

c4a576a

Signed-off-by: bishal7679 <[email protected]>

Merge branch 'develop' into feature/correct-cost-for-cloudfront

4832bb5

Merge branch 'develop' into feature/correct-cost-for-cloudfront

2dc22b6

Kolawole99 suggested changes Nov 1, 2023

View reviewed changes

mlabouardy requested a review from Azanul November 2, 2023 10:44

bishal7679 closed this Nov 16, 2023

bishal7679 deleted the feature/correct-cost-for-cloudfront branch January 4, 2024 03:28


		lambdaEdgeDurationCost := awsUtils.GetCost(priceMap["AWS-Lambda-Edge-Duration"], lambdaEdgeDuration)

		lambdaEdgeRequestsCost := awsUtils.GetCost(priceMap["AWS-Lambda-Edge-Requests"], lambdaEdgeRequests/10000000)

feat: Fix Cost Calculation for AWS CloudFront Distributions & Functions #1131

feat: Fix Cost Calculation for AWS CloudFront Distributions & Functions #1131

Conversation

bishal7679 commented Oct 23, 2023 • edited Loading

Problem #877

Solution

Changes Made

Checklist

bishal7679 commented Oct 23, 2023

Choose a reason for hiding this comment

Azanul left a comment

Choose a reason for hiding this comment

bishal7679 commented Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Azanul commented Oct 24, 2023

bishal7679 commented Oct 24, 2023

Azanul left a comment

Choose a reason for hiding this comment

bishal7679 commented Oct 24, 2023

Azanul commented Oct 24, 2023

Choose a reason for hiding this comment

bishal7679 Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Azanul commented Oct 26, 2023

bishal7679 commented Oct 26, 2023

Azanul commented Oct 26, 2023

mlabouardy commented Oct 27, 2023

jolo-dev commented Oct 27, 2023

bishal7679 commented Oct 27, 2023

Azanul commented Oct 30, 2023

bishal7679 commented Oct 31, 2023

Azanul commented Oct 31, 2023

bishal7679 commented Oct 31, 2023

Azanul commented Oct 31, 2023

bishal7679 commented Oct 31, 2023

Azanul commented Oct 31, 2023

bishal7679 commented Oct 31, 2023

Azanul commented Oct 31, 2023

bishal7679 commented Oct 31, 2023

bishal7679 commented Oct 31, 2023 • edited Loading

bishal7679 commented Nov 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kolawole99 commented Nov 1, 2023 • edited Loading

Azanul commented Nov 2, 2023

bishal7679 commented Nov 2, 2023

Azanul commented Nov 2, 2023

bishal7679 commented Nov 6, 2023

Azanul commented Nov 8, 2023

Azanul commented Nov 16, 2023

bishal7679 commented Nov 16, 2023

Azanul commented Nov 16, 2023

bishal7679 commented Oct 23, 2023 •

edited

Loading

bishal7679 commented Oct 24, 2023 •

edited

Loading

bishal7679 Oct 26, 2023 •

edited

Loading

bishal7679 commented Oct 31, 2023 •

edited

Loading

Kolawole99 commented Nov 1, 2023 •

edited

Loading