feat(google): Set refresh interval for compute module #59
Conversation
This alters the `collector.Collect` method on the compute module to also check if the pricing map needs to be refreshed. This is a configurable paramater so that folks that want a tighter refresh interval are easily able to do it. - closes #56
pkg/google/compute/pricing_map.go
Outdated
| @@ -106,15 +105,15 @@ func GeneratePricingMap(skus []*billingpb.Sku) (*StructuredPricingMap, error) { | |||
| rawData, err := getDataFromSku(sku) | |||
|
|
|||
| if errors.Is(err, SkuNotRelevant) { | |||
| log.Println(fmt.Errorf("%w: %s", SkuNotRelevant, sku.Description)) | |||
| fmt.Errorf("%w: %s", SkuNotRelevant, sku.Description) | |||
There was a problem hiding this comment.
Does this go to standard output by default? Also would you need a newline at the end now the log.Println() was removed?
There was a problem hiding this comment.
It won't print out by default. I had introduced this change in ce66897#diff-c93c3036217c276fcd4b242ebbbf2e87a902d6f6872530c58e787e89eafd32e4R86 without realizing just how noisy it would become.
Should definitely add a new line, I can follow up with that before merging.
There was a problem hiding this comment.
Ideally we'd have a better handle on log levels and have the ability to increase verbosity to print this out.
| if c.PricingMap == nil { | ||
| start := time.Now() | ||
| log.Printf("Collecting %s metrics", c.Name()) | ||
| if c.PricingMap == nil || time.Now().After(c.NextScrape) { |
There was a problem hiding this comment.
New condition should have a test case, unless it's already covered?
There was a problem hiding this comment.
The first time we call Collect this condition gets covered. I don't have a great way of testing out calling it again and ensuring it doesn't get called 🤔
Maybe I can create a counter to keep track of the number of times PricingMap has been updated and make sure it remains one between calls. How's that sound?
There was a problem hiding this comment.
Hmm, code for the sake of tests sounds bad. Can you make a change to the pricing between calls and assert that the map is stale on second call? Ie it should be the same response on both calls.
There was a problem hiding this comment.
Hacked on something in between my morning's 1:1's and agree this is the better path. I'll have a better test case later today my time. What I like about this approach is I can also force an update by setting the NextScrape time into the past and updating the http test servers response and ensuring the pricing map changes.
There was a problem hiding this comment.
Alright latest commit makes a better attempt at testing the code path for refreshing the pricing map. Ideally I would've changed the response from the cloudCatalogClient but short of creating a second struct, I don't know of an easy way of doing that. There's definitely potential here to improve out we mock out responses, but that's probably best in a separate PR.
pkg/google/compute/compute_test.go
Outdated
| // So that we can test that the pricing map is updated. | ||
| collector.NextScrape = time.Now().Add(-1 * time.Minute) | ||
| err = collector.Collect() | ||
| require.NotSame(t, pricingMap, collector.PricingMap) |
There was a problem hiding this comment.
Maybe this should be NotEqual? iirc NotSame means that the two arguments reference the same object, which could never be true I think (i.e. impossible to fail).
There was a problem hiding this comment.
Argh, that's what I had suspected. Okay, I need to revisit the changing the response for the cloudCatalogClient so that we can different responses. If I set it to NotEqual it fails because they are equivalent. IE, no values changed between runs
There was a problem hiding this comment.
Okay I don't love the solution, but I don't know a better way of doing it other then creating a second struct. It's one bit I really like from mocking http requests is that you can override methods inline, but gRPC requires you implement a struct that mocks the calls. Ideally I'd have a set of tests that has methods that implements the http handler method.
Pokom
deleted the
google/set-refresh-interval-for-compute-pricing-map
branch
This alters the
collector.Collectmethod on the compute module to also check if the pricing map needs to be refreshed. This is a configurable paramater so that folks that want a tighter refresh interval are easily able to do it.