Repository: knative/pkg
Pull request #1648 (2020-08-24 20:51:17)

Fix the the request logging boolean toggle
Since we have cut the 0.17, we can go for the proper toggle setup:

- if request logging is on — template must be present and the logging will be on
- if the request of logging is off — template can be present, but will not affect whether the request logs will be collected

Really
Fixes https://github.com/knative/serving/issues/8644

/assign @yanweiguo @julz 

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

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1648#" 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-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-pkg-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[metrics/config_observability.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1648/pull-knative-pkg-go-coverage/1298000168771129345/artifacts/line-cov.html#file0) | 64.7% | 64.3% | -0.4

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 | 1b33931a52977cf3aac970ce0c589807be4800c5 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_pkg/1648/pull-knative-pkg-unit-tests/1298000168771129346) | `/test pull-knative-pkg-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/1648). [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: /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 | 1b33931a52977cf3aac970ce0c589807be4800c5 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_pkg/1648/pull-knative-pkg-unit-tests/1298007090001874944) | `/test pull-knative-pkg-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/1648). [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: (ノಠ益ಠ)ノ彡┻━┻

Unrelated
/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 | 1b33931a52977cf3aac970ce0c589807be4800c5 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_pkg/1648/pull-knative-pkg-unit-tests/1298011745708675072) | `/test pull-knative-pkg-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/1648). [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: /retest
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-pkg-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[metrics/config_observability.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1648/pull-knative-pkg-go-coverage/1298038421914128387/artifacts/line-cov.html#file0) | 87.5% | 92.3% | 4.8

knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-pkg-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[metrics/config_observability.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1648/pull-knative-pkg-go-coverage/1298057547613212672/artifacts/line-cov.html#file0) | 87.5% | 92.3% | 4.8


Patch:
diff --git a/metrics/config_observability.go b/metrics/config_observability.go
index e88aa0e48..12df827de 100644
--- a/metrics/config_observability.go
+++ b/metrics/config_observability.go
@@ -17,8 +17,8 @@ limitations under the License.
 package metrics
 
 import (
+	"fmt"
 	"os"
-	"strings"
 	texttemplate "text/template"
 
 	corev1 "k8s.io/api/core/v1"
@@ -28,12 +28,16 @@ import (
 const (
 	// The following is used to set the default log url template
 	DefaultLogURLTemplate = "http://localhost:8001/api/v1/namespaces/knative-monitoring/services/kibana-logging/proxy/app/kibana#/discover?_a=(query:(match:(kubernetes.labels.knative-dev%2FrevisionUID:(query:'${REVISION_UID}',type:phrase))))"
+	// DefaultRequestLogTemplate is the default format for emitting request logs.
+	DefaultRequestLogTemplate = `{"httpRequest": {"requestMethod": "{{.Request.Method}}", "requestUrl": "{{js .Request.RequestURI}}", "requestSize": "{{.Request.ContentLength}}", "status": {{.Response.Code}}, "responseSize": "{{.Response.Size}}", "userAgent": "{{js .Request.UserAgent}}", "remoteIp": "{{js .Request.RemoteAddr}}", "serverIp": "{{.Revision.PodIP}}", "referer": "{{js .Request.Referer}}", "latency": "{{.Response.Latency}}s", "protocol": "{{.Request.Proto}}"}, "traceId": "{{index .Request.Header "X-B3-Traceid"}}"}`
 
 	// The following is used to set the default metrics backend
 	defaultRequestMetricsBackend = "prometheus"
 
 	// The env var name for config-observability
-	configMapNameEnv = "CONFIG_OBSERVABILITY_NAME"
+	configMapNameEnv  = "CONFIG_OBSERVABILITY_NAME"
+	reqLogTemplateKey = "logging.request-log-template"
+	enableReqLogKey   = "logging.enable-request-log"
 )
 
 // ObservabilityConfig contains the configuration defined in the observability ConfigMap.
@@ -72,6 +76,7 @@ type ObservabilityConfig struct {
 func defaultConfig() *ObservabilityConfig {
 	return &ObservabilityConfig{
 		LoggingURLTemplate:    DefaultLogURLTemplate,
+		RequestLogTemplate:    DefaultRequestLogTemplate,
 		RequestMetricsBackend: defaultRequestMetricsBackend,
 	}
 }
@@ -83,7 +88,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("logging.request-log-template", &oc.RequestLogTemplate),
+		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),
@@ -92,15 +98,8 @@ func NewObservabilityConfigFromConfigMap(configMap *corev1.ConfigMap) (*Observab
 		return nil, err
 	}
 
-	if raw, ok := configMap.Data["logging.enable-request-log"]; ok {
-		if strings.EqualFold(raw, "true") && oc.RequestLogTemplate != "" {
-			oc.EnableRequestLog = true
-		}
-	} else if oc.RequestLogTemplate != "" {
-		// TODO: remove this after 0.17 cuts, this is meant only for smooth transition to the new flag.
-		// Once 0.17 cuts we should set a proper default value and users will need to set the flag explicitly
-		// to enable request logging.
-		oc.EnableRequestLog = true
+	if oc.RequestLogTemplate == "" && oc.EnableRequestLog {
+		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 9a11a6cb7..9e752bd5a 100644
--- a/metrics/config_observability_test.go
+++ b/metrics/config_observability_test.go
@@ -33,13 +33,12 @@ func TestObservabilityConfiguration(t *testing.T) {
 		wantErr    bool
 		wantConfig *ObservabilityConfig
 	}{{
-		name:    "observability configuration with all inputs",
-		wantErr: false,
+		name: "observability configuration with all inputs",
 		wantConfig: &ObservabilityConfig{
 			EnableProbeRequestLog:  true,
 			EnableProfiling:        true,
-			EnableRequestLog:       false,
 			EnableVarLogCollection: true,
+			EnableRequestLog:       true,
 			LoggingURLTemplate:     "https://logging.io",
 			RequestLogTemplate:     `{"requestMethod": "{{.Request.Method}}"}`,
 			RequestMetricsBackend:  "stackdriver",
@@ -49,38 +48,65 @@ func TestObservabilityConfiguration(t *testing.T) {
 			"logging.enable-var-log-collection":           "true",
 			"logging.request-log-template":                `{"requestMethod": "{{.Request.Method}}"}`,
 			"logging.revision-url-template":               "https://logging.io",
-			"logging.enable-request-log":                  "false",
+			"logging.enable-request-log":                  "true",
 			"metrics.request-metrics-backend-destination": "stackdriver",
 			"profiling.enable":                            "true",
 		},
 	}, {
 		name:       "observability config with no map",
-		wantErr:    false,
 		wantConfig: defaultConfig(),
 	}, {
-		name:       "invalid request log template",
-		wantErr:    true,
-		wantConfig: nil,
+		name:    "invalid request log template",
+		wantErr: true,
 		data: map[string]string{
 			"logging.request-log-template": `{{ something }}`,
 		},
 	}, {
-		name:    "observability configuration with request log set and with template not set",
-		wantErr: false,
+		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",
+			"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",
+		},
+	}, {
+		name: "observability configuration with request log and template not set",
 		wantConfig: &ObservabilityConfig{
 			EnableProbeRequestLog:  true,
 			EnableProfiling:        true,
-			EnableRequestLog:       false,
 			EnableVarLogCollection: true,
 			LoggingURLTemplate:     "https://logging.io",
-			RequestLogTemplate:     "",
 			RequestMetricsBackend:  "stackdriver",
 		},
 		data: map[string]string{
 			"logging.enable-probe-request-log":            "true",
+			"logging.enable-request-log":                  "false",
 			"logging.enable-var-log-collection":           "true",
+			"logging.request-log-template":                "",
+			"logging.revision-url-template":               "https://logging.io",
+			"metrics.request-metrics-backend-destination": "stackdriver",
+			"profiling.enable":                            "true",
+		},
+	}, {
+		name:    "observability configuration with request log set and template not set",
+		wantErr: true,
+		data: map[string]string{
+			"logging.enable-probe-request-log":            "true",
+			"logging.enable-request-log":                  "true",
+			"logging.enable-var-log-collection":           "true",
+			"logging.request-log-template":                "",
 			"logging.revision-url-template":               "https://logging.io",
-			"logging.enable-request-log":                  "false",
 			"metrics.request-metrics-backend-destination": "stackdriver",
 			"profiling.enable":                            "true",
 		},
@@ -90,7 +116,6 @@ func TestObservabilityConfiguration(t *testing.T) {
 		wantConfig: &ObservabilityConfig{
 			EnableProbeRequestLog:  true,
 			EnableProfiling:        true,
-			EnableRequestLog:       true,
 			EnableVarLogCollection: true,
 			LoggingURLTemplate:     "https://logging.io",
 			RequestLogTemplate:     `{"requestMethod": "{{.Request.Method}}"}`,
@@ -105,15 +130,10 @@ func TestObservabilityConfiguration(t *testing.T) {
 			"profiling.enable":                            "true",
 		},
 	}, {
-		name:    "observability configuration with collector address",
-		wantErr: false,
+		name: "observability configuration with collector address",
 		wantConfig: &ObservabilityConfig{
-			EnableProbeRequestLog:   false,
-			EnableProfiling:         false,
-			EnableRequestLog:        false,
-			EnableVarLogCollection:  false,
 			LoggingURLTemplate:      DefaultLogURLTemplate,
-			RequestLogTemplate:      "",
+			RequestLogTemplate:      DefaultRequestLogTemplate,
 			RequestMetricsBackend:   "opencensus",
 			MetricsCollectorAddress: "otel:55678",
 		},