feat: add new kube_service_spec_ports metric#2849
feat: add new kube_service_spec_ports metric#2849Serializator wants to merge 1 commit intokubernetes:mainfrom
Conversation
a35c311 to
dc2f674
Compare
dc2f674 to
e9da2b6
Compare
|
Hey @dgrisonnet & @mrueg! Would one of you have time to approve the workflow so tests can run? Thank you in advance! |
bhope
left a comment
There was a problem hiding this comment.
Thanks for the PR. A few comments:
- Stability: new metrics should start at basemetrics.ALPHA, not basemetrics.STABLE (see inline)
node_port_number: should always be present as a label, using "" when no NodePort is set (see inline)
internal/store/service.go
Outdated
| "kube_service_spec_ports", | ||
| "Service ports. One series for each port", | ||
| metric.Gauge, | ||
| basemetrics.STABLE, |
There was a problem hiding this comment.
I believe this should be basemetrics.ALPHA. We can only promote a metric to alpha->beta->stable after it has gone through multiple releases without any issues. Happy to be corrected if I am missing any context.
There was a problem hiding this comment.
Thank you for the review! I updated it to be basemetrics.ALPHA
| values := []string{port.Name, string(protocol), strconv.FormatInt(int64(port.Port), 10)} | ||
|
|
||
| // 0 means no node port is set. It is also a reserved port (RFC-6335) so there shouldn't be a problem. | ||
| if port.NodePort > 0 { |
There was a problem hiding this comment.
node_port_number should always be present per the project's best practices. Use "" when NodePort == 0 instead of conditionally adding the label.
Check the best practices here: https://github.com/kubernetes/kube-state-metrics/blob/main/docs/design/metrics-best-practices.md#optional-properties
There was a problem hiding this comment.
Thank you! I've updated it to be an empty string when the node port is not set.
e9da2b6 to
12295e8
Compare
internal/store/service.go
Outdated
| nodePortNumber = strconv.FormatInt(int64(port.NodePort), 10) | ||
| } | ||
|
|
||
| keys := []string{"port_name", "port_protocol", "port_number", "node_port_number"} |
There was a problem hiding this comment.
nit: probably should sit outside for loop
| | kube_service_created | Gauge | Unix creation timestamp | seconds | `service`=<service-name> <br> `namespace`=<service-namespace> <br> `uid`=<service-uid> | STABLE | | ||
| | kube_service_spec_type | Gauge | Type about service | | `service`=<service-name> <br> `namespace`=<service-namespace> <br> `uid`=<service-uid> <br> `type`=<ClusterIP\|NodePort\|LoadBalancer\|ExternalName> | STABLE | | ||
| | kube_service_spec_external_ip | Gauge | Service external ips. One series for each ip | | `service`=<service-name> <br> `namespace`=<service-namespace> <br> `uid`=<service-uid> <br> `external_ip`=<external-ip> | STABLE | | ||
| | kube_service_spec_ports | Gauge | Service port. One series for each port | | `service`=<service-name> <br> `namespace`=<service-namespace> <br> `uid`=<service-uid> <br> `port_number`=<port-number> <br> `node_port_number`=<node-port-number> | EXPERIMENTAL | |
There was a problem hiding this comment.
Can you verify this? Your code emits port_name and port_protocol too
There was a problem hiding this comment.
You're correct. Good catch! I added port_name and port_protocol to the documentation.
|
/triage accepted |
12295e8 to
33423e9
Compare
|
Overall its in a good shape now. Can you squash the commits? |
Signed-off-by: Julian van den Berkmortel <7153670+Serializator@users.noreply.github.com>
33423e9 to
14684e0
Compare
|
@bhope squashed the commits and also resolved a linting problem. |
|
Thanks /lgtm |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bhope, Serializator The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Can we quantify the number of timeseries it generates per port, service, etc.? |
What this PR does / why we need it:
Add new metric
kube_service_spec_portsto represent ports on the service resource.The
kube_service_spec_prefix matches with the naming convention for the rest of the metrics.To stay aligned with other metrics,
.spec.ports.appProtocolis not included as a label. The same is true for thekube_endpoint_addressmetric..spec.ports.targetPortis not included as a label due to its inconsistent value. It can be a named port or port number.How does this change affect the cardinality of KSM:
Increases cardinality with a new time series.
Which issue(s) this PR fixes:
Fixes #2819