Export various stats about services in the metrics exported by this c… #1943

mmamczur · 2023-02-10T17:12:47Z

…ontroller.

k8s-ci-robot · 2023-02-10T17:12:56Z

Hi @mmamczur. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/servicemetrics/servicemetrics.go

aojea · 2023-02-13T10:35:03Z

pkg/servicemetrics/servicemetrics.go

+		IsStaticIPv4:          service.Spec.LoadBalancerIP != "",
+		IsStaticIPv6:          false,


@panslava is this assumption right? there are never StaticIPv6?

for now they are not supported in Google Cloud

Let's add a todo to extend this field as soon as we support this

aojea · 2023-02-13T10:40:54Z

pkg/servicemetrics/servicemetrics.go

+	return string(*service.Spec.InternalTrafficPolicy)
+}
+
+func getPortsBucket(ports []v1.ServicePort) string {


prometheus histogram metrics only need to receive the data and it already. manages the bucket, do we need to export this as a string?

so here we want to limit the number of possible values in the label and we decided on these specific 'bucket' ourselves. This is intended to limit the cardinality of the metric

aojea · 2023-02-13T11:48:29Z

/ok-to-test

pkg/servicemetrics/servicemetrics.go

cezarygerard · 2023-02-22T14:44:52Z

pkg/servicemetrics/servicemetrics.go

+		IsStaticIPv4:          service.Spec.LoadBalancerIP != "",
+		IsStaticIPv6:          false,


Let's add a todo to extend this field as soon as we support this

pkg/servicemetrics/servicemetrics.go

pkg/servicemetrics/servicemetrics_test.go

pkg/servicemetrics/servicemetrics.go

pkg/servicemetrics/servicemetrics_test.go

cezarygerard · 2023-02-22T15:20:52Z

Michał

overall LGTM from my side

please take from my comments whatever you find useful and we can commit soon
drop the comments that you find unuseful

thanks!

…ontroller.

swetharepakula

this mostly looks good to me. I would recommend customizing the buckets in prometheus if the goal is to reduce the cardinality and remove some of the logic necessary to maintain the buckets.

swetharepakula · 2023-02-23T18:37:35Z

pkg/servicemetrics/servicemetrics.go

+	labelIPFamilies            = "ip_families"
+	labelIPFamilyPolicy        = "ip_family_policy"
+	labelIsStaticIPv4          = "is_static_ip_v4"
+	labelIsStaticIPv6          = "is_static_ip_v6"


do we want a labelIsStaticDualStack?

LB that would have 2 static IPs?
is this possible? We could also set both these to true if we ever support a case like that

swetharepakula · 2023-02-23T18:38:21Z

pkg/servicemetrics/servicemetrics.go

+var (
+	serviceL4ProtocolStatsCount = prometheus.NewGaugeVec(
+		prometheus.GaugeOpts{
+			Name: "service_l4_protocol_stats",


nit: Since these aren't all protocol labels, many configuration or config?

something like service_l4_config_stats?

swetharepakula · 2023-02-23T18:41:05Z

pkg/servicemetrics/servicemetrics.go

+
+const (
+	// Names of the labels used by the service metrics.
+	labelType                  = "type"


what are the possible values for "type"

in general this is the service type, so could be ClusterIP, NodePort etc with a caveat that instead of just having LoadBalancer we have special values for each varian of them, like LegacyXLB, RBSXLB etc
we considered having separate label but we wanted to limit the number of possible label value combinatons

mmamczur · 2023-02-24T18:56:10Z

this mostly looks good to me. I would recommend customizing the buckets in prometheus if the goal is to reduce the cardinality and remove some of the logic necessary to maintain the buckets.

what do you mean about the buckets, can you have them for the label values (and not the metric value)?

cezarygerard · 2023-02-28T16:10:39Z

@swetharepakula
There is a problem in changing the metric labels with bucketing information
into proper prometheus histograms

the metrics we want to export are type gauge, not cumulative, and they are exported every 10 minutes
there is no way we can create histogram of gauges in prometheus as of now

even if there was such metric type, to calculate bucketed gauges we would need to keep a slice of buckets in memory while iterating over services (thus duplicating prometheus)

also we can not use histogram_metric.Reset() before setting the histogram values from scratch every 10 minutes, as the scraping may happen just after we reset values and before we se new ones fully resulting is some funny values exported sometimes

cezarygerard · 2023-03-07T10:57:27Z

/lgtm

k8s-ci-robot · 2023-03-07T10:57:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cezarygerard, mmamczur

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cezarygerard]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 10, 2023

k8s-ci-robot requested review from aojea and swetharepakula February 10, 2023 17:12

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 10, 2023

aojea reviewed Feb 13, 2023

View reviewed changes

pkg/servicemetrics/servicemetrics.go Show resolved Hide resolved

aojea reviewed Feb 13, 2023

View reviewed changes

mmamczur force-pushed the metrics_controller_attempt2 branch from 57194ba to f476884 Compare February 13, 2023 11:26

mmamczur marked this pull request as ready for review February 13, 2023 11:45

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 13, 2023

k8s-ci-robot requested review from aojea and freehan February 13, 2023 11:45

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 13, 2023

cezarygerard reviewed Feb 22, 2023

View reviewed changes

mmamczur force-pushed the metrics_controller_attempt2 branch 5 times, most recently from 309bac4 to ad346f4 Compare February 23, 2023 12:21

Export various stats about services in the metrics exported by this c…

0e76ed7

…ontroller.

mmamczur force-pushed the metrics_controller_attempt2 branch from ad346f4 to 0e76ed7 Compare February 23, 2023 12:35

swetharepakula reviewed Feb 23, 2023

View reviewed changes

k8s-ci-robot assigned cezarygerard Mar 7, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 7, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 7, 2023

k8s-ci-robot merged commit b225097 into kubernetes:master Mar 7, 2023

mmamczur deleted the metrics_controller_attempt2 branch April 10, 2024 13:16

FelipeYepez mentioned this pull request Jun 21, 2024

Remove deprecated metrics #2588

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Export various stats about services in the metrics exported by this c… #1943

Export various stats about services in the metrics exported by this c… #1943

mmamczur commented Feb 10, 2023

k8s-ci-robot commented Feb 10, 2023

aojea Feb 13, 2023

panslava Feb 13, 2023

cezarygerard Feb 22, 2023

aojea Feb 13, 2023

mmamczur Feb 13, 2023

aojea commented Feb 13, 2023

cezarygerard Feb 22, 2023

cezarygerard commented Feb 22, 2023

swetharepakula left a comment

swetharepakula Feb 23, 2023

mmamczur Feb 23, 2023

swetharepakula Feb 23, 2023

mmamczur Feb 23, 2023

swetharepakula Feb 23, 2023

mmamczur Feb 23, 2023

mmamczur commented Feb 24, 2023

cezarygerard commented Feb 28, 2023

cezarygerard commented Mar 7, 2023

k8s-ci-robot commented Mar 7, 2023

		IsStaticIPv4: service.Spec.LoadBalancerIP != "",
		IsStaticIPv6: false,

Export various stats about services in the metrics exported by this c… #1943

Export various stats about services in the metrics exported by this c… #1943

Conversation

mmamczur commented Feb 10, 2023

k8s-ci-robot commented Feb 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Feb 13, 2023

Choose a reason for hiding this comment

cezarygerard commented Feb 22, 2023

swetharepakula left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmamczur commented Feb 24, 2023

cezarygerard commented Feb 28, 2023

cezarygerard commented Mar 7, 2023

k8s-ci-robot commented Mar 7, 2023