Repository: knative/pkg
Pull request #1836 (2020-10-23 09:34:08)

Remove NamespacedName logging helper
As per title. This doesn't behave the way we expected it to. See https://github.com/knative/serving/pull/9923 for more info. This is only used in Serving now, so we can safely drop this after https://github.com/knative/serving/pull/9923 merges.

/assign @vagababov 

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

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1836#" title="Author self-approved">markusthoemmes</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:

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

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: @markusthoemmes: The following test **failed**, say `/retest` to rerun all failed tests:

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

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

<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 -->
markusthoemmes: /hold

Until the serving PR merges.
vagababov: /lgtm
markusthoemmes: /unhold

Patch:
diff --git a/logging/object_encoders.go b/logging/object_encoders.go
index 2b98fb5b6..ee14737ce 100644
--- a/logging/object_encoders.go
+++ b/logging/object_encoders.go
@@ -20,9 +20,7 @@ import (
 	"strings"
 
 	"go.uber.org/zap/zapcore"
-	"knative.dev/pkg/logging/logkey"
 
-	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
 )
 
@@ -42,19 +40,3 @@ func StringSet(s sets.String) zapcore.ObjectMarshalerFunc {
 		return nil
 	}
 }
-
-// NamespacedName returns a marshaler for NamespacedName.
-// To use this in sugared logger do:
-//	logger.Infow("Enqueuing", zap.Object("key", logging.NamespacedName(n)))
-// To use with non-sugared logger do:
-//	logger.Info("Enqueuing", zap.Object("key", logging.NamespacedName(n)))
-func NamespacedName(n types.NamespacedName) zapcore.ObjectMarshalerFunc {
-	return func(enc zapcore.ObjectEncoder) error {
-		if n.Namespace == "" {
-			enc.AddString(logkey.Key, n.Name)
-		} else {
-			enc.AddString(logkey.Key, n.Namespace+"/"+n.Name)
-		}
-		return nil
-	}
-}
diff --git a/metrics/config.go b/metrics/config.go
index 1bdf06607..093d26d45 100644
--- a/metrics/config.go
+++ b/metrics/config.go
@@ -341,14 +341,15 @@ func prometheusPort() (int, error) {
 	return int(pp), nil
 }
 
-// JsonToMetricsOptions converts a json string of a
-// ExporterOptions. Returns a non-nil ExporterOptions always.
+// JsonToMetricsOptions converts a JSON string to an ExporterOptions object.
 // TODO(vagababov): remove after updating deps.
+// Deprecated: Use JSONToOptions instead.
 func JsonToMetricsOptions(jsonOpts string) (*ExporterOptions, error) { //nolint // No rename due to backwards incompatibility. {
-	return JSONToMetricsOptions(jsonOpts)
+	return JSONToOptions(jsonOpts)
 }
 
-func JSONToMetricsOptions(jsonOpts string) (*ExporterOptions, error) {
+// JSONToOptions converts a json string to ExporterOptions.
+func JSONToOptions(jsonOpts string) (*ExporterOptions, error) {
 	var opts ExporterOptions
 	if jsonOpts == "" {
 		return nil, errors.New("json options string is empty")
@@ -361,13 +362,15 @@ func JSONToMetricsOptions(jsonOpts string) (*ExporterOptions, error) {
 	return &opts, nil
 }
 
-// MetricsOptionsToJson converts a ExporterOptions to a json string.
+// MetricsOptionsToJson converts an ExporterOptions object to a JSON string.
 // TODO(vagababov): remove after updating deps.
+// Deprecated: Use OptionsToJSON instead.
 func MetricsOptionsToJson(opts *ExporterOptions) (string, error) { //nolint // No rename due to backwards incompatibility.
-	return MetricsOptionsToJSON(opts)
+	return OptionsToJSON(opts)
 }
 
-func MetricsOptionsToJSON(opts *ExporterOptions) (string, error) {
+// OptionsToJSON converts an ExporterOptions object to a JSON string.
+func OptionsToJSON(opts *ExporterOptions) (string, error) {
 	if opts == nil {
 		return "", nil
 	}


Test patch:
diff --git a/metrics/config_test.go b/metrics/config_test.go
index 5e8f881e6..8d47a5716 100644
--- a/metrics/config_test.go
+++ b/metrics/config_test.go
@@ -921,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 := OptionsToJSON(tc.opts)
 			if err != nil {
 				t.Error("error while converting metrics config to json:", err)
 			}
@@ -937,7 +937,7 @@ func TestMetricsOptions(t *testing.T) {
 			// Test to options.
 			{
 				want := tc.opts
-				got, gotErr := JSONToMetricsOptions(jsonOpts)
+				got, gotErr := JSONToOptions(jsonOpts)
 
 				if gotErr != nil {
 					if diff := cmp.Diff(tc.wantErr, gotErr.Error()); diff != "" {