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

Add metrics for k8s statefulset #1637

Closed
ChrsMark opened this issue Nov 29, 2024 · 9 comments · Fixed by #1636
Closed

Add metrics for k8s statefulset #1637

ChrsMark opened this issue Nov 29, 2024 · 9 comments · Fixed by #1636
Labels
area:k8s enhancement New feature or request

Comments

@ChrsMark
Copy link
Member

ChrsMark commented Nov 29, 2024

Area(s)

area:k8s

Is your change request related to a problem? Please describe.

Part of #1032.

This issue is for adding the statefulset related metrics.

Describe the solution you'd like

The following metrics are already implemented by the Collector:
k8s.statefulset.desired_pods
k8s.statefulset.ready_pods
k8s.statefulset.current_pods
k8s.statefulset.updated_pods

ref: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/v0.114.0/receiver/k8sclusterreceiver/documentation.md#k8sstatefulsetcurrent_pods

Describe alternatives you've considered

No response

Additional context

API ref: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#statefulsetstatus-v1-apps

I would like to validate the names here though and make sure that we are happy with the .ready_pods and .desired_pods. The alternative would be to have them as .desired and .available as we have in the deployment and replicaset metrics (#1636)?

@TylerHelmuth @povilasv @dmitryax could you verify if that's the desired modeling here for any specific reason or if it would make sense to change it so as to be consistent with the other mentioned namespaces?

@ChrsMark ChrsMark added enhancement New feature or request experts needed This issue or pull request is outside an area where general approvers feel they can approve triage:needs-triage area:k8s and removed experts needed This issue or pull request is outside an area where general approvers feel they can approve triage:needs-triage labels Nov 29, 2024
@dmitryax
Copy link
Member

I don't remember any history behind these names. It makes sense to keep it consistent with other metrics and drop _pods suffix

@tetianakravchenko
Copy link
Contributor

I would like to validate the names here though and make sure that we are happy with the .ready_pods and .desired_pods. The alternative would be to have them as .desired and .available as we have in the deployment and replicaset metrics (#1636)?

k8s.statefulset.desired_pods fully match to the similar deployment/replicaset metrics, as it relies on the spec.replicas. Using the similar naming I think is a good idea - k8s.statefulset.desired

k8s.statefulset.ready_pods - is actually relying on Status.ReadyReplicas not on the Status.AvailableReplicas. It wouldn't be correct to rename k8s.statefulset.ready_pods to k8s.statefulset.available

@ChrsMark
Copy link
Member Author

ChrsMark commented Dec 3, 2024

I would like to validate the names here though and make sure that we are happy with the .ready_pods and .desired_pods. The alternative would be to have them as .desired and .available as we have in the deployment and replicaset metrics (#1636)?

k8s.statefulset.desired_pods fully match to the similar deployment/replicaset metrics, as it relies on the spec.replicas. Using the similar naming I think is a good idea - k8s.statefulset.desired

k8s.statefulset.ready_pods - is actually relying on Status.ReadyReplicas not on the Status.AvailableReplicas. It wouldn't be correct to rename k8s.statefulset.ready_pods to k8s.statefulset.available

Good point! So if we are ok with removing the _pods suffix the suggest metrics' names would be the following:

k8s.statefulset.desired
k8s.statefulset.ready
k8s.statefulset.current
k8s.statefulset.updated

However again we need to keep that consistent with other metrics like those of hpa. So an alternative in general would be to include the replicas term in the name:

k8s.statefulset.replicas.desired
k8s.statefulset.replicas.ready
k8s.statefulset.replicas.current
k8s.statefulset.replicas.updated

@TylerHelmuth
Copy link
Member

TylerHelmuth commented Dec 3, 2024

There is also the pattern of k8s.daemonset.current_scheduled_nodes to consider.

One issue I have with k8s.statefulset.ready is that it implies (to mean at least) to be a metric concerning the number of ready statefulsets, but that is not what it is counting. For all these metrics I believe that is why the addition of _pods, _scheduled_nodes, and _replicas was added to the name - to make it clear in the name exactly what is being counted with relation to statefulset, daemonset, and hpa.

I'm of the opinion that we leave the names as is since they are conveying correct information and have been in place for a while.

@ChrsMark
Copy link
Member Author

ChrsMark commented Dec 3, 2024

Same issue seems to appear for the deployment, replicaset etc resources: #1636

k8s.deployment.desired might again be received (wrongly) as number of desired deployments, no?

@TylerHelmuth
Copy link
Member

TylerHelmuth commented Dec 3, 2024

Bummer, we're already inconsistent 😢

@dmitryax
Copy link
Member

dmitryax commented Dec 3, 2024

This is an interesting topic. Keep in mind that we also have k8s.container.ready which has a completely different meaning but sounds like k8s.statefulset.ready. It doesn't seem right. We also have k8s.daemonset.ready_nodes. Now I'm thinking it may be better to add the _pods suffix to other metrics in #1636 if we want consistency. WDYT?

@jaronoff97
Copy link

jaronoff97 commented Dec 3, 2024

I like the proposal that we should attempt to align more closely with what KSM has here? i.e. k8s.deployment.ready_replicas or k8s.deployment.status.replicas_ready, it seems a bit closer to the expectations of users.

@ChrsMark
Copy link
Member Author

ChrsMark commented Dec 4, 2024

+1 for having consistency here.

We can add the _pods or _replicas suffix as it was mentioned. This would also be in-line with the pluralization guideline:

#### Pluralization
Metric namespaces SHOULD NOT be pluralized.
Metric names SHOULD NOT be pluralized, unless the value being recorded
represents discrete instances of a
[countable quantity](https://wikipedia.org/wiki/Count_noun).
Generally, the name SHOULD be pluralized only if the unit of the metric in
question is a non-unit (like `{fault}` or `{operation}`).
Examples:
* `system.filesystem.utilization`, `http.server.request.duration`, and `system.cpu.time`
should not be pluralized, even if many data points are recorded.
* `system.paging.faults`, `system.disk.operations`, and `system.network.packets`
should be pluralized, even if only a single data point is recorded.

I would be more in favor of k8s.deployment.available_replicas, k8s.deployment.desired_replicas, k8s.statefulset.desired_replicas etc. but maybe _pods is more "user friendly" so I would be fine keeping this as well.

We cannot really reduce the number of breaking changes. 4 statefulset metrics come with the _pods suffix while the hpa metrics come with the _replicas one.

_replicas is indeed aligned with KSM as it was mentioned above but it's not that KSM provides the best naming for all. See for example the daemonset-metrics where the metrics are called like kube_daemonset_status_number_ready. For jobs it make sense to have the _pods suffix (i.e. k8s.job.active_pods) and for daemonsets the pattern of k8s.daemonset.current_scheduled_nodes.

I will group all the "replicas-related" together as part of the #1636 to illustrate the idea and we can continue the discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:k8s enhancement New feature or request
Projects
Status: Done
5 participants