Collect prometheus metrics for custom resources #57682

nikhita · 2017-12-28T09:18:59Z

Enables apiserver metrics for custom resources.

Release note:

Enable apiserver metrics for custom resources.

/cc sttts deads2k kargakis brancz

nikhita · 2017-12-28T09:44:27Z

/area custom-resources

sttts · 2018-01-02T07:30:50Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

+				httpCode = rw.Status()
+			}
+		}
+		metrics.Record(req, requestInfo, w.Header().Get("Content-Type"), httpCode, responseLength, time.Now().Sub(reqStart))


is this go func duplicated from the normal endpoints? Any chance to factor out the later and reuse it here?

Yes, will do. 👍

sttts · 2018-01-02T14:22:48Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/proxy.go

@@ -57,16 +57,7 @@ func (r *ProxyHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {

 	var httpCode int
 	var requestInfo *request.RequestInfo
-	defer func() {


this is only the special proxy handler. Something similar must be in the other endpoints.

brancz

Just one comment, otherwise lgtm

brancz · 2018-01-03T10:46:14Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

@@ -152,6 +154,9 @@ func (r *crdHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 		return
 	}

+	var httpCode int
+	defer handlers.RecordMetrics(w, req, requestInfo, httpCode, reqStart)


Do we really want to add this code to each and every handler? Seems like it would make sense to wrap each handler in a handler that records this.

You probably overestimate the number of handlers. We have the proxy handler, the crd handler and basically all other API handlers (impemented in a generic way).

Though having this as a decorator would be better. Not sure that's feasible code-wise?

I see, then I'm fine with this, I just thought we're doing this for every API avilable.

0xmichalis · 2018-01-03T11:01:30Z

Is the crdHandler handling CRDs or CRs?

nikhita · 2018-01-03T11:07:05Z

Is the crdHandler handling CRDs or CRs?

CRs.

nikhita · 2018-01-03T11:08:04Z

Is the crdHandler handling CRDs or CRs?

To be precise, it is named as customresource_handler, not crdHandler in reality.

sttts · 2018-01-03T14:13:17Z

Is the crdHandler handling CRDs or CRs?

CRDs use the normal code-path of native resources. Do we have metrics problems as well for CRDs?

0xmichalis · 2018-01-03T14:18:36Z

Do we have metrics problems as well for CRDs?

I think we already collect metrics for CRDs but personally I am not interested in those. User CRD requests to the k8s API are almost zero (one per resource during registration). I just wanted to make sure because the name of the handler type is confusing.

nikhita · 2018-01-03T16:05:08Z

Do we have metrics problems as well for CRDs?

No, metrics for CRDs works fine.

nikhita · 2018-01-03T16:05:37Z

/retest

sttts · 2018-01-05T07:31:21Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/proxy.go

@@ -283,3 +274,14 @@ func singleJoiningSlash(a, b string) string {
 	}
 	return a + b
 }
+
+func RecordMetrics(w http.ResponseWriter, req *http.Request, requestInfo *request.RequestInfo, httpCode int, reqStart time.Time) {


Compare

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

Line 151 in fabce1b

func InstrumentRouteFunc(verb, resource, subresource, scope string, routeFunc restful.RouteFunction) restful.RouteFunction {

. Can you move this there and align the signature?

Any blockers here?

Any blockers here?

We don't have a restful.RouteFunction here in this handler. Any reason why the RecordMetrics is incorrect?

@sttts updated to use InstrumentHandlerFunc. ptal, thanks.

sttts · 2018-02-08T09:10:46Z

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

+	}
+}
+
+func CleanScope(requestInfo *request.RequestInfo) string {


sttts · 2018-02-08T09:11:24Z

@brancz do you know the metrics code in the apiserver? Can you take a look?

brancz

@sttts I wouldn't say I'm intimately familiar with it, but from a general metrics perspective and digging through the code for some time now, I think this looks good.

brancz · 2018-02-08T09:42:29Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

 		handler(w, req)
 		return
 	case "deletecollection":
 		checkBody := true
 		handler := handlers.DeleteCollection(storage, checkBody, requestScope, r.admission)
+		handler = metrics.InstrumentHandlerFunc(verb, resource, subresource, scope, handler)


These metrics.InstrumentHandlerFunc calls seem duplicated, maybe we can just set the handler in each of these blocks and call this once?

with a global handler var, we could remove the instrumentation call, the handler(w,req) call and the return and do those uniformely for all non-default cases below the switch.

Since we have a custom handler for apiextensions-apiserver, we need to record the metrics here.

nikhita · 2018-02-08T15:10:57Z

/retest

brancz · 2018-02-09T07:20:07Z

/lgtm

sttts · 2018-02-09T09:18:32Z

/approve

k8s-ci-robot · 2018-02-09T09:18:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brancz, nikhita, sttts

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 OWNERS Files:

~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [sttts]
~~staging/src/k8s.io/apiserver/OWNERS~~ [sttts]

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

k8s-github-robot · 2018-02-09T09:20:13Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-02-09T10:18:45Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot requested review from brancz, deads2k and 0xmichalis December 28, 2017 09:18

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Dec 28, 2017

k8s-ci-robot requested a review from sttts December 28, 2017 09:18

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 28, 2017

k8s-github-robot assigned enisoc and sttts Dec 28, 2017

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Dec 28, 2017

k8s-ci-robot added the area/custom-resources label Dec 28, 2017

nikhita force-pushed the customresource-metrics branch from e373ba4 to 3fd9f50 Compare December 28, 2017 15:48

sttts reviewed Jan 2, 2018

View reviewed changes

nikhita force-pushed the customresource-metrics branch from 3fd9f50 to 6ccdd75 Compare January 2, 2018 13:12

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 2, 2018

nikhita force-pushed the customresource-metrics branch from 6ccdd75 to 5cdf4ec Compare January 2, 2018 13:24

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 2, 2018

sttts reviewed Jan 2, 2018

View reviewed changes

brancz reviewed Jan 3, 2018

View reviewed changes

nikhita force-pushed the customresource-metrics branch from 5cdf4ec to 86c2aee Compare January 3, 2018 16:06

sttts reviewed Jan 5, 2018

View reviewed changes

nikhita force-pushed the customresource-metrics branch from 86c2aee to 336f33b Compare February 7, 2018 20:24

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 7, 2018

sttts reviewed Feb 8, 2018

View reviewed changes

brancz reviewed Feb 8, 2018

View reviewed changes

nikhita force-pushed the customresource-metrics branch from e03a11b to 6043876 Compare February 8, 2018 11:35

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 8, 2018

nikhita added 2 commits February 8, 2018 17:06

Collect prometheus metrics for custom resources

74cd45f

Since we have a custom handler for apiextensions-apiserver, we need to record the metrics here.

update staging godeps

6043876

k8s-ci-robot assigned brancz Feb 9, 2018

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

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

k8s-github-robot merged commit 8eae0a8 into kubernetes:master Feb 9, 2018

0xmichalis mentioned this pull request Apr 6, 2018

Decorate PodSpec with initcontainers kubernetes/test-infra#7594

Merged

10 tasks

nikhita deleted the customresource-metrics branch July 10, 2018 08:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collect prometheus metrics for custom resources #57682

Collect prometheus metrics for custom resources #57682

nikhita commented Dec 28, 2017 •

edited

Loading

nikhita commented Dec 28, 2017

sttts Jan 2, 2018 •

edited

Loading

nikhita Jan 2, 2018

nikhita Jan 2, 2018

sttts Jan 2, 2018

brancz left a comment

brancz Jan 3, 2018

sttts Jan 3, 2018

brancz Jan 3, 2018

0xmichalis commented Jan 3, 2018

nikhita commented Jan 3, 2018

nikhita commented Jan 3, 2018

sttts commented Jan 3, 2018

0xmichalis commented Jan 3, 2018

nikhita commented Jan 3, 2018

nikhita commented Jan 3, 2018

sttts Jan 5, 2018

sttts Jan 15, 2018

nikhita Jan 17, 2018 •

edited

Loading

0xmichalis Feb 5, 2018

nikhita Feb 7, 2018

sttts Feb 8, 2018

nikhita Feb 8, 2018

sttts commented Feb 8, 2018

brancz left a comment

brancz Feb 8, 2018

sttts Feb 8, 2018

nikhita Feb 8, 2018

nikhita commented Feb 8, 2018

brancz commented Feb 9, 2018

sttts commented Feb 9, 2018

k8s-ci-robot commented Feb 9, 2018

k8s-github-robot commented Feb 9, 2018

k8s-github-robot commented Feb 9, 2018

Collect prometheus metrics for custom resources #57682

Collect prometheus metrics for custom resources #57682

Conversation

nikhita commented Dec 28, 2017 • edited Loading

nikhita commented Dec 28, 2017

sttts Jan 2, 2018 • edited Loading

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

brancz 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

0xmichalis commented Jan 3, 2018

nikhita commented Jan 3, 2018

nikhita commented Jan 3, 2018

sttts commented Jan 3, 2018

0xmichalis commented Jan 3, 2018

nikhita commented Jan 3, 2018

nikhita commented Jan 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhita Jan 17, 2018 • edited Loading

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

sttts commented Feb 8, 2018

brancz 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

nikhita commented Feb 8, 2018

brancz commented Feb 9, 2018

sttts commented Feb 9, 2018

k8s-ci-robot commented Feb 9, 2018

k8s-github-robot commented Feb 9, 2018

k8s-github-robot commented Feb 9, 2018

nikhita commented Dec 28, 2017 •

edited

Loading

sttts Jan 2, 2018 •

edited

Loading

nikhita Jan 17, 2018 •

edited

Loading