Repository: knative/pkg
Pull request #1834 (2020-10-22 23:11:02)

Some cleanups arounds the metrics code
- use switch
- add new method that won't trigger lint, with a deprecation notice when deps are updated
- other minor things
- string(nil) actually works, so no need for an extra if

/assign @yanweiguo @evankanderson 

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

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1834#" 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/1834?src=pr&el=h1) Report
> Merging [#1834](https://codecov.io/gh/knative/pkg/pull/1834?src=pr&el=desc) into [master](https://codecov.io/gh/knative/pkg/commit/8139298650a43a7daf68b97bb70404373c9e216a?el=desc) will **increase** coverage by `0.00%`.
> The diff coverage is `77.77%`.

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

```diff
@@           Coverage Diff           @@
##           master    #1834   +/-   ##
=======================================
  Coverage   68.17%   68.17%           
=======================================
  Files         222      222           
  Lines        9435     9434    -1     
=======================================
  Hits         6432     6432           
- Misses       2680     2681    +1     
+ Partials      323      321    -2     
```


| [Impacted Files](https://codecov.io/gh/knative/pkg/pull/1834?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [metrics/config.go](https://codecov.io/gh/knative/pkg/pull/1834/diff?src=pr&el=tree#diff-bWV0cmljcy9jb25maWcuZ28=) | `80.48% <77.77%> (-0.97%)` | :arrow_down: |
| [test/gcs/mock/mock.go](https://codecov.io/gh/knative/pkg/pull/1834/diff?src=pr&el=tree#diff-dGVzdC9nY3MvbW9jay9tb2NrLmdv) | `91.39% <0.00%> (+1.07%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/pkg/pull/1834?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/1834?src=pr&el=footer). Last update [8139298...f5d4b52](https://codecov.io/gh/knative/pkg/pull/1834?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

yanweiguo: /lgtm
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 | f5d4b5281f859fcb3ae3042ed03d63f5d821b98c | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_pkg/1834/pull-knative-pkg-unit-tests/1319416150194393088) | `/test pull-knative-pkg-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/1834). [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 -->
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1834#pullrequestreview-515203378" title="LGTM">tcnghia</a>*, *<a href="https://github.com/knative/pkg/pull/1834#" 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)~~ [tcnghia,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":[]} -->
tcnghia: 
/test pull-knative-pkg-unit-tests

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

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/1834). [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 -->
knative-prow-robot: @vagababov: you cannot LGTM your own PR.

<details>

In response to [this](https://github.com/knative/pkg/pull/1834#issuecomment-714846349):

>/lgtm


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.
</details>
vagababov: /lgtm
vagababov: /retest
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1834#pullrequestreview-515217458" title="Approved">evankanderson</a>*, *<a href="https://github.com/knative/pkg/pull/1834#pullrequestreview-515203378" title="LGTM">tcnghia</a>*, *<a href="https://github.com/knative/pkg/pull/1834#" 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)~~ [evankanderson,tcnghia,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":[]} -->
vagababov: Sorry, watching debates, pressing random buttons.

Patch:
diff --git a/metrics/config.go b/metrics/config.go
index 8214bde40..1bdf06607 100644
--- a/metrics/config.go
+++ b/metrics/config.go
@@ -200,15 +200,16 @@ func createMetricsConfig(ctx context.Context, ops ExporterOptions) (*metricsConf
 	if backendFromConfig, ok := m[BackendDestinationKey]; ok {
 		backend = backendFromConfig
 	}
-	lb := metricsBackend(strings.ToLower(backend))
-	switch lb {
+
+	switch lb := metricsBackend(strings.ToLower(backend)); lb {
 	case stackdriver, prometheus, openCensus:
 		mc.backendDestination = lb
 	default:
 		return nil, fmt.Errorf("unsupported metrics backend value %q", backend)
 	}
 
-	if mc.backendDestination == openCensus {
+	switch mc.backendDestination {
+	case openCensus:
 		mc.collectorAddress = ops.ConfigMap[collectorAddressKey]
 		if isSecure := ops.ConfigMap[collectorSecureKey]; isSecure != "" {
 			var err error
@@ -223,9 +224,7 @@ func createMetricsConfig(ctx context.Context, ops ExporterOptions) (*metricsConf
 				}
 			}
 		}
-	}
-
-	if mc.backendDestination == prometheus {
+	case prometheus:
 		pp := ops.PrometheusPort
 		if pp == 0 {
 			var err error
@@ -241,12 +240,10 @@ func createMetricsConfig(ctx context.Context, ops ExporterOptions) (*metricsConf
 		}
 
 		mc.prometheusPort = pp
-	}
-
-	// If stackdriverClientConfig is not provided for stackdriver backend destination, OpenCensus will try to
-	// use the application default credentials. If that is not available, Opencensus would fail to create the
-	// metrics exporter.
-	if mc.backendDestination == stackdriver {
+	case stackdriver:
+		// If stackdriverClientConfig is not provided for stackdriver backend destination, OpenCensus will try to
+		// use the application default credentials. If that is not available, Opencensus would fail to create the
+		// metrics exporter.
 		scc := NewStackdriverClientConfigFromMap(m)
 		mc.stackdriverClientConfig = *scc
 		mc.isStackdriverBackend = true
@@ -285,7 +282,7 @@ func createMetricsConfig(ctx context.Context, ops ExporterOptions) (*metricsConf
 	// For Prometheus, we will use a lower value since the exporter doesn't
 	// push anything but just responds to pull requests, and shorter durations
 	// do not really hurt the performance and we rely on the scraping configuration.
-	if repStr, ok := m[reportingPeriodKey]; ok && repStr != "" {
+	if repStr := m[reportingPeriodKey]; repStr != "" {
 		repInt, err := strconv.Atoi(repStr)
 		if err != nil {
 			return nil, fmt.Errorf("invalid %s value %q", reportingPeriodKey, repStr)
@@ -346,7 +343,12 @@ func prometheusPort() (int, error) {
 
 // JsonToMetricsOptions converts a json string of a
 // ExporterOptions. Returns a non-nil ExporterOptions always.
-func JsonToMetricsOptions(jsonOpts string) (*ExporterOptions, error) { //nolint // No rename due to backwards incompatibility.
+// TODO(vagababov): remove after updating deps.
+func JsonToMetricsOptions(jsonOpts string) (*ExporterOptions, error) { //nolint // No rename due to backwards incompatibility. {
+	return JSONToMetricsOptions(jsonOpts)
+}
+
+func JSONToMetricsOptions(jsonOpts string) (*ExporterOptions, error) {
 	var opts ExporterOptions
 	if jsonOpts == "" {
 		return nil, errors.New("json options string is empty")
@@ -360,15 +362,16 @@ func JsonToMetricsOptions(jsonOpts string) (*ExporterOptions, error) { //nolint
 }
 
 // MetricsOptionsToJson converts a ExporterOptions to a json string.
+// TODO(vagababov): remove after updating deps.
 func MetricsOptionsToJson(opts *ExporterOptions) (string, error) { //nolint // No rename due to backwards incompatibility.
+	return MetricsOptionsToJSON(opts)
+}
+
+func MetricsOptionsToJSON(opts *ExporterOptions) (string, error) {
 	if opts == nil {
 		return "", nil
 	}
 
 	jsonOpts, err := json.Marshal(opts)
-	if err != nil {
-		return "", err
-	}
-
-	return string(jsonOpts), nil
+	return string(jsonOpts), err
 }


Test patch:
diff --git a/metrics/config_test.go b/metrics/config_test.go
index a6adf3bb7..5e8f881e6 100644
--- a/metrics/config_test.go
+++ b/metrics/config_test.go
@@ -81,7 +81,6 @@ var (
 			ConfigMap: map[string]string{
 				BackendDestinationKey: string(prometheus),
 			},
-			Domain:    "",
 			Component: testComponent,
 		},
 		expectedErr: "metrics domain cannot be empty",
@@ -91,8 +90,7 @@ var (
 			ConfigMap: map[string]string{
 				BackendDestinationKey: string(openCensus),
 			},
-			Domain:    servingDomain,
-			Component: "",
+			Domain: servingDomain,
 		},
 		expectedErr: "metrics component name cannot be empty",
 	}, {
@@ -538,7 +536,7 @@ func TestGetMetricsConfig(t *testing.T) {
 		t.Run(test.name, func(t *testing.T) {
 			_, err := createMetricsConfig(ctx, test.ops)
 			if err == nil || err.Error() != test.expectedErr {
-				t.Errorf("Wanted err: %v, got: %v", test.expectedErr, err)
+				t.Errorf("Err = %v, want: %v", err, test.expectedErr)
 			}
 		})
 	}
@@ -923,7 +921,7 @@ func TestMetricsOptions(t *testing.T) {
 	}
 	for n, tc := range testCases {
 		t.Run(n, func(t *testing.T) {
-			jsonOpts, err := MetricsOptionsToJson(tc.opts)
+			jsonOpts, err := MetricsOptionsToJSON(tc.opts)
 			if err != nil {
 				t.Error("error while converting metrics config to json:", err)
 			}
@@ -939,7 +937,7 @@ func TestMetricsOptions(t *testing.T) {
 			// Test to options.
 			{
 				want := tc.opts
-				got, gotErr := JsonToMetricsOptions(jsonOpts)
+				got, gotErr := JSONToMetricsOptions(jsonOpts)
 
 				if gotErr != nil {
 					if diff := cmp.Diff(tc.wantErr, gotErr.Error()); diff != "" {