Add activator metrics and dashboard by akyyy · Pull Request #1567 · knative/serving

akyyy · 2018-07-10T22:49:48Z

Fixes #743

Proposed Changes

Add activator metrics
Add activator dashboard

google-prow-robot · 2018-07-10T22:49:52Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: akyyy
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: mattmoor

If they are not already assigned, you can assign the PR to them by writing /assign @mattmoor in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

OWNERS

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

steuhs · 2018-07-10T23:14:32Z

/test pull-knative-serving-go-coverage-dev

steuhs · 2018-07-10T23:20:46Z

/test pull-knative-serving-go-coverage

steuhs · 2018-07-10T23:26:51Z

/test pull-knative-serving-go-coverage-dev

steuhs · 2018-07-10T23:39:58Z

/test pull-knative-serving-go-coverage-dev

steuhs · 2018-07-10T23:54:23Z

/test pull-knative-serving-go-coverage-dev

steuhs · 2018-07-10T23:58:26Z

/test pull-knative-serving-go-coverage-dev

steuhs · 2018-07-11T00:23:30Z

/test pull-knative-serving-go-coverage-dev

steuhs · 2018-07-11T00:38:49Z

/test pull-knative-serving-go-coverage

steuhs · 2018-07-11T03:08:11Z

/test pull-knative-serving-go-coverage

steuhs · 2018-07-11T04:01:04Z

/test pull-knative-serving-go-coverage-dev

steuhs · 2018-07-11T04:12:28Z

/test pull-knative-serving-go-coverage-dev

steuhs · 2018-07-11T04:26:55Z

/test pull-knative-serving-go-coverage-dev

steuhs · 2018-07-11T04:38:18Z

/test pull-knative-serving-go-coverage-dev

steuhs · 2018-07-11T04:44:40Z

/test pull-knative-serving-go-coverage-dev

steuhs · 2018-07-11T05:02:43Z

/test pull-knative-serving-go-coverage

akyyy · 2018-07-12T16:37:06Z

@mdemirhan @josephburnett gentle ping. It would be nice if I could get this in by the end of this week. Thanks!

steuhs · 2018-07-12T18:21:10Z

/test pull-knative-serving-go-coverage

steuhs · 2018-07-12T18:31:28Z

/test pull-knative-serving-go-coverage-dev

mattmoor · 2018-07-12T19:38:52Z

/hold

Wait until after we cut first release.

steuhs · 2018-07-13T20:17:40Z

/test pull-knative-serving-go-coverage-dev --skipped

steuhs · 2018-07-13T22:01:32Z

/test pull-knative-serving-go-coverage-dev
/skip

knative-metrics-robot · 2018-07-13T22:03:34Z

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

File	Old Coverage	New Coverage	Delta
pkg/activator/dedupe.go	93.3%	92.3%	-1.0
pkg/activator/revision.go	80.0%	78.7%	-1.3
pkg/activator/stats_reporter.go	Do not exist	78.4%

steuhs · 2018-07-13T22:06:38Z

/skip

mdemirhan · 2018-07-23T22:07:42Z

+
 	if resp != nil {
 		rrt.logger.Infof("It took %d tries to get response code %d", i, resp.StatusCode)
+		namespace := r.Header.Get(controller.GetRevisionHeaderNamespace())


what is these are empty? Should we still send metrics?

If it's empty or doesn't exist, we'll get 500 errors based on

serving/pkg/activator/revision.go

Line 68 in 5b8542f

revision, err := revisionClient.Get(rev.name, metav1.GetOptions{})

I think we should report error for the given headers -- all requests routed to activator should have the headers set. If not, we can use the metrics to debug.

mdemirhan · 2018-07-23T22:12:12Z

+	start    time.Time
 }

 func (rrt retryRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) {


The interface implementation should be on the pointer, not the value type.

func (rrt *retryRoundTripper) RoundTrip(...

mdemirhan · 2018-07-23T22:13:42Z

+		rrt.reporter.ReportResponseCount(namespace, config, name, resp.StatusCode, i, 1.0)
+		rrt.reporter.ReportResponseTime(namespace, config, name, resp.StatusCode, time.Now().Sub(rrt.start))
 	}
 	return resp, err


Why not report anything when all retries fail with an error (i.e. no response)?

Sure, I can do that. In that case, what status code dimension you would suggest to use?

mdemirhan · 2018-07-23T22:16:45Z

 }

 func (a *activationHandler) handler(w http.ResponseWriter, r *http.Request) {
+	namespace := r.Header.Get(controller.GetRevisionHeaderNamespace())


None of this new functionality in main.go have any unit tests - see #1585 that refactoring and adding unit tests. I would prefer waiting for that to get in and adding this functionality such that it is tested properly.

All of these should be easy to test - inject headers, test edge cases, see if the reporter is getting called, ...etc.

If that pull request #1585 takes long time, I think we can get activator dashboard in first and then add unit tests based on #1585 later.

Just a note, I moved #1585 to #1689

mdemirhan · 2018-07-23T22:19:46Z

 // status code indicating why it could not.
 type Activator interface {
-	ActiveEndpoint(namespace, name string) (Endpoint, Status, error)
+	ActiveEndpoint(namespace, configuration, name string) (Endpoint, Status, error)


Is configuration needed for activation? If not, it doesn't look right to add it here. Is there no better way to get this information for the stats?

The configuration parameter is needed for activator to report metrics.

mdemirhan · 2018-07-23T22:22:10Z

 	}
 	if reqs, ok := a.pendingRequests[id]; ok {
+		a.reporter.ReportRequest(id.namespace, id.configuration, id.name, state, float64(len(reqs)))
+		logger.Infof("Wrote request_count metric for revision %s for namespace %s with value %d", id.name, id.namespace, len(reqs))


We should not write logs to say that we wrong metrics. Seems extremely verbose. If you absolutely need this, I recommend using "Debug" rather than "Info"

Deleted the log.

mdemirhan · 2018-07-23T22:25:24Z

+var (
+	measurements = []*stats.Float64Measure{
+		RequestCountM: stats.Float64(
+			"revision_request_count",


is it intentional to use the same name as revision request count from Istio? If so, do we have the same matching labels.

Yes, we do.

akyyy · 2018-07-23T22:29:30Z

@mdemirhan I added "start" field in retryRoundTripper struct to calculate the response time in function
func (rrt retryRoundTripper) RoundTrip

If we reuse one retryRoundTripper, we lost the request start time?

akyyy · 2018-07-26T22:37:21Z

This is moved to #1726 after creating a new repo.

akyyy added 10 commits July 6, 2018 11:46

add activator metrics

f04ee2f

merge

54f7b62

number of tries metric

3ee623d

add response time metrics

08ed16b

Update the dubugging dashboard

a4a4fdf

adjust dashboard order

0e2c1fc

simplify interface

f96f43f

add unit test

6c8530c

Merge remote-tracking branch 'upstream/master' into activator-metrics1

5510c7e

minor cleanup

e30b737

google-prow-robot requested review from evankanderson, josephburnett, mdemirhan and tcnghia July 10, 2018 22:49

google-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 10, 2018

akyyy assigned mdemirhan and josephburnett Jul 10, 2018

minor change

d3b88fd

google-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 12, 2018

mdemirhan reviewed Jul 23, 2018

View reviewed changes

akyyy closed this Jul 26, 2018

akyyy mentioned this pull request Jul 26, 2018

Add activator metrics and dashboard #1726

Merged

Conversation

akyyy commented Jul 10, 2018

Proposed Changes

Uh oh!

google-prow-robot commented Jul 10, 2018

Uh oh!

steuhs commented Jul 10, 2018

Uh oh!

steuhs commented Jul 10, 2018

Uh oh!

steuhs commented Jul 10, 2018

Uh oh!

steuhs commented Jul 10, 2018

Uh oh!

steuhs commented Jul 10, 2018

Uh oh!

steuhs commented Jul 10, 2018

Uh oh!

steuhs commented Jul 11, 2018

Uh oh!

steuhs commented Jul 11, 2018

Uh oh!

steuhs commented Jul 11, 2018

Uh oh!

steuhs commented Jul 11, 2018

Uh oh!

steuhs commented Jul 11, 2018

Uh oh!

steuhs commented Jul 11, 2018

Uh oh!

steuhs commented Jul 11, 2018

Uh oh!

steuhs commented Jul 11, 2018

Uh oh!

steuhs commented Jul 11, 2018

Uh oh!

akyyy commented Jul 12, 2018

Uh oh!

steuhs commented Jul 12, 2018

Uh oh!

steuhs commented Jul 12, 2018

Uh oh!

mattmoor commented Jul 12, 2018

Uh oh!

steuhs commented Jul 13, 2018

Uh oh!

steuhs commented Jul 13, 2018

Uh oh!

knative-metrics-robot commented Jul 13, 2018

Uh oh!

steuhs commented Jul 13, 2018

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!

Choose a reason for hiding this comment

Uh oh!