Add and update metric status #5395

taragu · 2019-09-04T19:53:43Z

/lint

Proposed Changes

Currently, there's no status field for metric so there's no way of knowing the readiness of metric objects. This PR adds and updates the metric status.

Release Note

NONE

/cc @nak3
/cc @jonjohnsonjr

knative-prow-robot

@taragu: 0 warnings.

In response to this:

/lint

Fixes #5304

Proposed Changes

Currently, there's no status field for metric so there's no way of knowing the readiness of metric objects. This PR adds the metric status and propagate it to pod autoscaler.

Release Note
NONE
/cc @nak3
/cc @jonjohnsonjr

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.

pkg/reconciler/autoscaling/kpa/scaler.go

pkg/reconciler/autoscaling/kpa/kpa_test.go

pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go

vagababov

metric_lifecycle.go needs unit tests.

pkg/reconciler/metric/metric.go

pkg/apis/autoscaling/v1alpha1/metric_lifecycle.go

pkg/reconciler/autoscaling/reconciler.go

vagababov · 2019-09-05T16:35:59Z

pkg/apis/autoscaling/v1alpha1/metric_lifecycle_test.go

+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			if e, a := tc.isReady, tc.status.IsReady(); e != a {
+				t.Errorf("%q expected: %v got: %v", tc.name, e, a)


Ready = %v, want: %v
no need to log tc.name, t.Run takes care of that.

vagababov · 2019-09-05T16:43:33Z

pkg/apis/autoscaling/v1alpha1/metric_types.go

@@ -63,7 +64,9 @@ type MetricSpec struct {
 }

 // MetricStatus reflects the status of metric collection for this specific entity.
-type MetricStatus struct{}
+type MetricStatus struct {


FWIW, we can just do type MetricStatus duckv1beta1.Status?

@vagababov I'm not sure if type alias is appropriate here, since it's for gradual code repair while moving a type between packages during large-scale refactoring

vagababov · 2019-09-05T16:44:28Z

pkg/reconciler/metric/metric.go

 	if err := r.collector.CreateOrUpdate(metric); err != nil {
+		if err.Error() == "failed to get endpoints" {


I'd rather we switched to typed errors (e.g. collector.ErrFailedGetEndpoints) and then check equality.

savitaashture · 2019-09-06T06:17:46Z

pkg/apis/autoscaling/v1alpha1/metric_lifecycle_test.go

+	apitest.CheckConditionOngoing(m.duck(), MetricConditionReady, t)
+
+	const wantReason = "reason"
+	const wantMessage = "the error message"


Suggested change

const wantMessage = "the error message"

const (

wantReason = "reason"

wantMessage = "the error message"

)

savitaashture · 2019-09-06T06:18:25Z

pkg/apis/autoscaling/v1alpha1/metric_lifecycle_test.go

+	}
+
+	ms.MarkMetricReady()
+


Please remove extra lines from all the places

savitaashture · 2019-09-06T06:20:03Z

pkg/apis/autoscaling/v1alpha1/metric_lifecycle_test.go

+func TestMetricGetGroupVersionKind(t *testing.T) {
+	r := &Metric{}
+	want := schema.GroupVersionKind{
+		Group:   "autoscaling.internal.knative.dev",


I'd rather use autoscaling.InternalGroupName

…emove unused consts

vagababov · 2019-09-06T16:30:51Z

pkg/autoscaler/stats_scraper.go

+var (
+	// errFailedGetEndpoints specifies the error returned by scraper when it fails to
+	// get endpoints.
+	errFailedGetEndpoints = "failed to get endpoints"


why not just errFailedGetEndpoints = errors.New("failed to get endpoints")?

vagababov · 2019-09-06T16:31:07Z

pkg/autoscaler/stats_scraper.go

@@ -131,7 +141,7 @@ func urlFromTarget(t, ns string) string {
 func (s *ServiceScraper) Scrape() (*StatMessage, error) {
 	readyPodsCount, err := s.counter.ReadyCount()
 	if err != nil {
-		return nil, errors.Wrap(err, "failed to get endpoints")
+		return nil, errors.Wrap(err, errFailedGetEndpoints)


log and return canned error

vagababov · 2019-09-06T18:23:03Z

pkg/autoscaler/stats_scraper.go

+var (
+	// errFailedGetEndpoints specifies the error returned by scraper when it fails to
+	// get endpoints.
+	errFailedGetEndpoints = errors.New("failed to get endpoints")


If you make these public, then the methods below will not be required
and you can just check the errors for equality since you are returning the exact object.

This is usually how go does this kind of stuff (e.g. https://golang.org/pkg/io/#pkg-variables)

Cool!! That's good to know

vagababov · 2019-09-06T20:33:35Z

pkg/autoscaler/collector.go

@@ -261,15 +261,17 @@ func newCollection(metric *av1alpha1.Metric, scraper StatsScraper, logger *zap.S
 			case <-scrapeTicker.C:
 				message, err := c.getScraper().Scrape(logger)
 				if err != nil {
+					copy := *(metric.DeepCopy())


Don't dereference here

vagababov · 2019-09-06T20:34:03Z

pkg/autoscaler/collector_test.go

-			wait.PollImmediate(10*time.Millisecond, 2*time.Second, func() (bool, error) {
-				got = test.metric.Status.Status
-				return equality.Semantic.DeepEqual(got, test.expectedMetricStatus), nil
+			wait.PollImmediate(10*time.Millisecond, 5*time.Second, func() (bool, error) {


why did we change time?

Yeah this is not necessary. Just removed

knative-metrics-robot · 2019-09-06T20:45:02Z

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/autoscaler/collector.go	95.6%	96.9%	1.3
pkg/autoscaler/stats_scraper.go	91.2%	89.8%	-1.4
pkg/reconciler/metric/metric.go	92.3%	93.3%	1.0

vagababov

/lgtm
/approve

knative-prow-robot · 2019-09-06T20:45:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: taragu, vagababov

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:

~~pkg/apis/autoscaling/OWNERS~~ [vagababov]
~~pkg/autoscaler/OWNERS~~ [vagababov]
~~pkg/reconciler/OWNERS~~ [vagababov]

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

taragu · 2019-09-06T21:17:44Z

/test pull-knative-serving-unit-tests

nak3 · 2019-09-07T07:34:26Z

Does this patch work well? I think UpdateStatus() needs to be called somewhere, but I cannot find it anywhere.

vagababov · 2019-09-07T19:12:22Z

/reopen
Nope.

knative-prow-robot · 2019-09-07T19:12:23Z

@vagababov: Failed to re-open PR: state cannot be changed. The pull request cannot be reopened.

In response to this:

/reopen
Nope.

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.

knative-prow-robot requested review from jonjohnsonjr and nak3 September 4, 2019 19:53

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 4, 2019

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 4, 2019

knative-prow-robot reviewed Sep 4, 2019

View reviewed changes

knative-prow-robot added area/API API objects and controllers area/autoscale labels Sep 4, 2019

vagababov reviewed Sep 4, 2019

View reviewed changes

pkg/reconciler/autoscaling/kpa/scaler.go Outdated Show resolved Hide resolved

pkg/reconciler/autoscaling/kpa/kpa_test.go Outdated Show resolved Hide resolved

pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go Outdated Show resolved Hide resolved

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 4, 2019

taragu changed the title ~~Add and propagate metric status to autoscaler~~ Add and update metric status Sep 4, 2019

taragu force-pushed the metric-status branch from 8ef72a4 to 010e315 Compare September 4, 2019 22:14

vagababov reviewed Sep 4, 2019

View reviewed changes

pkg/reconciler/metric/metric.go Outdated Show resolved Hide resolved

pkg/apis/autoscaling/v1alpha1/metric_lifecycle.go Outdated Show resolved Hide resolved

pkg/reconciler/autoscaling/reconciler.go Outdated Show resolved Hide resolved

knative-prow-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 Sep 5, 2019

vagababov reviewed Sep 5, 2019

View reviewed changes

savitaashture reviewed Sep 6, 2019

View reviewed changes

Tara Gu added 5 commits September 6, 2019 10:02

Add status field to metrics object

5bc7599

Codegen

821e7ee

Remove propagate metric status to PA

338e0c8

Add metric lifecycle tests; check err for collector.CreateOrUpdate; r…

5bb7073

…emove unused consts

Review

0adf580

taragu force-pushed the metric-status branch from df7ea55 to 0adf580 Compare September 6, 2019 14:03

vagababov reviewed Sep 6, 2019

View reviewed changes

Review: error handling in scraper

07b3bd8

taragu force-pushed the metric-status branch from e9fdda4 to 07b3bd8 Compare September 6, 2019 18:10

vagababov reviewed Sep 6, 2019

View reviewed changes

Review comment; add test for metric collector error use case

42ec3a3

taragu force-pushed the metric-status branch from fc42144 to 42ec3a3 Compare September 6, 2019 19:47

Fix data race

c47c3dd

vagababov reviewed Sep 6, 2019

View reviewed changes

Review

b51f686

vagababov reviewed Sep 6, 2019

View reviewed changes

knative-prow-robot assigned vagababov Sep 6, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 6, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 6, 2019

knative-prow-robot merged commit 2130d9f into knative:master Sep 6, 2019

nak3 mentioned this pull request Sep 7, 2019

Metrics still does not have status field #5425

Closed

vagababov mentioned this pull request Sep 9, 2019

Normally compute and propagate metric status #5437

Merged

taragu mentioned this pull request Sep 16, 2019

Update metric status when processing fails #5556

Closed

taragu deleted the metric-status branch February 18, 2020 15:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add and update metric status #5395

Add and update metric status #5395

taragu commented Sep 4, 2019 •

edited

Loading

knative-prow-robot left a comment

vagababov left a comment

vagababov Sep 5, 2019

vagababov Sep 5, 2019

taragu Sep 6, 2019

vagababov Sep 5, 2019

savitaashture Sep 6, 2019

savitaashture Sep 6, 2019

savitaashture Sep 6, 2019 •

edited

Loading

vagababov Sep 6, 2019

vagababov Sep 6, 2019

vagababov Sep 6, 2019

taragu Sep 6, 2019

vagababov Sep 6, 2019

vagababov Sep 6, 2019

taragu Sep 6, 2019

knative-metrics-robot commented Sep 6, 2019

vagababov left a comment

knative-prow-robot commented Sep 6, 2019

taragu commented Sep 6, 2019

nak3 commented Sep 7, 2019 •

edited

Loading

vagababov commented Sep 7, 2019

knative-prow-robot commented Sep 7, 2019

		if err := r.collector.CreateOrUpdate(metric); err != nil {
		if err.Error() == "failed to get endpoints" {

Add and update metric status #5395

Add and update metric status #5395

Conversation

taragu commented Sep 4, 2019 • edited Loading

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

vagababov 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

Choose a reason for hiding this comment

savitaashture Sep 6, 2019 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-metrics-robot commented Sep 6, 2019

vagababov left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Sep 6, 2019

taragu commented Sep 6, 2019

nak3 commented Sep 7, 2019 • edited Loading

vagababov commented Sep 7, 2019

knative-prow-robot commented Sep 7, 2019

taragu commented Sep 4, 2019 •

edited

Loading

savitaashture Sep 6, 2019 •

edited

Loading

nak3 commented Sep 7, 2019 •

edited

Loading