Get disk pricing working #118

Pokom · 2024-02-22T20:09:11Z

This implements a ListDisks method in gke that is then used to list out disks for the GKE projects by zone.
ListDisks will currently return a raw compute.Disk struct that is filtered by
2. Disks only associated with GKE clusters

Relates to Persistent Volume Hourly Cost Metric #95
refs [cloudcost-exporter]: Road to open source #5

This implements a `ListDisks` method in `gke` that is then used to list out disks for the GKE projects by zone. `ListDisks` will currently return a raw `compute.Disk` struct that is filtered by 1. Used disks 2. Disks only associated with GKE clusters The exported price is assuming the disks costs $0.05/GiB/Month and does not account for the type of disk.

Starts to updating compute pricing_map to also take into account storage. Still need to add regex's to capture the data. Added documentation for the pvc metrics.

Pokom · 2024-03-19T18:33:27Z

pkg/google/compute/pricing_map.go

-	"Extreme PD Capacity",
-	"Storage PD Capacity",


Removes these because we explicitly need them now.

Pokom · 2024-03-19T18:33:58Z

pkg/google/compute/pricing_map.go

-	price, err := getPricingInfoFromSku(sku)
-	if err != nil {
-		return nil, PricingDataIsOff
-	}
+


Moved this into the respective code blocks for compute and storage parsing.

the-it

Had a first run on it (second view + local testing will follow). Should we implemend also a metric for compute, as a pure VM can also have persistent disks?

docs/metrics/gcp/persistentvolumes.md

the-it · 2024-03-21T13:49:24Z

pkg/google/compute/pricing_map.go

+)
+
+const (
+	hoursInMonth = 730.5


Is it possible we have this value already in another module?

We do, but I don't know if we have one common spot where we can introduce variables like this. Do you have any thoughts?

create something like pkg/utils/consts.go?

to put magic numbers in

This feels too much of a magic number. Perhaps something like the following makes it easier to maintain in the future:

Suggested change

hoursInMonth = 730.5

// hoursInMonth is the average of hours in a month, computed as

hoursInMonth = 24.35 * 30

I like pkg/utils/consts.go

Let's do it

Moved over on latest push. Thanks all!

inkel

So far LGTM! Left a few comments, happy to discuss them 😸

inkel · 2024-03-21T19:30:41Z

pkg/google/compute/pricing_map.go

+)
+
+const (
+	hoursInMonth = 730.5


This feels too much of a magic number. Perhaps something like the following makes it easier to maintain in the future:

Suggested change

hoursInMonth = 730.5

// hoursInMonth is the average of hours in a month, computed as

hoursInMonth = 24.35 * 30

inkel · 2024-03-21T19:32:43Z

pkg/google/compute/pricing_map.go

@@ -140,43 +172,68 @@ func GeneratePricingMap(skus []*billingpb.Sku) (*StructuredPricingMap, error) {
 		rawData, err := getDataFromSku(sku)

 		if errors.Is(err, SkuNotRelevant) {
-			log.Printf("%v: %s", SkuNotRelevant, sku.Description)
+			//log.Printf("%v: %s", SkuNotRelevant, sku.Description)


Nitpick: remove commented out lines.

pkg/google/compute/pricing_map.go

scripts/gcp-fetch-skus/gcp-fetch-skus.go

logyball

Nice work! This is really cool stuff and overall works quite well.

I have a couple of NIT comments as well as questions. There's one thing that I think should absolutely be addressed (case where region is populated on the disk and spits out something like us-central1-f, resulting in missing the disk.

However, this is very impressive stuff. Let me know when you have a chance to take a look!

pkg/google/billing/billing.go

logyball · 2024-03-21T17:44:51Z

pkg/google/compute/pricing_map.go

+)
+
+const (
+	hoursInMonth = 730.5


create something like pkg/utils/consts.go?

to put magic numbers in

pkg/google/compute/pricing_map.go

pkg/google/compute/pricing_map_test.go

pkg/google/gke/gke.go

pkg/google/gke/gke_test.go

inkel

Getting nicer and nicer! FWIW, LGTM.

pkg/google/compute/pricing_map.go

pkg/google/gke/gke.go

pkg/utils/consts.go

pkg/google/gke/gke_test.go

Co-authored-by: Leandro López <[email protected]>

logyball

LGTM! Nice work.

As for the copying versus importing, I can see there being other use cases for the const package, but I also tend to agree with the "do the single thing" approach, and a "const" file is doing many things. But I also don't always agree with the programming dogmatism (my opinion here) that utility classes/functions/variables/etc are evil, as they often make it easier to not forget to update something in more than one place as a project grows.

That being said, unit and other tests are probably the things that we should rely on here rather than programming anti-patterns, even if you don't agree with them always being anti-patterns :D

Pokom added 9 commits February 22, 2024 14:59

Start collecting pricing data and document it

3756ae5

Starts to updating compute pricing_map to also take into account storage. Still need to add regex's to capture the data. Added documentation for the pvc metrics.

Implement script to fetch skus

4f97e29

Implement parsing STorage skus

a21aacc

Implement disk tests in gke and fix broken tests

a4d1ed6

Fix linter mistakes

b2857cf

refactor gcp-fetch-skus to have some semblence of reusability

c049b51

Remove unused code in script

2fdb16a

Minor cleanup

12f464f

Pokom commented Mar 20, 2024

View reviewed changes

logyball self-requested a review March 20, 2024 17:01

Pokom added 4 commits March 20, 2024 14:53

Implement fetching namespace from the disks

011443b

Update pricing_map tests to use const value for hours in a month

7978b1f

Merge branch 'main' into feat/gke-pvcs

c6cc750

Fix linting errors

d83be9c

Pokom marked this pull request as ready for review March 20, 2024 19:13

Pokom requested a review from the-it March 20, 2024 19:40

the-it reviewed Mar 21, 2024

View reviewed changes

Update documentation around persistent volumes to merge it with GKE

6309b51

inkel reviewed Mar 21, 2024

View reviewed changes

logyball requested changes Mar 21, 2024

View reviewed changes

Pokom added 2 commits March 21, 2024 16:19

Implement PR review suggestions

d94b1c8

Unbreak the build

a3ffaa0

inkel approved these changes Mar 22, 2024

View reviewed changes

pkg/google/compute/pricing_map.go Show resolved Hide resolved

pkg/google/gke/gke.go Show resolved Hide resolved

pkg/google/gke/gke.go Show resolved Hide resolved

Fix issue parsing out disks that were specific to a zone

ac4112f

Pokom requested a review from logyball March 22, 2024 14:17

Move HoursInMonth to dedicated consts.go package

8eae7ad

inkel reviewed Mar 22, 2024

View reviewed changes

pkg/utils/consts.go Outdated Show resolved Hide resolved

pkg/google/gke/gke_test.go Outdated Show resolved Hide resolved

pkg/google/gke/gke_test.go Outdated Show resolved Hide resolved

Update pkg/google/gke/gke_test.go

743babe

Co-authored-by: Leandro López <[email protected]>

logyball approved these changes Mar 22, 2024

View reviewed changes

Use backticks instead of escaping quotas in test json

30d9e51

Pokom merged commit 47f5228 into main Mar 22, 2024
1 check passed

Pokom deleted the feat/gke-pvcs branch March 22, 2024 19:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get disk pricing working #118

Get disk pricing working #118

Pokom commented Feb 22, 2024 •

edited

Loading

Pokom Mar 19, 2024

Pokom Mar 19, 2024

the-it left a comment

the-it Mar 21, 2024

Pokom Mar 21, 2024

logyball Mar 21, 2024

inkel Mar 21, 2024

the-it Mar 22, 2024

Pokom Mar 22, 2024

Pokom Mar 22, 2024

inkel left a comment

inkel Mar 21, 2024

inkel Mar 21, 2024

logyball left a comment

logyball Mar 21, 2024

inkel left a comment

logyball left a comment

	hoursInMonth = 730.5
	// hoursInMonth is the average of hours in a month, computed as
	hoursInMonth = 24.35 * 30

Get disk pricing working #118

Get disk pricing working #118

Conversation

Pokom commented Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

the-it left a 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

Choose a reason for hiding this comment

inkel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logyball left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inkel left a comment

Choose a reason for hiding this comment

logyball left a comment

Choose a reason for hiding this comment

Pokom commented Feb 22, 2024 •

edited

Loading