Repository: knative/pkg
Pull request #1898 (2020-11-06 18:59:04)

Add a flag to disable QP profiling.
Turning profiling on or off toggles full rolling restart
of knative pods. And if profiling QP is not of interest, this
might be quite undesireable. To permit that.

For https://github.com/knative/serving/issues/10051

knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1898#" title="Author self-approved">vagababov</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[metrics/OWNERS](https://github.com/knative/pkg/blob/master/metrics/OWNERS)~~ [vagababov]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/pkg/pull/1898?src=pr&el=h1) Report
> Merging [#1898](https://codecov.io/gh/knative/pkg/pull/1898?src=pr&el=desc) (294c970) into [master](https://codecov.io/gh/knative/pkg/commit/8c31b27dfba8ca8223c311b0ec458d93c1373d46?el=desc) (8c31b27) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/knative/pkg/pull/1898/graphs/tree.svg?width=650&height=150&src=pr&token=rlqpDnfxbO)](https://codecov.io/gh/knative/pkg/pull/1898?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master    #1898   +/-   ##
=======================================
  Coverage   69.42%   69.42%           
=======================================
  Files         207      207           
  Lines        8738     8740    +2     
=======================================
+ Hits         6066     6068    +2     
  Misses       2402     2402           
  Partials      270      270           
```


| [Impacted Files](https://codecov.io/gh/knative/pkg/pull/1898?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [metrics/config\_observability.go](https://codecov.io/gh/knative/pkg/pull/1898/diff?src=pr&el=tree#diff-bWV0cmljcy9jb25maWdfb2JzZXJ2YWJpbGl0eS5nbw==) | `92.59% <100.00%> (+0.59%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/pkg/pull/1898?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative/pkg/pull/1898?src=pr&el=footer). Last update [8c31b27...6b41a11](https://codecov.io/gh/knative/pkg/pull/1898?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

vagababov: /assign @yanweiguo @julz 
yanweiguo: /lgtm

Patch:
diff --git a/metrics/config_observability.go b/metrics/config_observability.go
index ae622c928..18df1e917 100644
--- a/metrics/config_observability.go
+++ b/metrics/config_observability.go
@@ -73,6 +73,12 @@ type ObservabilityConfig struct {
 	// the pods via an HTTP server in the format expected by the pprof visualization tool.
 	EnableProfiling bool
 
+	// DisableQPProfiling indicates whether EnableProfiling flag applies to the QueueProxy processes.
+	// If false (default) — enabling or disabling profiling causes a global rolling restart of the
+	// Knative service pods, which might not be desirable (especially if there is no need to profile
+	// QP processes).
+	DisableQPProfiling bool
+
 	// EnableRequestLog enables activator/queue-proxy to write request logs.
 	EnableRequestLog bool
 
@@ -101,6 +107,7 @@ func NewObservabilityConfigFromConfigMap(configMap *corev1.ConfigMap) (*Observab
 		cm.AsBool(EnableProbeReqLogKey, &oc.EnableProbeRequestLog),
 		cm.AsString("metrics.request-metrics-backend-destination", &oc.RequestMetricsBackend),
 		cm.AsBool("profiling.enable", &oc.EnableProfiling),
+		cm.AsBool("profiling.disable-qp", &oc.DisableQPProfiling),
 		cm.AsString("metrics.opencensus-address", &oc.MetricsCollectorAddress),
 	); err != nil {
 		return nil, err


Test patch:
diff --git a/metrics/config_observability_test.go b/metrics/config_observability_test.go
index 64dac6891..50e48c248 100644
--- a/metrics/config_observability_test.go
+++ b/metrics/config_observability_test.go
@@ -37,6 +37,7 @@ func TestObservabilityConfiguration(t *testing.T) {
 		wantConfig: &ObservabilityConfig{
 			EnableProbeRequestLog:  true,
 			EnableProfiling:        true,
+			DisableQPProfiling:     true,
 			EnableVarLogCollection: true,
 			EnableRequestLog:       true,
 			LoggingURLTemplate:     "https://logging.io",
@@ -51,6 +52,7 @@ func TestObservabilityConfiguration(t *testing.T) {
 			EnableReqLogKey:                               "true",
 			"metrics.request-metrics-backend-destination": "stackdriver",
 			"profiling.enable":                            "true",
+			"profiling.disable-qp":                        "true",
 		},
 	}, {
 		name:       "observability config with no map",
@@ -64,78 +66,61 @@ func TestObservabilityConfiguration(t *testing.T) {
 	}, {
 		name: "observability configuration with request log set and template default",
 		data: map[string]string{
-			EnableProbeReqLogKey:                          "true",
-			EnableReqLogKey:                               "true",
-			"logging.enable-var-log-collection":           "true",
-			"logging.revision-url-template":               "https://logging.io",
-			"metrics.request-metrics-backend-destination": "stackdriver",
-			"profiling.enable":                            "true",
-		},
-		wantConfig: &ObservabilityConfig{
-			EnableProbeRequestLog:  true,
-			EnableProfiling:        true,
-			EnableRequestLog:       true,
-			EnableVarLogCollection: true,
-			LoggingURLTemplate:     "https://logging.io",
-			RequestLogTemplate:     DefaultRequestLogTemplate,
-			RequestMetricsBackend:  "stackdriver",
+			EnableProbeReqLogKey:            "true",
+			EnableReqLogKey:                 "true",
+			"logging.revision-url-template": "https://logging.io",
 		},
+		wantConfig: func() *ObservabilityConfig {
+			oc := defaultConfig()
+			oc.EnableProbeRequestLog = true
+			oc.EnableRequestLog = true
+			oc.LoggingURLTemplate = "https://logging.io"
+			return oc
+		}(),
 	}, {
 		name: "observability configuration with request log and template not set",
-		wantConfig: &ObservabilityConfig{
-			EnableProbeRequestLog:  true,
-			EnableProfiling:        true,
-			EnableVarLogCollection: true,
-			LoggingURLTemplate:     "https://logging.io",
-			RequestMetricsBackend:  "stackdriver",
-		},
+		wantConfig: func() *ObservabilityConfig {
+			oc := defaultConfig()
+			oc.RequestLogTemplate = ""
+			oc.EnableProbeRequestLog = true
+			return oc
+		}(),
 		data: map[string]string{
-			EnableProbeReqLogKey:                          "true",
-			EnableReqLogKey:                               "false",
-			"logging.enable-var-log-collection":           "true",
-			ReqLogTemplateKey:                             "",
-			"logging.revision-url-template":               "https://logging.io",
-			"metrics.request-metrics-backend-destination": "stackdriver",
-			"profiling.enable":                            "true",
+			EnableProbeReqLogKey: "true",
+			EnableReqLogKey:      "false", // Explicit default.
+			ReqLogTemplateKey:    "",
 		},
 	}, {
 		name:    "observability configuration with request log set and template not set",
 		wantErr: true,
 		data: map[string]string{
-			EnableProbeReqLogKey:                          "true",
-			EnableReqLogKey:                               "true",
-			"logging.enable-var-log-collection":           "true",
-			ReqLogTemplateKey:                             "",
-			"logging.revision-url-template":               "https://logging.io",
-			"metrics.request-metrics-backend-destination": "stackdriver",
-			"profiling.enable":                            "true",
+			EnableProbeReqLogKey:                "true",
+			EnableReqLogKey:                     "true",
+			"logging.enable-var-log-collection": "true",
+			ReqLogTemplateKey:                   "",
 		},
 	}, {
 		name: "observability configuration with request log not set and with template set",
-		wantConfig: &ObservabilityConfig{
-			EnableProbeRequestLog:  true,
-			EnableProfiling:        true,
-			EnableVarLogCollection: true,
-			LoggingURLTemplate:     "https://logging.io",
-			RequestLogTemplate:     `{"requestMethod": "{{.Request.Method}}"}`,
-			RequestMetricsBackend:  "stackdriver",
-		},
+		wantConfig: func() *ObservabilityConfig {
+			oc := defaultConfig()
+			oc.EnableProbeRequestLog = true
+			oc.EnableVarLogCollection = true
+			oc.RequestLogTemplate = `{"requestMethod": "{{.Request.Method}}"}`
+			return oc
+		}(),
 		data: map[string]string{
-			EnableProbeReqLogKey:                          "true",
-			"logging.enable-var-log-collection":           "true",
-			ReqLogTemplateKey:                             `{"requestMethod": "{{.Request.Method}}"}`,
-			"logging.revision-url-template":               "https://logging.io",
-			"metrics.request-metrics-backend-destination": "stackdriver",
-			"profiling.enable":                            "true",
+			EnableProbeReqLogKey:                "true",
+			"logging.enable-var-log-collection": "true",
+			ReqLogTemplateKey:                   `{"requestMethod": "{{.Request.Method}}"}`,
 		},
 	}, {
 		name: "observability configuration with collector address",
-		wantConfig: &ObservabilityConfig{
-			LoggingURLTemplate:      DefaultLogURLTemplate,
-			RequestLogTemplate:      DefaultRequestLogTemplate,
-			RequestMetricsBackend:   "opencensus",
-			MetricsCollectorAddress: "otel:55678",
-		},
+		wantConfig: func() *ObservabilityConfig {
+			oc := defaultConfig()
+			oc.RequestMetricsBackend = "opencensus"
+			oc.MetricsCollectorAddress = "otel:55678"
+			return oc
+		}(),
 		data: map[string]string{
 			"metrics.request-metrics-backend-destination": "opencensus",
 			"metrics.opencensus-address":                  "otel:55678",