Repository: knative/pkg
Pull request #1690 (2020-09-08 22:51:32)

Use the constant in the unit tests that we already have
and make it public so it can be used in serving as well.

/assign mattmoor

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

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1690#" 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":[]} -->
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1690#pullrequestreview-484547966" title="Approved">mattmoor</a>*, *<a href="https://github.com/knative/pkg/pull/1690#" 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)~~ [mattmoor,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":[]} -->
knative-prow-robot: @vagababov: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-pkg-unit-tests | fc87a384d9ad81cff5ee64120b1c348708f3dfc1 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_pkg/1690/pull-knative-pkg-unit-tests/1303466318355238914) | `/test pull-knative-pkg-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/1690). [Your PR dashboard](https://gubernator.knative.dev/pr/vagababov).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
yanweiguo: /lgtm
/retest
knative-prow-robot: @vagababov: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-pkg-unit-tests | fc87a384d9ad81cff5ee64120b1c348708f3dfc1 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_pkg/1690/pull-knative-pkg-unit-tests/1303477391040974848) | `/test pull-knative-pkg-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/1690). [Your PR dashboard](https://gubernator.knative.dev/pr/vagababov).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
vagababov: Alright. Now it is informed watcher that flakes...
vagababov: /retest

Patch:
diff --git a/metrics/config_observability.go b/metrics/config_observability.go
index 12df827de..177b35cb1 100644
--- a/metrics/config_observability.go
+++ b/metrics/config_observability.go
@@ -35,9 +35,13 @@ const (
 	defaultRequestMetricsBackend = "prometheus"
 
 	// The env var name for config-observability
-	configMapNameEnv  = "CONFIG_OBSERVABILITY_NAME"
-	reqLogTemplateKey = "logging.request-log-template"
-	enableReqLogKey   = "logging.enable-request-log"
+	configMapNameEnv = "CONFIG_OBSERVABILITY_NAME"
+
+	// ReqLogTemplateKey is the CM key for the request log template.
+	ReqLogTemplateKey = "logging.request-log-template"
+
+	// EnableReqLogKey is the CM key to enable request log.
+	EnableReqLogKey = "logging.enable-request-log"
 )
 
 // ObservabilityConfig contains the configuration defined in the observability ConfigMap.
@@ -88,8 +92,8 @@ func NewObservabilityConfigFromConfigMap(configMap *corev1.ConfigMap) (*Observab
 	if err := cm.Parse(configMap.Data,
 		cm.AsBool("logging.enable-var-log-collection", &oc.EnableVarLogCollection),
 		cm.AsString("logging.revision-url-template", &oc.LoggingURLTemplate),
-		cm.AsString(reqLogTemplateKey, &oc.RequestLogTemplate),
-		cm.AsBool(enableReqLogKey, &oc.EnableRequestLog),
+		cm.AsString(ReqLogTemplateKey, &oc.RequestLogTemplate),
+		cm.AsBool(EnableReqLogKey, &oc.EnableRequestLog),
 		cm.AsBool("logging.enable-probe-request-log", &oc.EnableProbeRequestLog),
 		cm.AsString("metrics.request-metrics-backend-destination", &oc.RequestMetricsBackend),
 		cm.AsBool("profiling.enable", &oc.EnableProfiling),
@@ -99,7 +103,7 @@ func NewObservabilityConfigFromConfigMap(configMap *corev1.ConfigMap) (*Observab
 	}
 
 	if oc.RequestLogTemplate == "" && oc.EnableRequestLog {
-		return nil, fmt.Errorf("%q was set to true, but no %q was specified", enableReqLogKey, reqLogTemplateKey)
+		return nil, fmt.Errorf("%q was set to true, but no %q was specified", EnableReqLogKey, ReqLogTemplateKey)
 	}
 
 	if oc.RequestLogTemplate != "" {


Test patch:
diff --git a/metrics/config_observability_test.go b/metrics/config_observability_test.go
index 9e752bd5a..075db67b9 100644
--- a/metrics/config_observability_test.go
+++ b/metrics/config_observability_test.go
@@ -46,9 +46,9 @@ func TestObservabilityConfiguration(t *testing.T) {
 		data: map[string]string{
 			"logging.enable-probe-request-log":            "true",
 			"logging.enable-var-log-collection":           "true",
-			"logging.request-log-template":                `{"requestMethod": "{{.Request.Method}}"}`,
+			ReqLogTemplateKey:                             `{"requestMethod": "{{.Request.Method}}"}`,
 			"logging.revision-url-template":               "https://logging.io",
-			"logging.enable-request-log":                  "true",
+			EnableReqLogKey:                               "true",
 			"metrics.request-metrics-backend-destination": "stackdriver",
 			"profiling.enable":                            "true",
 		},
@@ -59,13 +59,13 @@ func TestObservabilityConfiguration(t *testing.T) {
 		name:    "invalid request log template",
 		wantErr: true,
 		data: map[string]string{
-			"logging.request-log-template": `{{ something }}`,
+			ReqLogTemplateKey: `{{ something }}`,
 		},
 	}, {
 		name: "observability configuration with request log set and template default",
 		data: map[string]string{
 			"logging.enable-probe-request-log":            "true",
-			"logging.enable-request-log":                  "true",
+			EnableReqLogKey:                               "true",
 			"logging.enable-var-log-collection":           "true",
 			"logging.revision-url-template":               "https://logging.io",
 			"metrics.request-metrics-backend-destination": "stackdriver",
@@ -91,9 +91,9 @@ func TestObservabilityConfiguration(t *testing.T) {
 		},
 		data: map[string]string{
 			"logging.enable-probe-request-log":            "true",
-			"logging.enable-request-log":                  "false",
+			EnableReqLogKey:                               "false",
 			"logging.enable-var-log-collection":           "true",
-			"logging.request-log-template":                "",
+			ReqLogTemplateKey:                             "",
 			"logging.revision-url-template":               "https://logging.io",
 			"metrics.request-metrics-backend-destination": "stackdriver",
 			"profiling.enable":                            "true",
@@ -103,16 +103,15 @@ func TestObservabilityConfiguration(t *testing.T) {
 		wantErr: true,
 		data: map[string]string{
 			"logging.enable-probe-request-log":            "true",
-			"logging.enable-request-log":                  "true",
+			EnableReqLogKey:                               "true",
 			"logging.enable-var-log-collection":           "true",
-			"logging.request-log-template":                "",
+			ReqLogTemplateKey:                             "",
 			"logging.revision-url-template":               "https://logging.io",
 			"metrics.request-metrics-backend-destination": "stackdriver",
 			"profiling.enable":                            "true",
 		},
 	}, {
-		name:    "observability configuration with request log not set and with template set",
-		wantErr: false,
+		name: "observability configuration with request log not set and with template set",
 		wantConfig: &ObservabilityConfig{
 			EnableProbeRequestLog:  true,
 			EnableProfiling:        true,
@@ -124,7 +123,7 @@ func TestObservabilityConfiguration(t *testing.T) {
 		data: map[string]string{
 			"logging.enable-probe-request-log":            "true",
 			"logging.enable-var-log-collection":           "true",
-			"logging.request-log-template":                `{"requestMethod": "{{.Request.Method}}"}`,
+			ReqLogTemplateKey:                             `{"requestMethod": "{{.Request.Method}}"}`,
 			"logging.revision-url-template":               "https://logging.io",
 			"metrics.request-metrics-backend-destination": "stackdriver",
 			"profiling.enable":                            "true",