Added requestSloLatencies metric #105890

pawbana · 2021-10-25T22:20:11Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Current apiserver_request_duration_* metrics measure whole request duration including time spent processing webhooks. Webhook duration is mostly dependant on user configuration de so they should not be counted when considering request latency SLOs.

Which issue(s) this PR fixes:

This PR introduces better metric for measuring request latency SLOs.

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

k8s-ci-robot · 2021-10-25T22:20:19Z

Hi @pawbana. 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.

pawbana · 2021-10-26T09:15:19Z

/cc @marseel
/cc @wojtek-t

wojtek-t · 2021-10-26T13:31:21Z

/ok-to-test

I'm super interested in that metric (as something that is really reflecting healthiness of the control-plane and not skewed by badly setup webhooks).
But I would like to get some feedback from sig-apimachinery folks.

@sttts @deads2k - thoughts?

wojtek-t · 2021-11-12T08:30:23Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go

@@ -263,7 +264,13 @@ func (a *mutatingDispatcher) callAttrMutatingHook(ctx context.Context, h *admiss
 		}
 	}

-	if err := r.Do(ctx).Into(response); err != nil {
+	wd, ok := endpointsrequest.WebhookDurationFrom(ctx)


How about:

do := func() { err = r.Do(ctx).Into(response) if wd, ok := endpointsrequest.WebhookDurationFrom(ctx); ok { tmp := do do = func() { wd.AdmitTracker.Track(do()) } do()

staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go

staging/src/k8s.io/apiserver/pkg/endpoints/request/webhook_duration.go

wojtek-t · 2021-11-12T08:35:55Z

staging/src/k8s.io/apiserver/pkg/endpoints/request/webhook_duration.go

+}
+
+func WithWebhookDurationAndCustomClock(parent context.Context, c clock.Clock) context.Context {
+	return WithValue(parent, webhookDurationKey, WebhookDuration{


I would actually store a pointer in the context.

[And then below you just return "nil, false" in WebhookDurationFrom below]

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/plugin_test.go

wojtek-t · 2021-11-12T15:11:28Z

staging/src/k8s.io/apiserver/pkg/endpoints/request/webhook_duration.go

+type durationTracker struct {
+	clock             clock.Clock
+	latency           time.Duration
+	mu                *sync.RWMutex


Don't make it a pointer - just make it sync.Lock

[and don't use RWLock - we don't expect any contention on read here and RWLock is more expensive than Lock]

wojtek-t · 2021-11-15T08:27:49Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/plugin_test.go

+			wd, ok := request.WebhookDurationFrom(ctx)
+			if !ok {
+				if test.InitContext {
+					t.Errorf("expected webhook duraiton to be initialized")


nit: duration (same below)

Thanks, fixed.

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/testcase.go

staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/webhook_server.go

wojtek-t · 2021-11-15T10:05:00Z

staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go

@@ -468,6 +482,10 @@ func MonitorRequest(req *http.Request, verb, group, version, resource, subresour
 		}
 	}
 	requestLatencies.WithContext(req.Context()).WithLabelValues(reportedVerb, dryRun, group, version, resource, subresource, scope, component).Observe(elapsedSeconds)
+	wd, ok := request.WebhookDurationFrom(req.Context())


nit;

if wd, ok := ... ; ok {

wojtek-t · 2021-11-15T10:05:51Z

staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go

@@ -468,6 +482,10 @@ func MonitorRequest(req *http.Request, verb, group, version, resource, subresour
 		}
 	}
 	requestLatencies.WithContext(req.Context()).WithLabelValues(reportedVerb, dryRun, group, version, resource, subresource, scope, component).Observe(elapsedSeconds)
+	wd, ok := request.WebhookDurationFrom(req.Context())
+	if ok {
+		requestSloLatencies.WithContext(req.Context()).WithLabelValues(reportedVerb, group, version, resource, subresource, scope).Observe(elapsedSeconds - (wd.AdmitTracker.GetLatency() + wd.ValidateTracker.GetLatency()).Seconds())


nit:

sloLatency := elapsedSecond - (wd.AdmiTracker.GetLatency() + wd.ValidateTracker.GetLatency()).Seconds() requestSLOLatencies...

wojtek-t · 2021-11-15T11:22:54Z

/lgtm
/approve

Thanks!

k8s-ci-robot · 2021-11-15T11:23:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pawbana, wojtek-t

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/apiserver/OWNERS~~ [wojtek-t]

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

…available in <1.23.0-0 clusters In upstream Kubernetes (specifically in this commit kubernetes/apiserver@9144ea1), starting k8s 1.23 a new version of the apiserver_request_duration_seconds metric was released called apiserver_request_slo_duration_seconds. As noted in the pull request kubernetes/kubernetes#105890, the fundamental difference between the newer and older metric is that the newer metric excludes the processing time spent by webhooks that the API server calls out to when configured as part of a MutatingWebhookConfiguration or ValidatingWebhookConfiguration. However, since this metric was only introduced in k8s 1.23, this breaks the API Server dashboards in clusters <1.23.0 since that metric does not exist. Therefore, to fix this issue in older clusters using the latest Monitoring chart, specific logic has been added to the chart to utilize the older version of the metric when the Kubernetes version is detected to be <1.23.

k8s-ci-robot requested review from hzxuzhonghu and jimmidyson October 25, 2021 22:21

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 26, 2021

k8s-ci-robot requested review from marseel and wojtek-t October 26, 2021 09:15

pawbana force-pushed the added_request_slo_latency_metric branch from f4104f6 to 7efe5d4 Compare October 26, 2021 09:21

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 Oct 26, 2021

pawbana force-pushed the added_request_slo_latency_metric branch 3 times, most recently from 532d6e7 to 5d0a3d7 Compare October 26, 2021 16:13

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Oct 26, 2021

pawbana force-pushed the added_request_slo_latency_metric branch from 0aef7dc to 0c54cfe Compare November 10, 2021 16:18

wojtek-t reviewed Nov 12, 2021

View reviewed changes

pawbana force-pushed the added_request_slo_latency_metric branch 4 times, most recently from b1ff221 to d6bc59e Compare November 12, 2021 14:56

wojtek-t reviewed Nov 12, 2021

View reviewed changes

pawbana force-pushed the added_request_slo_latency_metric branch 2 times, most recently from f5918c2 to dc4a8df Compare November 12, 2021 19:10

wojtek-t reviewed Nov 15, 2021

View reviewed changes

pawbana force-pushed the added_request_slo_latency_metric branch 2 times, most recently from 7ad0c53 to fc0f7be Compare November 15, 2021 09:46

wojtek-t reviewed Nov 15, 2021

View reviewed changes

pawbana force-pushed the added_request_slo_latency_metric branch from fc0f7be to 2446939 Compare November 15, 2021 10:49

Added requestSloLatencies metric

0afa569

pawbana force-pushed the added_request_slo_latency_metric branch from 2446939 to 0afa569 Compare November 15, 2021 11:11

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 15, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 15, 2021

k8s-ci-robot merged commit 1e6f3b5 into kubernetes:master Nov 15, 2021

k8s-ci-robot added this to the v1.23 milestone Nov 15, 2021

pawbana deleted the added_request_slo_latency_metric branch December 22, 2021 14:11

pawbana mentioned this pull request Aug 3, 2022

REQUEST: New membership for pawbana kubernetes/org#3586

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added requestSloLatencies metric #105890

Added requestSloLatencies metric #105890

pawbana commented Oct 25, 2021 •

edited

k8s-ci-robot commented Oct 25, 2021

pawbana commented Oct 26, 2021

wojtek-t commented Oct 26, 2021

wojtek-t Nov 12, 2021

wojtek-t Nov 12, 2021

pawbana Nov 12, 2021

wojtek-t Nov 12, 2021

wojtek-t Nov 15, 2021

pawbana Nov 15, 2021

wojtek-t Nov 15, 2021

wojtek-t Nov 15, 2021

wojtek-t commented Nov 15, 2021

k8s-ci-robot commented Nov 15, 2021

Added requestSloLatencies metric #105890

Added requestSloLatencies metric #105890

Conversation

pawbana commented Oct 25, 2021 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Oct 25, 2021

pawbana commented Oct 26, 2021

wojtek-t commented Oct 26, 2021

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

Choose a reason for hiding this comment

wojtek-t commented Nov 15, 2021

k8s-ci-robot commented Nov 15, 2021

pawbana commented Oct 25, 2021 •

edited