New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Collect prometheus metrics for custom resources #57682

Merged
merged 2 commits into from Feb 9, 2018

Conversation

@nikhita
Member

nikhita commented Dec 28, 2017

Enables apiserver metrics for custom resources.

Fixes #55146

Release note:

Enable apiserver metrics for custom resources.

/cc sttts deads2k kargakis brancz

@nikhita

This comment has been minimized.

Member

nikhita commented Dec 28, 2017

/area custom-resources

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

This comment has been minimized.

@sttts

sttts Jan 2, 2018

Contributor

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

This comment has been minimized.

@nikhita

nikhita Jan 2, 2018

Member

Yes, will do. 👍

This comment has been minimized.

@nikhita

nikhita Jan 2, 2018

Member

Done.

@k8s-ci-robot k8s-ci-robot added size/M size/S and removed size/S size/M labels Jan 2, 2018

@@ -57,16 +57,7 @@ func (r *ProxyHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
var httpCode int
var requestInfo *request.RequestInfo
defer func() {

This comment has been minimized.

@sttts

sttts Jan 2, 2018

Contributor

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

@brancz

Just one comment, otherwise lgtm

@@ -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)

This comment has been minimized.

@brancz

brancz Jan 3, 2018

Member

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.

This comment has been minimized.

@sttts

sttts Jan 3, 2018

Contributor

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?

This comment has been minimized.

@brancz

brancz Jan 3, 2018

Member

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

@kargakis

This comment has been minimized.

Member

kargakis commented Jan 3, 2018

Is the crdHandler handling CRDs or CRs?

@nikhita

This comment has been minimized.

Member

nikhita commented Jan 3, 2018

Is the crdHandler handling CRDs or CRs?

CRs.

@nikhita

This comment has been minimized.

Member

nikhita commented Jan 3, 2018

Is the crdHandler handling CRDs or CRs?

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

@sttts

This comment has been minimized.

Contributor

sttts commented Jan 3, 2018

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?

@kargakis

This comment has been minimized.

Member

kargakis commented Jan 3, 2018

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

This comment has been minimized.

Member

nikhita commented Jan 3, 2018

Do we have metrics problems as well for CRDs?

No, metrics for CRDs works fine.

@nikhita

This comment has been minimized.

Member

nikhita commented Jan 3, 2018

/retest

@@ -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) {

This comment has been minimized.

@sttts

sttts Jan 5, 2018

Contributor

Compare

func InstrumentRouteFunc(verb, resource, subresource, scope string, routeFunc restful.RouteFunction) restful.RouteFunction {
. Can you move this there and align the signature?

This comment has been minimized.

@sttts

sttts Jan 15, 2018

Contributor

Any blockers here?

This comment has been minimized.

@nikhita

nikhita Jan 17, 2018

Member

Any blockers here?

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

This comment has been minimized.

@kargakis

This comment has been minimized.

@nikhita

nikhita Feb 7, 2018

Member

@sttts updated to use InstrumentHandlerFunc. ptal, thanks.

@k8s-ci-robot k8s-ci-robot added size/M and removed size/S labels Feb 7, 2018

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

This comment has been minimized.

@sttts

sttts Feb 8, 2018

Contributor

godoc

This comment has been minimized.

@nikhita

nikhita Feb 8, 2018

Member

Done.

@sttts

This comment has been minimized.

Contributor

sttts commented Feb 8, 2018

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

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

This comment has been minimized.

@brancz

brancz Feb 8, 2018

Member

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

This comment has been minimized.

@sttts

sttts Feb 8, 2018

Contributor

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.

This comment has been minimized.

@nikhita

nikhita Feb 8, 2018

Member

Done.

@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Feb 8, 2018

nikhita added some commits Dec 28, 2017

Collect prometheus metrics for custom resources
Since we have a custom handler for apiextensions-apiserver,
we need to record the metrics here.
@nikhita

This comment has been minimized.

Member

nikhita commented Feb 8, 2018

/retest

@brancz

This comment has been minimized.

Member

brancz commented Feb 9, 2018

/lgtm

@sttts

This comment has been minimized.

Contributor

sttts commented Feb 9, 2018

/approve

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 9, 2018

[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:

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 9, 2018

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 9, 2018

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

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

12 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-verify
Details
cla/linuxfoundation nikhita authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@kargakis kargakis referenced this pull request Apr 6, 2018

Merged

Decorate PodSpec with initcontainers #7594

10 of 10 tasks complete

@nikhita nikhita deleted the nikhita:customresource-metrics branch Jul 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment