client-go: add request and response size metrics #108296

aojea · 2022-02-23T10:58:37Z

/kind feature

What this PR does / why we need it:

client-go: add new histogram metric to record the size of the requests and responses.

aojea · 2022-02-23T16:26:25Z

/assign @logicalhan @dgrisonnet @wojtek-t @tkashem

logicalhan

Just one concern but this otherwise lgtm

logicalhan · 2022-02-23T16:27:35Z

staging/src/k8s.io/component-base/metrics/prometheus/restclient/metrics.go

+			// Use buckets ranging from 1000 bytes (1KB) to 10^9 bytes (1GB).
+			Buckets: k8smetrics.ExponentialBuckets(1000, 10.0, 7),
+		},
+		[]string{"verb", "url"},


this should definitely not use url and should most definitely use host due to cardinality concerns.

yeah, changed

oh, wait, this is a new interface, we can pass the host directly

aojea · 2022-02-23T16:33:04Z

staging/src/k8s.io/client-go/tools/metrics/metrics.go

@@ -42,6 +42,11 @@ type LatencyMetric interface {
 	Observe(ctx context.Context, verb string, u url.URL, latency time.Duration)
 }

+// SizeMetric observes client response size partitioned by verb and url.
+type SizeMetric interface {
+	Observe(ctx context.Context, verb string, u url.URL, size float64)


@logicalhan better if I set the host here instead of the url, right?

It's fine to pass the URL and use the host property.

logicalhan

/lgtm
/approve

staging/src/k8s.io/component-base/metrics/prometheus/restclient/metrics.go

aojea · 2022-02-23T16:43:04Z

staging/src/k8s.io/component-base/metrics/prometheus/restclient/metrics.go

@@ -40,6 +40,16 @@ var (
 		[]string{"verb", "host"},
 	)

+	requestSize = k8smetrics.NewHistogramVec(
+		&k8smetrics.HistogramOpts{
+			Name: "rest_client_request_size_bytes",


hmm, I think response is a better name

that's totally different, are you measuring what we get or what we are returning?

yeah, we should measure both , there are cases that the problems can be related to POST or GET with huge payloads,

aojea · 2022-02-23T17:13:13Z

/hold

I want to verify the metrics before merging now that we have a presubmit job that gives us a prometheus dump ... but please keep adding comments

dgrisonnet · 2022-02-24T09:39:41Z

staging/src/k8s.io/component-base/metrics/prometheus/restclient/metrics.go

+			Help:           "Response size in bytes. Broken down by verb and host.",
+			StabilityLevel: k8smetrics.ALPHA,
+			// 64 bytes to 16MB
+			Buckets: []float64{64, 256, 512, 1024, 4096, 16384, 65536, 262144, 1048576, 4194304, 16777216},


Should we perhaps sync the buckets of apiserver_response_sizes based on these new ones as a follow-up? https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go#L125-L134

I think that this ones are more "realistic" , I'm +1 on that

aojea · 2022-02-24T09:44:36Z

/test pull-kubernetes-integration

logicalhan

/lgtm

logicalhan · 2022-02-24T15:36:53Z

/approve

aojea · 2022-02-24T15:56:03Z

/approve

I see @liggitt active, let's see if we can put this in his queue , wojtek told me he is a bit busy these days

liggitt · 2022-02-24T16:28:08Z

staging/src/k8s.io/client-go/rest/request.go

@@ -901,6 +901,7 @@ func (r *Request) request(ctx context.Context, fn func(*http.Request, *http.Resp
 		}
 		resp, err := client.Do(req)
 		updateURLMetrics(ctx, r, resp, err)
+		metrics.RequestSize.Observe(ctx, r.verb, r.URL().Host, float64(req.ContentLength))


is ContentLength always populated? is there ever a case we should ignore (-1 or 0 with non-nil body?)

I honestly can't say, I was trying to skim the golang code but found it confusing, I will add an if to avoid the -1 and 0 corner cases to be safe

the condition is weird, but is based on
https://cs.opensource.google/go/go/+/refs/tags/go1.17.7:src/net/http/request.go;l=919-927;drc=e6dda19888180c5159460486d30c0412e4980748

is ContentLength always populated? is there ever a case we should ignore (-1 or 0 with non-nil body?)

ignored on that case

liggitt · 2022-02-24T16:31:52Z

staging/src/k8s.io/client-go/rest/request.go

@@ -963,6 +964,7 @@ func (r *Request) Do(ctx context.Context) Result {
 	if err != nil {
 		return Result{err: err}
 	}
+	metrics.ResponseSize.Observe(ctx, r.verb, r.URL().Host, float64(len(result.body)))


does this do the right thing if result is an error result? (result.err != nil)

I had the same doubt for latency, and asking people with more experience they told me that latency should take into account errors too, because measure the time the client is waiting.
However, for size, I can't see why do we want to store a zero on a failed request, we didn't have a result so I don't think we should measure a length of nothing

result.body is nil on failure, hence I don't think we should measure it, there is no response

I confirmed there is no null value in prometheus , so this is correct, only measure if it exists

it still seems like this should be guarded on something like if result.err == nil of if result.body != nil

liggitt · 2022-02-24T16:32:22Z

staging/src/k8s.io/client-go/rest/request.go

@@ -979,6 +981,7 @@ func (r *Request) DoRaw(ctx context.Context) ([]byte, error) {
 	if err != nil {
 		return nil, err
 	}
+	metrics.ResponseSize.Observe(ctx, r.verb, r.URL().Host, float64(len(result.body)))


same question about this doing the right thing if result.err != nil

the metrics is associated to the response, and there is no response if err != nil, that is different from result.err != nil

should this be guarded on if result.body != nil?

can it be nil?

// The http Client and Transport guarantee that Body is always
// non-nil, even on responses without a body or responses with
// a zero-length body. It is the caller's responsibility to
// close Body.

beside that, is a great question, do we record it as 0 or do we ignore?

I think if result.err != nil and len(result.body) == 0, we should not record

fedebongio · 2022-02-24T17:39:33Z

/triage accepted
Thank you to all the reviewers!

logicalhan · 2022-02-24T17:42:06Z

/triage accepted

aojea · 2022-02-26T17:39:24Z

ping @liggitt

Get metrics for the request and response size, so we can correlate latency and size on a request, otherwise we could get confused because we don't know if the network is slow or just the request size huge.

liggitt · 2022-02-28T16:08:18Z

/approve

k8s-ci-robot · 2022-02-28T16:09:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, liggitt, logicalhan

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:

~~staging/src/k8s.io/client-go/OWNERS~~ [liggitt]
~~staging/src/k8s.io/component-base/metrics/OWNERS~~ [liggitt,logicalhan]

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

aojea · 2022-02-28T17:19:15Z

/retest
E2eNode Suite: [sig-node] Summary API [NodeConformance] when querying /stats/summary should report resource usage through the stats api expand_less
k8s.io/kubernetes/vendor/k8s.io/apiextensions-apiserver/test/integration: TestCRValidationOnCRDUpdate expand_less

aojea · 2022-02-28T18:26:19Z

E2eNode Suite: [sig-node] Summary API [NodeConformance] when querying /stats/summary should report resource usage through the stats api
/test pull-kubernetes-node-e2e-containerd

tkashem · 2022-02-28T21:17:29Z

/lgtm

aojea · 2022-02-28T21:55:49Z

Kubernetes e2e suite: [sig-network] Proxy version v1 A set of valid responses are returned for both pod and service ProxyWithPath [Conformance] expand_less
/test pull-kubernetes-conformance-kind-ga-only-parallel

k8s-ci-robot requested review from jayunit100 and RainbowMango February 23, 2022 10:59

k8s-ci-robot assigned dgrisonnet, logicalhan, tkashem and wojtek-t Feb 23, 2022

logicalhan reviewed Feb 23, 2022

View reviewed changes

aojea force-pushed the client_go_size_metrics branch from 347cd8c to ec2a48a Compare February 23, 2022 16:29

aojea commented Feb 23, 2022

View reviewed changes

logicalhan approved these changes Feb 23, 2022

View reviewed changes

dgrisonnet reviewed Feb 23, 2022

View reviewed changes

staging/src/k8s.io/component-base/metrics/prometheus/restclient/metrics.go Outdated Show resolved Hide resolved

aojea commented Feb 23, 2022

View reviewed changes

staging/src/k8s.io/component-base/metrics/prometheus/restclient/metrics.go Outdated Show resolved Hide resolved

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 23, 2022

aojea commented Feb 23, 2022

View reviewed changes

aojea force-pushed the client_go_size_metrics branch from ec2a48a to 5c2403d Compare February 23, 2022 17:10

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 23, 2022

aojea force-pushed the client_go_size_metrics branch from 5c2403d to 58c44aa Compare February 23, 2022 17:12

dgrisonnet reviewed Feb 24, 2022

View reviewed changes

logicalhan reviewed Feb 24, 2022

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 24, 2022

liggitt reviewed Feb 24, 2022

View reviewed changes

aojea force-pushed the client_go_size_metrics branch from 3770f73 to 6e80080 Compare February 24, 2022 17:07

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 24, 2022

aojea force-pushed the client_go_size_metrics branch from 6e80080 to ace9bdd Compare February 24, 2022 17:12

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 24, 2022

client-go: add request and response size metrics

64d9d05

Get metrics for the request and response size, so we can correlate latency and size on a request, otherwise we could get confused because we don't know if the network is slow or just the request size huge.

aojea force-pushed the client_go_size_metrics branch from ace9bdd to 64d9d05 Compare February 28, 2022 15:59

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 28, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2022

k8s-ci-robot merged commit 5ee80de into kubernetes:master Feb 28, 2022

k8s-ci-robot added this to the v1.24 milestone Feb 28, 2022

aojea mentioned this pull request May 24, 2023

metrics: Add k8s client rate limiter latency metric cilium/cilium#25555

Merged

8 tasks

client-go: add request and response size metrics #108296

client-go: add request and response size metrics #108296

Conversation

aojea commented Feb 23, 2022 • edited

What this PR does / why we need it:

aojea commented Feb 23, 2022

logicalhan 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

logicalhan 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

aojea commented Feb 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Feb 24, 2022

logicalhan left a comment

Choose a reason for hiding this comment

logicalhan commented Feb 24, 2022

aojea commented Feb 24, 2022

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 Feb 24, 2022 • edited

Choose a reason for hiding this comment

aojea Feb 24, 2022 • edited

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

aojea Feb 28, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedebongio commented Feb 24, 2022

logicalhan commented Feb 24, 2022

aojea commented Feb 26, 2022

liggitt commented Feb 28, 2022

k8s-ci-robot commented Feb 28, 2022

aojea commented Feb 28, 2022

aojea commented Feb 28, 2022

tkashem commented Feb 28, 2022

aojea commented Feb 28, 2022

aojea commented Feb 23, 2022 •

edited

aojea Feb 24, 2022 •

edited

aojea Feb 24, 2022 •

edited

aojea Feb 28, 2022 •

edited