Report RPS for autoscaler metrics #5238

taragu · 2019-08-21T14:52:59Z

/lint

Part of #5228

Proposed Changes

Add reporting for stable and panic RPS metrics

Release Note

NONE

/cc @yanweiguo

knative-prow-robot

@taragu: 0 warnings.

In response to this:

/lint

Part of #5228

Proposed Changes

Add reporting for stable and panic RPS metrics

Release Note
NONE
/cc @yanweiguo

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 · 2019-08-21T14:53:14Z

Hi @taragu. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

config/monitoring/metrics/prometheus/100-grafana-dash-knative-scaling.yaml

pkg/autoscaler/stats_reporter.go

taragu · 2019-08-21T15:07:45Z

/hold
because knative/pkg#589 should be merged before this one

yanweiguo

Please revert knative/pkg#589.

yanweiguo · 2019-08-21T17:59:22Z

pkg/autoscaler/stats_reporter.go

+	panicRPSM = stats.Float64(
+		"panic_requests_per_second",
+		"Average requests-per-second per observed pod over the panic window",
+		stats.UnitDimensionless)


Could we add targetRPS as well?

yanweiguo · 2019-08-21T18:01:20Z

pkg/autoscaler/stats_reporter.go

@@ -53,6 +53,14 @@ var (
 		"panic_request_concurrency",
 		"Average of requests count per observed pod over the panic window",
 		stats.UnitDimensionless)
+	stableRPSM = stats.Float64(


You need to register these metrics in func register.

taragu · 2019-08-21T19:02:26Z

/hold cancel

yanweiguo · 2019-08-22T01:16:39Z

pkg/autoscaler/multiscaler.go

@@ -48,6 +48,8 @@ type DeciderSpec struct {
 	// The value of scaling metric per pod that we target to maintain.
 	// TargetValue <= TotalValue.
 	TargetValue float64
+	// The value of requests-per-second per pod that we target to maintain.
+	TargetRPS float64


I don't think we need this. TargetValue is intended to be used for any metric.

Ahh got it!

yanweiguo · 2019-08-22T01:21:57Z

config/monitoring/metrics/prometheus/100-grafana-dash-knative-scaling.yaml

+                  "expr": "sum(autoscaler_target_requests_per_second{namespace_name=\"$namespace\", configuration_name=\"$configuration\", revision_name=\"$revision\"})",
+                  "format": "time_series",
+                  "interval": "1s",
+                  "intervalFactor": 1,


I think we need to change the title for this dashboard as well or create a new one for RPS.

markusthoemmes · 2019-08-22T06:52:05Z

pkg/reconciler/autoscaling/kpa/resources/decider.go

@@ -65,6 +65,7 @@ func MakeDecider(ctx context.Context, pa *v1alpha1.PodAutoscaler, config *autosc
 			MaxScaleUpRate:      config.MaxScaleUpRate,
 			ScalingMetric:       pa.Metric(),
 			TargetValue:         target,
+			TargetRPS:           config.RPSTargetDefault,


As @yanweiguo mentioned, this shouldn't be needed.

Still here.

yanweiguo · 2019-08-22T16:56:36Z

Hi @taragu, thanks for this PR.

Could you test it end to end to get a screenshot of the Grafana dashboard? I'm not confident with what it will look like when we put concurrency and RPS into one table.

taragu · 2019-08-26T14:23:56Z

@yanweiguo I've updated the PR to put the RPS metrics in a separate graph. Would you please review the PR again? This is what the dashboard looks like

vagababov · 2019-08-26T14:34:03Z

/ok-to-test

knative-test-reporter-robot · 2019-08-26T14:57:04Z

The following jobs failed due to test flakiness:

Test name	Triggers	Retries
pull-knative-serving-integration-tests		0/3

Failed non-flaky tests preventing automatic retry of pull-knative-serving-integration-tests:

test/e2e.TestDestroyPodTimely

knative-metrics-robot · 2019-08-26T15:20:03Z

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/autoscaler.go	96.1%	96.2%	0.1
pkg/autoscaler/stats_reporter.go	87.2%	88.1%	0.9

yanweiguo · 2019-08-26T17:43:35Z

/lgtm

taragu · 2019-08-26T18:02:45Z

/assign @mdemirhan

yanweiguo · 2019-08-26T18:05:38Z

/approve
For dashboard part.

vagababov

/approve

knative-prow-robot · 2019-08-26T18:24:30Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~config/monitoring/OWNERS~~ [yanweiguo]
~~pkg/autoscaler/OWNERS~~ [vagababov]

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

knative-prow-robot requested a review from yanweiguo August 21, 2019 14:53

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

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/autoscale area/monitoring labels Aug 21, 2019

knative-prow-robot reviewed Aug 21, 2019

View reviewed changes

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 21, 2019

markusthoemmes reviewed Aug 21, 2019

View reviewed changes

config/monitoring/metrics/prometheus/100-grafana-dash-knative-scaling.yaml Outdated Show resolved Hide resolved

pkg/autoscaler/stats_reporter.go Outdated Show resolved Hide resolved

pkg/autoscaler/stats_reporter.go Outdated Show resolved Hide resolved

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 21, 2019

yanweiguo reviewed Aug 21, 2019

View reviewed changes

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/API API objects and controllers and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 21, 2019

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 21, 2019

taragu force-pushed the autoscaler-rps-metrics branch from e3cf049 to ba70dd1 Compare August 21, 2019 19:03

yanweiguo reviewed Aug 22, 2019

View reviewed changes

markusthoemmes reviewed Aug 22, 2019

View reviewed changes

taragu force-pushed the autoscaler-rps-metrics branch 2 times, most recently from 5d3d452 to 0f4a00c Compare August 26, 2019 14:21

taragu force-pushed the autoscaler-rps-metrics branch from 0f4a00c to ad886da Compare August 26, 2019 14:29

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 26, 2019

Report RPS for autoscaler metrics

aead62f

taragu force-pushed the autoscaler-rps-metrics branch from ad886da to aead62f Compare August 26, 2019 15:14

knative-prow-robot assigned yanweiguo Aug 26, 2019

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

knative-prow-robot assigned mdemirhan Aug 26, 2019

vagababov reviewed Aug 26, 2019

View reviewed changes

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

knative-prow-robot merged commit 6591f41 into knative:master Aug 26, 2019

yanweiguo mentioned this pull request Aug 28, 2019

Add RPS support for HPA #5312

Merged

taragu deleted the autoscaler-rps-metrics branch November 12, 2019 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report RPS for autoscaler metrics #5238

Report RPS for autoscaler metrics #5238

taragu commented Aug 21, 2019

knative-prow-robot left a comment

knative-prow-robot commented Aug 21, 2019

taragu commented Aug 21, 2019

yanweiguo left a comment

yanweiguo Aug 21, 2019

yanweiguo Aug 21, 2019

taragu commented Aug 21, 2019

yanweiguo Aug 22, 2019

taragu Aug 22, 2019

yanweiguo Aug 22, 2019

markusthoemmes Aug 22, 2019

yanweiguo Aug 22, 2019

yanweiguo commented Aug 22, 2019

taragu commented Aug 26, 2019 •

edited

Loading

vagababov commented Aug 26, 2019

knative-test-reporter-robot commented Aug 26, 2019

knative-metrics-robot commented Aug 26, 2019

yanweiguo commented Aug 26, 2019

taragu commented Aug 26, 2019

yanweiguo commented Aug 26, 2019

vagababov left a comment

knative-prow-robot commented Aug 26, 2019

Report RPS for autoscaler metrics #5238

Report RPS for autoscaler metrics #5238

Conversation

taragu commented Aug 21, 2019

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

knative-prow-robot commented Aug 21, 2019

taragu commented Aug 21, 2019

yanweiguo left a comment

Choose a reason for hiding this comment

yanweiguo Aug 21, 2019

Choose a reason for hiding this comment

yanweiguo Aug 21, 2019

Choose a reason for hiding this comment

taragu commented Aug 21, 2019

yanweiguo Aug 22, 2019

Choose a reason for hiding this comment

taragu Aug 22, 2019

Choose a reason for hiding this comment

yanweiguo Aug 22, 2019

Choose a reason for hiding this comment

markusthoemmes Aug 22, 2019

Choose a reason for hiding this comment

yanweiguo Aug 22, 2019

Choose a reason for hiding this comment

yanweiguo commented Aug 22, 2019

taragu commented Aug 26, 2019 • edited Loading

vagababov commented Aug 26, 2019

knative-test-reporter-robot commented Aug 26, 2019

knative-metrics-robot commented Aug 26, 2019

yanweiguo commented Aug 26, 2019

taragu commented Aug 26, 2019

yanweiguo commented Aug 26, 2019

vagababov left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Aug 26, 2019

taragu commented Aug 26, 2019 •

edited

Loading