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

Update readme and fix naming for garden_shoot_node_info #108

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Sinscerly
Copy link
Contributor

What this PR does / why we need it:
This PR removes a duplicate line the readme for the metric garden_shoot_operation_states and renames the metric garden_shoot_node_info to garden_shoot_worker_node_info. The new naming is what the metric really is about, the worker group info and not all nodes in the shoot cluster.

Which issue(s) this PR fixes:
Fixes #98

Release note:

Update readme
renames the metric `garden_shoot_node_info` to `garden_shoot_worker_node_info`

This name is a better representation for this metric. As it is based on each worker group and not all nodes.
@Sinscerly Sinscerly requested a review from a team as a code owner April 24, 2024 12:31
@gardener-robot gardener-robot added the needs/review Needs review label Apr 24, 2024
@gardener-robot-ci-2
Copy link
Contributor

Thank you @Sinscerly for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below.

@gardener-robot
Copy link

@Sinscerly Thank you for your contribution.

@gardener-robot gardener-robot added the size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) label Apr 24, 2024
@rickardsjp
Copy link
Contributor

Hi @Sinscerly, thank you for your contribution.
I had a chat with @vicwicker about this and we have a couple of thoughts:

  • We can't simply rename a metric, we would have to deprecate the old metric and create a new metric to give consumers (i.e. only the gardener/gardener in our case) the chance to update recording rules and alerts.
  • If the concern is clarifying that it's about worker groups rather than the actual nodes, the name should probably be garden_shoot_worker_group_info.
  • At the moment, we don't really see it as a pressing issue, seeing how the label is called worker_group and not so misleading. I appreciate the sentiment though, and willing to change my mind about it ;)

@Sinscerly
Copy link
Contributor Author

Hi @rickardsjp.

It's understandable that renaming directly might not be feasible. I would agree to introduce the new renamed metric initially, followed by deprecating the old one.

Currently, the naming convention for worker groups appears to be 'worker,' based on my review of the code. Correct me if I'm wrong, but a glance at the Gardener code (https://gardener.cloud/docs/gardener/api-reference/core/#worker) and previously submitted code for the metrics exporter reveals metrics such as 'garden_shoot_worker_node_min_total' and 'garden_shoot_worker_node_max_total'.

Additionally, my main issue is that 'garden_shoot_node_info' seems to be an inappropriate name for the metrics it returns.

I'll add another PR for the duplicated line; garden_shoot_operation_states.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/review Needs review size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing metric in readme.md - garden_shoot_node_info / rename
4 participants