Proposal: Introduce Custom Metrics API #152

DirectXMan12 · 2016-12-07T19:59:47Z

This proposal details the custom metrics API as proposed in the new
monitoring vision. It is designed for use with the HPA, but should be
generally useful for controllers that wish to consumer custom metrics.

(transferred from kubernetes/kubernetes#34586)

DirectXMan12 · 2016-12-07T20:00:30Z

cc @kubernetes/autoscaling @kubernetes/sig-instrumentation @piosz @fgrzadkowski @smarterclayton @derekwaynecarr

luxas · 2016-12-18T10:55:31Z

+1 for this

Any ETA on the first implementation?

Also, I heard that InfraStore might be a thing during 2017, are there any proposals/code/docs for that yet?

DirectXMan12 · 2016-12-20T15:04:01Z

alright, this should address @jszczepkowski comments.

I've tweaked the APIs a bit to make sure they work with the normal mechanics for the API server (basically, we'd register a namespaced resource/subresource of "objects/metrics", where the "name" of the object is the string-form GroupResource, and the metrics subresource is a GetterWithOptions, so that it gets the path after /metrics/ and the URL parameters. @kubernetes/api-review-team unless there's a better way to do this? The alternative seems to be using a custom chain (like the discovery server) and using go-restful manually, but it seems like that would have a higher maintenance burden. WDYT?

DirectXMan12 · 2016-12-20T15:25:29Z

@luxas first, we need to get the repo created and API types merged. Then I'll probably do a PoC implementation.

jszczepkowski · 2016-12-21T09:32:27Z

contributors/design-proposals/custom-metrics-api.md

+  retrieve the given metric for all non-namespaced objects (e.g. Node,
+  PersistentVolume) of the given type matching the given label selector.
+
+- `/objects/{object-type}/metrics/{metric-name...}?names=foo,bar`:


s/objects/root-objects?

jszczepkowski · 2016-12-21T09:42:31Z

contributors/design-proposals/custom-metrics-api.md

+    Timestamp unversioned.Time `json:"timestamp"`
+
+    // indicates the duration of the time window containing these metrics
+    Window    unversioned.Duration `json:"window"`


nit: please remove extra spaces between "Window" and "unversionde".

this comment was not applied

jszczepkowski · 2016-12-21T12:43:14Z

contributors/design-proposals/custom-metrics-api.md

+                "name": "server2",
+                "namespace": "webapp"
+            },
+            "timestamp": SOME_TIMESTAMP_HERE,


Does the timestamps in lines 108 and 118 must be the same? I guess no. So, maybe rename it some_timestampa_1 and some_timestamp_2?

jszczepkowski · 2016-12-21T12:43:53Z

In general LGTM, just two nits.

DirectXMan12 · 2016-12-22T15:59:15Z

I've actually had a conversation with @liggitt as part of the API review, and he had some suggestions for some tweaks to the API paths to make them easier to integrate with our auth patterns, discovery schema, etc. I should have those up shortly.

DirectXMan12 · 2017-01-04T19:22:34Z

alright, new revision is up, bringing the API more in line with the structure of normal Kubernetes APIs. PTAL

jszczepkowski · 2017-01-05T15:04:54Z

contributors/design-proposals/custom-metrics-api.md

+Since the HPA itself is only interested in groups of pods (by name or
+label selector) or in individual objects, one could potentially argue that
+it would be better to have separate endpoints for pods vs other objects.
+The complicates the API structure a bit, but does make it possible to


s/The complicates/This complicates/

jszczepkowski · 2017-01-05T15:05:55Z

contributors/design-proposals/custom-metrics-api.md

+  metric for all non-namespaced objects of the given type matching the
+  given label selector
+
+- `/namespaces/{namespace-name}/metrics/{metric-name}`: retrieve the given


So, object-type cannot equal "metrics"?

yes, but there's no type metrics in the legacy API group (which is the only group that has no explicit group name).

piosz · 2017-01-05T20:08:28Z

contributors/design-proposals/custom-metrics-api.md

+- `/{object-type}/{object-name}/{metric-name...}`:  retrieve the given
+  metric for the given non-namespaced object (e.g. Node, PersistentVolume)
+
+- `/{object-type}/*/{metric-name...}`: retrieve the given metric for all


Could you please add an explanation what is the motivation behind the star?

piosz · 2017-01-05T20:11:43Z

contributors/design-proposals/custom-metrics-api.md

+- `/namespaces/{namespace-name}/{object-type}/*/{metric-name...}`: retrieve the given metric for all
+  non-namespaced objects of the given type
+
+- `/namespaces/{namespace-name}/{object-type}/*/{metric-name...}?labelSelector=foo`: retrieve the given


As discussed offline from HPA point of view when querying for metrics from a set of pods (defined by label selector) we expect to have them from the same point in time to be able to perform some aggregations on them.

I've added a section under semantics indicating that implementors should attempt to return results from roughly the same timestamp, but that clients should verify.

piosz · 2017-01-05T20:13:40Z

contributors/design-proposals/custom-metrics-api.md

+    // these metrics come from some time in [Timestamp-Window, Timestamp])
+    Timestamp unversioned.Time `json:"timestamp"`
+
+    // indicates the duration of the time window containing these metrics


To support both types of metrics: instantaneous and from some time window we may want to introduce the rule that when the window is equal to 0 this means that the metric is instantaneous.

piosz · 2017-01-05T20:17:24Z

cc @kubernetes/sig-instrumentation-misc @kubernetes/sig-autoscaling-misc

piosz · 2017-01-05T20:24:04Z

cc @fabxc @irabinovitch

DirectXMan12 · 2017-01-05T21:57:42Z

@kubernetes/sig-autoscaling-misc updated as discussed in the SIG meeting, PTAL

liggitt · 2017-01-16T21:37:22Z

contributors/design-proposals/custom-metrics-api.md

+
+### Metric Names ###
+
+Metric names must be escaped so that any given metric appears as a single


escaped how?

This is important to get defined as there's been issues with URL escaping previously (such as %2F which Golang http library incorrectly handles). And also if there are restrictions.

@liggitt do we have a recommended way of escaping then? I would have said normal URL escaping, but I was not aware that Go's HTTP library was broken like that.

rather than try to allow a way to include an escaped "/" in an object name, kubernetes objects are simply not allowed to have names containing / or % characters, and are not allowed to be . or ..

that allows you to use normal URL path segment encoding to safely plumb values through

I supposed we could just disallow the same things here.

liggitt · 2017-01-16T21:38:34Z

contributors/design-proposals/custom-metrics-api.md

+The `object-type` parameter should be the string form of
+`unversioned.GroupResource`.  Note that we do not include version in this;
+we simply wish to uniquely identify all the different types of objects in
+Kubernetes.


a couple examples here would be helpful (maybe for pods in the legacy group, and one for jobs in the batch group)

liggitt · 2017-01-17T03:05:02Z

contributors/design-proposals/custom-metrics-api.md

+For metrics systems that support differentiating metrics beyond the Kubernetes
+object hierarchy (such as using additional labels), the metrics systems should
+have a metric which represents all such series aggregated together.
+Additionally, implementors may choose to the individual "sub-metrics" via the


may choose to identify?

liggitt · 2017-01-17T03:05:55Z

contributors/design-proposals/custom-metrics-api.md

+
+The types and clients for this API will live in a separate repository
+under the Kubernetes organization (e.g. `kubernetes/metrics`).  This
+respository will most likely also house other metrics-related APIs for


liggitt · 2017-01-17T03:07:48Z

contributors/design-proposals/custom-metrics-api.md

+- `/{object-type}/*/{metric-name...}`: retrieve the given metric for all
+  non-namespaced objects of the given type
+
+- `/{object-type}/*/{metric-name...}?labelSelector=foo`: retrieve the


this is expected to return a MetricValueList?

liggitt · 2017-01-17T03:08:07Z

contributors/design-proposals/custom-metrics-api.md

+The root API path will look like `/apis/custom-metrics/v1alpha1`.  For
+brevity, this will be left off below.
+
+- `/{object-type}/{object-name}/{metric-name...}`:  retrieve the given


and this is expected to return a MetricValue?

if this same url pattern (with an object-name of *) is expected to return a MetricValueList, this should as well

yeah, all of these URLs return a MetricValueList. I'll clarify that (I missed that the API objects section I had written still said that some URLs returned the singular form).

liggitt · 2017-01-17T03:09:10Z

contributors/design-proposals/custom-metrics-api.md

+    unversioned.ListMeta `json:"metadata,omitempty"`
+
+    // the value of the metric across the described objects
+    Items []MetricValue `json:"metricValues"`


json:"items"

@smarterclayton can correct me if I'm wrong, but I think API lists have to contain runtime.Objects, which means TypeMeta on MetricValue

liggitt · 2017-01-17T03:11:59Z

contributors/design-proposals/custom-metrics-api.md

+  given metric for all non-namespaced objects of the given type matching
+  the given label selector
+
+- `/namespaces/{namespace-name}/{object-type}/{object-name}/{metric-name...}`:


would object-name be expected to match namespace-name?

@liggitt - i don't follow.

i assumed this was saying:

/namespaces/ns1/services/myservice/my-metric

hmm, this was supposed to be attached to the "metrics for a namespace" example:

- `/namespaces/{namespace-name}/metrics/{metric-name}`: retrieve the given metric which describes the given namespace.

liggitt · 2017-01-17T03:15:20Z

contributors/design-proposals/custom-metrics-api.md

+
+Note that we introduce the syntax of having a name of ` * ` here since
+there is no current syntax for getting the output of a subresource on
+multiple objects.


this is also what might make it necessary to return a MetricsValueList from the subresource, even when dealing with a single named object.

liggitt · 2017-01-17T03:16:47Z

contributors/design-proposals/custom-metrics-api.md

+### Metric Values and Timing ###
+
+There should be only one metric value per object requested.  The returned
+metrics should be the most recenly available metrics, as with the resource


liggitt · 2017-01-17T03:18:09Z

contributors/design-proposals/custom-metrics-api.md

+corresponds to a URL of the form
+`/namespaces/$NS/pods/*/$METRIC_NAME?labelSelector=foo`, while the
+`object` source type corresponds to a URL of the form
+`/namespaces/$NS/$KIND.$GROUP/$OBJECT_NAME/$METRIC_NAME`.


$RESOURCE.$GROUP?

liggitt · 2017-01-17T03:19:02Z

contributors/design-proposals/custom-metrics-api.md

+This API is intended to be implemented by monitoring pipelines (e.g.
+inside Heapster, or as an adapter on top of a solution like Prometheus).
+It shares many mechanical requirements with normal Kubernetes APIs, such
+as needed to support encoding different versions of objects in both JSON


"such as the need"?

liggitt · 2017-01-17T03:23:50Z

contributors/design-proposals/custom-metrics-api.md

+reasons, it is expected that implemenators will make use of the Kubernetes
+genericapiserver code.  If implementors choose not to use this, they must
+still follow all of the Kubernetes API server conventions in order to work
+properly with consumers of the API.


make this more specific, especially around which API verbs are supported (appears only to be get?) and what semantics resourceVersion carries for MetricValue objects

vishh · 2017-01-17T22:03:11Z

contributors/design-proposals/custom-metrics-api.md

+    // which these metrics were calculated, when returning rate
+    // metrics calculated from cumulative metrics (or zero for
+    // non-calculated instantaneous metrics).
+    Window  unversioned.Duration `json:"window"`


I can think of three different types of metrics:

Guage (memory usage for example)

Cumulative (API hits since start, uptime, etc)

Rate (API hits per second)

With Window are we intending to state that the metric is of type 3? What about 2 and 1, how are they represented?

From past discussion my take away was that the exact semantic meaning is left to the provider of the custom metrics. Integrating systems behave differently and there's little point in specifying something here we can't enforce anyway.

I'd consider window as a return attribute a gimmick. It's much more likely that one wants to specify it in the request as the aggregation window has significant effect on the value.
Let's say we implemented a proxy for Prometheus. We'd follow the recording rules (materialized queries) naming pattern like:

/...//namespaces/webapp/ingress.extensions/*/requests:rate5m?labelSelector=app%3Dfrontend

The window is specified in the requested metric and the proxy would merely copy that into the window field in the response.

How is window used in the HPA? It seems tedious to extract from the metric name in this example – especially if users violate naming conventions.

@vishh for cases where window doesn't apply, it is simply set to zero. Window only applies in cases where you have a derived metric (e.g. cpu usage).

I'd consider window as a return attribute a gimmick. It's much more likely that one wants to specify it in the request as the aggregation window has significant effect on the value.

Recapping the discussion in SIG instrumentation:

This value is set to inform the consumers what range of data was used to produce a particular value, so they can avoid making decisions on largely overlapping data (e.g. in the HPA, we might want to wait until we get mostly non-overlapping data to make a scale-up decision after an initial scale up, so that we don't over scale).

Isn't timestamp enough for HPA? I assume HPA will only deal with instantaneous metrics?

DirectXMan12 · 2017-01-19T19:44:40Z

@liggitt I think I've address most of your comments.

re the namespace path, it follows a slightly different pattern (we use a "resource" metrics, and the object names are metric name), since otherwise we'd have to have redundant object name -- with the former, we have /namespaces/foo/metrics/mymetric whereas with the latter we'd need /namespaces/foo/metrics/foo/mymetric.

liggitt · 2017-01-19T19:47:33Z

re the namespace path, it follows a slightly different pattern (we use a "resource" metrics, and the object names are metric name), since otherwise we'd have to have redundant object name -- with the former, we have /namespaces/foo/metrics/mymetric whereas with the latter we'd need /namespaces/foo/metrics/foo/mymetric.

do you see it being important to select namespace metrics across all namespaces (or by labelSelector)?

DirectXMan12 · 2017-01-19T19:50:53Z

do you see it being important to select namespace metrics across all namespaces (or by labelSelector)?

not particularly, no. Selecting things inside a namespace is useful, but since namespaces are the "boundary", selecting across all namespaces seems less of a big deal. The one case where it might be useful is dashboards, but even there, I'm not sure how useful it would be.

derekwaynecarr · 2017-01-20T18:44:53Z

contributors/design-proposals/custom-metrics-api.md

+    // which these metrics were calculated, when returning rate
+    // metrics calculated from cumulative metrics (or zero for
+    // non-calculated instantaneous metrics).
+    Window  unversioned.Duration `json:"window"`


usage of duration has been a problem, do we need subsecond here.

see:
https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#naming-conventions
https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#units

see:
kubernetes/kubernetes#39914 (comment)

if we dont need sub-second, may be best to move the window to WindowSeconds

derekwaynecarr · 2017-01-20T20:43:56Z

contributors/design-proposals/custom-metrics-api.md

+    // which these metrics were calculated, when returning rate
+    // metrics calculated from cumulative metrics (or zero for
+    // non-calculated instantaneous metrics).
+    WindowSeconds int64 `json:"window"`


hate to nit here, but why not make this a pointer so we can ignore the zero case?

or just omitempty so we don't have to deal with a pointer

derekwaynecarr · 2017-01-20T21:15:38Z

both ActiveDeadlineSeconds and TerminationGracePeriodSeconds are pointers. so basically, we are always pointers on these things right now.

…

On Fri, Jan 20, 2017 at 3:46 PM, Solly Ross ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In contributors/design-proposals/custom-metrics-api.md <#152>: > + metav1.TypeMeta`json:",inline"` + + // a reference to the described object + DescribedObject ObjectReference `json:"describedObject"` + + // the name of the metric + MetricName string `json:"metricName"` + + // indicates the time at which the metrics were produced + Timestamp unversioned.Time `json:"timestamp"` + + // indicates the window ([Timestamp-Window, Timestamp]) from + // which these metrics were calculated, when returning rate + // metrics calculated from cumulative metrics (or zero for + // non-calculated instantaneous metrics). + WindowSeconds int64 `json:"window"` or just omitempty so we don't have to deal with a pointer — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#152>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF8dbJuihToWV2Hp6iWCJzEZA7aWQnQpks5rUR0egaJpZM4LHCWX> .

DirectXMan12 · 2017-01-26T05:15:04Z

both ActiveDeadlineSeconds and TerminationGracePeriodSeconds are pointers. so basically, we are always pointers on these things right now.

ok, done.

@liggitt @vishh @piosz @derekwaynecarr any more thoughts, or is this good to merge?

piosz · 2017-01-31T11:15:41Z

LGTM from me.

derekwaynecarr · 2017-01-31T23:12:40Z

contributors/design-proposals/custom-metrics-api.md

+The resource source type is taken from the API provided by the
+"metrics" API group (the master/resource metrics API).
+
+The HPA will consume the API as a federated API server.


nit: assume we mean aggregated API server now?

derekwaynecarr · 2017-01-31T23:14:43Z

LGTM from me, we should augment in the future how we choose to promote this to a non v1alpha1 form. For example, require community demonstrated adapters that conform to the model.

This proposal details the custom metrics API as proposed in the new monitoring vision. It is designed for use with the HPA, but should be generally useful for controllers that wish to consumer custom metrics.

DirectXMan12 · 2017-02-02T16:10:42Z

latest update just clarifies that the LIST operations on branch nodes (i.e. those operations not defined in the list of API paths) are not currently supported.

liggitt · 2017-02-02T16:11:47Z

LGTM

Remove extraneous info from draft

…etrics-api Proposal: Introduce Custom Metrics API

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 7, 2016

DirectXMan12 mentioned this pull request Dec 7, 2016

Proposal: Introduce Custom Metrics API kubernetes/kubernetes#34586

Closed

fgrzadkowski assigned fgrzadkowski and jszczepkowski Dec 8, 2016

fgrzadkowski requested a review from piosz December 8, 2016 14:06

DirectXMan12 assigned mwielgus and unassigned fgrzadkowski Dec 15, 2016

DirectXMan12 force-pushed the proposal/custom-metrics-api branch from 71b09fc to 3ed5f62 Compare December 19, 2016 21:27

jszczepkowski reviewed Dec 21, 2016

View reviewed changes

DirectXMan12 force-pushed the proposal/custom-metrics-api branch from 3ed5f62 to 5fd3aef Compare January 4, 2017 19:21

jszczepkowski reviewed Jan 5, 2017

View reviewed changes

piosz assigned piosz and unassigned mwielgus Jan 5, 2017

piosz suggested changes Jan 5, 2017

View reviewed changes

DirectXMan12 force-pushed the proposal/custom-metrics-api branch 2 times, most recently from 363b555 to 0ffdbe6 Compare January 5, 2017 21:57

ngtuna mentioned this pull request Jan 9, 2017

Add prometheus/HPA vmware-archive/kubeless#10

Closed

piosz assigned fabxc Jan 13, 2017

liggitt reviewed Jan 16, 2017

View reviewed changes

piosz assigned liggitt Jan 16, 2017

liggitt reviewed Jan 17, 2017

View reviewed changes

vishh reviewed Jan 17, 2017

View reviewed changes

DirectXMan12 force-pushed the proposal/custom-metrics-api branch from eb443a2 to cc963b3 Compare January 19, 2017 19:41

derekwaynecarr reviewed Jan 20, 2017

View reviewed changes

DirectXMan12 force-pushed the proposal/custom-metrics-api branch from cc963b3 to e094b55 Compare January 20, 2017 20:22

fabxc approved these changes Jan 20, 2017

View reviewed changes

derekwaynecarr reviewed Jan 20, 2017

View reviewed changes

DirectXMan12 force-pushed the proposal/custom-metrics-api branch from e094b55 to a53ae2c Compare January 20, 2017 20:47

DirectXMan12 force-pushed the proposal/custom-metrics-api branch from a53ae2c to 5236fd2 Compare January 26, 2017 05:14

derekwaynecarr reviewed Jan 31, 2017

View reviewed changes

Proposal: Introduce Custom Metrics API

7cd59be

This proposal details the custom metrics API as proposed in the new monitoring vision. It is designed for use with the HPA, but should be generally useful for controllers that wish to consumer custom metrics.

DirectXMan12 force-pushed the proposal/custom-metrics-api branch from 5236fd2 to 7cd59be Compare February 2, 2017 16:03

piosz merged commit 657df96 into kubernetes:master Feb 2, 2017

DirectXMan12 deleted the proposal/custom-metrics-api branch February 2, 2017 16:58

shyamjvs pushed a commit to shyamjvs/community that referenced this pull request Sep 22, 2017

Merge pull request kubernetes#152 from kubernetes/draft-format-cleanup

c7dd889

Remove extraneous info from draft

MadhavJivrajani pushed a commit to MadhavJivrajani/community that referenced this pull request Nov 30, 2021

Merge pull request kubernetes#152 from DirectXMan12/proposal/custom-m…

f4a6a05

…etrics-api Proposal: Introduce Custom Metrics API


		### Metric Names ###

		Metric names must be escaped so that any given metric appears as a single

Proposal: Introduce Custom Metrics API #152

Proposal: Introduce Custom Metrics API #152

Uh oh!

Conversation

DirectXMan12 commented Dec 7, 2016

Uh oh!

DirectXMan12 commented Dec 7, 2016

Uh oh!

luxas commented Dec 18, 2016

Uh oh!

DirectXMan12 commented Dec 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DirectXMan12 commented Dec 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jszczepkowski commented Dec 21, 2016

Uh oh!

DirectXMan12 commented Dec 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DirectXMan12 commented Jan 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

piosz commented Jan 5, 2017

Uh oh!

piosz commented Jan 5, 2017

Uh oh!

DirectXMan12 commented Jan 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 commented Dec 20, 2016 •

edited

Loading

DirectXMan12 commented Dec 22, 2016 •

edited

Loading

DirectXMan12 Jan 19, 2017 •

edited

Loading

derekwaynecarr Jan 31, 2017 •

edited

Loading