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

Paula/add ebs pricing metrics #240

Merged
merged 25 commits into from
Jul 26, 2024
Merged

Conversation

paulajulve
Copy link
Contributor

@paulajulve paulajulve commented Jul 12, 2024

re #235

This PR:

  • renames struct StructuredPriceMap into ComputePriceMap, to tell it apart from the new StoragePricingMap
  • refactors the AWS collector to extract bits of the Collect function into smaller more testable ones

Next steps, out of the scope of this PR:

  • revamp the old tests for the collector that test compute only
  • populate compute and storage pricing maps async in a background process

This code emits metrics in this format:

# HELP cloudcost_aws_ec2_persistent_volume_usd_per_hour The cost of an AWS EBS Volume in USD.
# TYPE cloudcost_aws_ec2_persistent_volume_usd_per_hour gauge
cloudcost_aws_ec2_persistent_volume_usd_per_hour{availability_zone="ca-central-1a",disk="vol-<id>",persistent_volume="pvc-<pvc>,region="ca-central-1",size_gib="300",state="available",type="gp2"} 33

The labels aren't necessarily the final ones we'll settle on, but rather a starting point to see how those serve us. Naming is subject to change too.

@paulajulve paulajulve force-pushed the paula/add-ebs-pricing-metrics branch 2 times, most recently from 5f47120 to da59c8c Compare July 12, 2024 15:45
pkg/aws/ec2/ec2.go Outdated Show resolved Hide resolved
pkg/aws/ec2/storage.go Outdated Show resolved Hide resolved
pkg/aws/ec2/storage.go Outdated Show resolved Hide resolved
pkg/aws/ec2/storage.go Outdated Show resolved Hide resolved
pkg/aws/ec2/pricing_map.go Outdated Show resolved Hide resolved
pkg/aws/ec2/pricing_map.go Outdated Show resolved Hide resolved
pkg/aws/ec2/pricing_map.go Outdated Show resolved Hide resolved
@paulajulve paulajulve force-pushed the paula/add-ebs-pricing-metrics branch 2 times, most recently from 36d1ebd to 1d32a5e Compare July 18, 2024 16:10
@paulajulve paulajulve self-assigned this Jul 18, 2024
@paulajulve
Copy link
Contributor Author

I've organised the changes into two more meaningful commits to make final review easier. First one is just a no-op refactor of the old code to prepare it to fit the new bits. Second one adds all the new functionality.

I think I've addressed all previous comments. This code is now ready to review 🎉

@paulajulve paulajulve marked this pull request as ready for review July 18, 2024 16:16
pkg/aws/ec2/ec2_test.go Outdated Show resolved Hide resolved
@paulajulve paulajulve requested review from Pokom and logyball July 18, 2024 16:27
Copy link
Contributor

@logyball logyball left a comment

Choose a reason for hiding this comment

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

Ok I have to sign off for the day, but I think this should be a sizeable chunk... :)

Main things:

  • This is an awesome start. All the building blocks are there, it just needs a little bit of tweaking
  • The biggest thing that I noticed was that we should not be creating as many context objects, rather prefer to pass them into functions from their "parent", usually the EC2 or AWS Collector
  • Current cost of volumes appears to be in USD/mo rather than USD/hr
  • Implement locking when writing to the StorageMap
  • Implement a logger on the StorageMap so we can avoid log.Print

docs/metrics/aws/ec2.md Outdated Show resolved Hide resolved
pkg/aws/ec2/ec2.go Outdated Show resolved Hide resolved
pkg/aws/ec2/ec2.go Outdated Show resolved Hide resolved
pkg/aws/ec2/ec2.go Outdated Show resolved Hide resolved
pkg/aws/ec2/ec2.go Outdated Show resolved Hide resolved
pkg/aws/ec2/ec2.go Outdated Show resolved Hide resolved
pkg/aws/ec2/ec2.go Outdated Show resolved Hide resolved
pkg/aws/ec2/ec2.go Outdated Show resolved Hide resolved
pkg/aws/ec2/pricing_map.go Show resolved Hide resolved
pkg/aws/ec2/pricing_map.go Outdated Show resolved Hide resolved
@paulajulve paulajulve force-pushed the paula/add-ebs-pricing-metrics branch 3 times, most recently from 553b3ce to 9d4e733 Compare July 22, 2024 15:12
@paulajulve paulajulve requested a review from logyball July 24, 2024 07:30
Copy link
Contributor

@logyball logyball left a comment

Choose a reason for hiding this comment

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

Nice progress! I think with just a few touch-ups, we should be good here!

Note: I'm going to continue to noodle on the waitgroup test + channels closing and see if I can figure out something useful there.

pkg/aws/ec2/ec2.go Outdated Show resolved Hide resolved
pkg/aws/ec2/ec2.go Outdated Show resolved Hide resolved
pkg/aws/ec2/ec2.go Show resolved Hide resolved
pkg/aws/ec2/ec2.go Outdated Show resolved Hide resolved
pkg/aws/ec2/ec2.go Outdated Show resolved Hide resolved
pkg/aws/ec2/disk_test.go Outdated Show resolved Hide resolved
pkg/aws/ec2/ec2_test.go Show resolved Hide resolved
pkg/aws/ec2/ec2_test.go Show resolved Hide resolved
pkg/aws/ec2/ec2_test.go Show resolved Hide resolved
pkg/aws/ec2/ec2_test.go Outdated Show resolved Hide resolved
pkg/aws/ec2/ec2_test.go Outdated Show resolved Hide resolved
@paulajulve paulajulve force-pushed the paula/add-ebs-pricing-metrics branch from 98701bf to 7d79ceb Compare July 26, 2024 08:17
@paulajulve paulajulve requested a review from a team as a code owner July 26, 2024 08:17
@paulajulve paulajulve requested a review from logyball July 26, 2024 13:08
pkg/aws/ec2/ec2_test.go Show resolved Hide resolved
@paulajulve paulajulve force-pushed the paula/add-ebs-pricing-metrics branch from 5f219e3 to cb43f60 Compare July 26, 2024 13:26
@paulajulve paulajulve merged commit bbe5961 into main Jul 26, 2024
2 checks passed
@paulajulve paulajulve deleted the paula/add-ebs-pricing-metrics branch July 26, 2024 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants