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: fetch DynamoDB costs for RCUs and WCUs #1111

Merged
merged 14 commits into from
Oct 19, 2023
Merged

Conversation

Traxmaxx
Copy link
Contributor

@Traxmaxx Traxmaxx commented Oct 17, 2023

Problem

We have no costs on DynamoDB resources

Solution

Fetch costs of DynamoDB resources

Changes Made

  • Extend aws:dynamodb:tables.go to fetch RCUs and WCUs through table details
  • Add test for util function

How to Test

Util can be tested with

go test ./providers/aws/dynamodb/

// should print
// ok  	github.com/tailwarden/komiser/providers/aws/dynamodb	0.290s

Otherwise start komiser locally and see if DynamoDB costs are being fetched properly.

Screenshot

Screenshot 2023-10-19 at 15 58 02

Notes

First time doing Go. Feel free to give any tips on what to improve!

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

@Traxmaxx Traxmaxx marked this pull request as ready for review October 17, 2023 18:34
providers/aws/dynamodb/tables.go Outdated Show resolved Hide resolved
providers/aws/dynamodb/tables.go Outdated Show resolved Hide resolved
Comment on lines +27 to +28
// there is something strange going on when using pricing client with regions other than us-east-1
// https://discord.com/channels/932683789384183808/1117721764957536318/1162338171435090032
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this mean there could be potentially different prices returned per region? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

At the same place they say

The AWS Region is the API endpoint for the Price List Query API. The endpoints aren't related to product or service attributes.

So, IDTS but it's worth checking out

Copy link
Collaborator

Choose a reason for hiding this comment

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

evem the cost explorer gives only one endpoint https://docs.aws.amazon.com/cost-management/latest/userguide/ce-api.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I talked with @mlabouardy and I will create a helper to take care of this in a new PR in the next days.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it's worth fixing as the refactor work @AvineshTripathi is doing will probably address this as well. Since the client will be created only once.

Copy link
Contributor Author

@Traxmaxx Traxmaxx Oct 19, 2023

Choose a reason for hiding this comment

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

I don't think it's worth fixing as the refactor work @AvineshTripathi is doing will probably address this as well. Since the client will be created only once.

Awesome, good to know thanks for the heads-up! I think the PR is good to go from my side if you agree as well. Thanks a lot for the thorough review!

Comment on lines 99 to 100
RCUCharges := awsUtils.GetCost(priceMap["AWS-DynamoDB-ProvisionedReadCapacityUnits"], awsUtils.Int64PtrToFloat64(provisionedRCUs))
PWUCharges := awsUtils.GetCost(priceMap["AWS-DynamoDB-ProvisionedWriteCapacityUnits"], awsUtils.Int64PtrToFloat64(provisionedWCUs))
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 these keys for priceMap?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the correct ones are DDB-ReadUnits and DDB-WriteUnits

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: we should use WCU instead of PWU to keep consistency with AWS docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Azanul got them from metrics docs https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/metrics-dimensions.html and from other sources as well. Where did you find the values you mentioned above?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The link is for metrics reference.
If you log the pricing output which is used to create the price map, you'll find the key values I provided

Copy link
Contributor Author

@Traxmaxx Traxmaxx Oct 19, 2023

Choose a reason for hiding this comment

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

Nice catch, works better and actually returns values now for some entities. Will log it from now on to make sure! It's weird that there is no error being returned though 🤔

Screenshot 2023-10-19 at 15 58 02

@Traxmaxx Traxmaxx requested a review from Azanul October 19, 2023 16:00
@mlabouardy mlabouardy added this to the v3.1.2 milestone Oct 19, 2023
@mlabouardy mlabouardy merged commit 0b7166f into develop Oct 19, 2023
3 checks passed
@mlabouardy mlabouardy deleted the feature/tech-1702 branch October 19, 2023 19:09
roblambell added a commit to roblambell/komiser that referenced this pull request Jan 8, 2024
Azanul pushed a commit that referenced this pull request Jan 9, 2024
* feat: add missing dynamodb action to policy.json

describe table has been needed since #1111

* feat: add missing lambda action to policy.json

list event source mappings has been needed since #1040

* feat: add missing iam action to policy.json

list users has been needed since #991

* fix: correct s3 permission for list buckets api call

listbucket iam action is for the contents of a bucket

* feat: add missing code suite actions to policy.json

needed since #1216, #1229, and #1228
Azanul pushed a commit that referenced this pull request Jan 9, 2024
* feat: add missing dynamodb action to policy.json

describe table has been needed since #1111

* feat: add missing lambda action to policy.json

list event source mappings has been needed since #1040

* feat: add missing iam action to policy.json

list users has been needed since #991

* fix: correct s3 permission for list buckets api call

listbucket iam action is for the contents of a bucket

* feat: add missing code suite actions to policy.json

needed since #1216, #1229, and #1228
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.

4 participants