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

fix(gke): Update collect method to gracefully fail #336

Merged
merged 2 commits into from
Oct 22, 2024

Conversation

Pokom
Copy link
Contributor

@Pokom Pokom commented Oct 22, 2024

There is a condition where a failure to find a machine type in the price map was causing the Collect method to return instead of continuing. Instead, this changes the logic to log out the message within Collect and continue operating.
Adds a unit test to reproduce the error and then validates it is resolved.

A future improvement here would be to

  1. Export a metric
  2. Set a sane default value(if provided?)

There is a condition where a failure to find a machine type in the price
map was causing the `Collect` method to return instead of continuing.
Instead, this changes the logic to log out the message within `Collect`
and continue operating.
Adds a unit test to reproduce the error and then validates it is
resolved.

A future improvement here would be to
1. Export a metric
2. Set a sane default value(if provided?)

- closes #335
@Pokom Pokom requested a review from a team as a code owner October 22, 2024 17:53
Comment on lines +217 to +225
c.logger.LogAttrs(ctx,
slog.LevelError,
err.Error(),
slog.String("disk_name", disk.Name),
slog.String("project", project),
slog.String("region", d.Region()),
slog.String("cluster_name", d.Cluster),
slog.String("storage_class", d.StorageClass()),
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wasn't part of the issue linked in the PR, but it's good cleanup to convert the section to using slog.

Copy link

Choose a reason for hiding this comment

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

total +1

Copy link

@jjo jjo left a comment

Choose a reason for hiding this comment

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

LGTM!, thanks for the quick one :shipit:

@Pokom Pokom merged commit f52c86f into main Oct 22, 2024
2 checks passed
@Pokom Pokom deleted the fix/gke-blocked-on-pricing-map-miss branch October 22, 2024 18:20
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.

GKE stops exporting metrics if machine family isn't found in pricing map
2 participants