Repository: go-logr/logr
Pull request #91 (2021-09-20 17:57:43)

LogObject: optional interface for logging objects differently
The intention is to use this when the output is structured (like JSON) when the
original type would be logged as string. It also has some other use cases.

This approach was chosen instead of a full marshaler API as in
zapcore.ObjectMarshaler because the implementation is simpler. The overhead for
large types is expected to be higher, but it is not certain yet whether this is
relevant in practice. If it is, then a marshaler API can still be added later.

For alternatives, see the discussion in https://github.com/kubernetes/klog/issues/262

@wojas: @thockin and I were already discussing this a bit. Here's a formal PR, perhaps you can also have a look?

pohly: @thockin please take another look. I updated the comments to address your remarks.

wojas: I had a quick look at the discussion. I cannot think of a better way to solve this. It is clean and generic enough for any logger to directly support it without adding a dependency on logr.

My only nit is about the naming. Calling the interface `LogObject` and the method `GetLogObject` feels out of place in Go.

Something like `LogObjecter` for the interface and `LogObject` for the method would be more in line with general Go naming, but perhaps feels a bit clunky.

Perhaps `LogMarshaler` and `LogMarshal` in line with the [encoding package](https://pkg.go.dev/encoding)? Not sure if `Marshal` would be confusing for non-`[]byte` return values.


pohly: > Something like LogObjecter for the interface and LogObject for the method would be more in line with general Go naming, but perhaps feels a bit clunky.

My first reaction was "LogObjecter - what is is objecting against"? `LogObject` as method name is okay.

Perhaps "LogObjectProvider" as interface name?

> Perhaps LogMarshaler and LogMarshal in line with the encoding package? Not sure if Marshal would be confusing for non-[]byte return values.

That might also get confused with [zapcore.ObjectMarshaler](https://github.com/uber-go/zap/blob/10d89a76cc8b9787e408aee8882e40a8bd29c585/zapcore/marshaler.go#L30). Both work differently, so I think "marshaler" here is not the right word.
wojas: I just noticed that the encoding package and zap actually switch the order, so it would be:

```go
type LogMarshaler interface {
    MarshalLog() interface{}
}
```

While it could get confused with `zapcore.ObjectMarshaler`, the signature is very different and the number of people familiar with that interface is likely small. 

`zapcore.ObjectMarshaler` also does not return `[]byte`, so that part is fine. 

And `gopkg.in/yaml.v2` [uses a similar interface](https://pkg.go.dev/gopkg.in/yaml.v2#Marshaler):

```go
type Marshaler interface {
    MarshalYAML() (interface{}, error)
}
```

pohly: > the number of people familiar with that interface is likely small

True, I am probably biased after having discussed it at length :grin: 

With the similarity to yaml in mind I retract my comment. I'm fine with "LogMarshaler + MarshalLog".
thockin: I previously objected  to "Marshal" because that is so strongly associated
with serialization formats (the only times I have ever come across that
word in my career is wrt JSON, YAML, SOAP, XML, SunRPC etc).

I don't hate it in this context, I guess.  The familiarity is nice.  Is
`MarshalLog` right?  MarshalJSON() --> JSON.  MarshalYAML() --> YAML.
MarshalLog -/-> Log.

Just trying for size - `MarshalForLog()`, `MarshalLogObject()`,
`MarshalLoggable()`  ?  Are any of those better?

If we can't do better, I will yield - we could certainly do worse.

On Wed, Sep 22, 2021 at 12:39 AM Patrick Ohly ***@***.***>
wrote:

> the number of people familiar with that interface is likely small
>
> True, I am probably biased after having discussed it at length 😁
>
> With the similarity to yaml in mind I retract my comment. I'm fine with
> "LogMarshaler + MarshalLog".
>
> —
> You are receiving this because you were assigned.
> Reply to this email directly, view it on GitHub
> <https://github.com/go-logr/logr/pull/91#issuecomment-924667550>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ABKWAVD7T5LOO2EDGUKRVPLUDGBZZANCNFSM5EMPTIXA>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
>

wojas: My take on those alternatives:

`MarshalForLog()` is inconsistent with the common FooMarshaler / MarshalFoo naming pattern.

`MarshalLogObject()` and `MarshalLoggable()`  are not bad, but longer without making it much clearer.

> MarshalJSON() --> JSON.  MarshalYAML() --> YAML. MarshalLog -/-> Log.

Depends on how you look at it. MarshalYAML is used to convert it into something that will end up in YAML, it does not return YAML. Similarly MarshalLog will convert it into something that will end up in the log.


thockin: Ok, I can buy those arguments.

I'll take another look ASAP.

On Thu, Sep 23, 2021, 12:03 AM wojas ***@***.***> wrote:

> My take on those alternatives:
>
> MarshalForLog() is inconsistent with the common FooMarshaler / MarshalFoo
> naming pattern.
>
> MarshalLogObject() and MarshalLoggable() are not bad, but longer without
> making it much clearer.
>
> MarshalJSON() --> JSON. MarshalYAML() --> YAML. MarshalLog -/-> Log.
>
> Depends on how you look at it. MarshalYAML is used to convert it into
> something that will end up in YAML, it does not return YAML. Similarly
> MarshalLog will convert it into something that will end up in the log.
>
> —
> You are receiving this because you were assigned.
> Reply to this email directly, view it on GitHub
> <https://github.com/go-logr/logr/pull/91#issuecomment-925553362>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ABKWAVC623J25PSSSN4EEG3UDLGNTANCNFSM5EMPTIXA>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
>

pohly: I've switched to `logr.Marshaler` and `MarshalLog` and also addressed the pending feedback.


Patch:
diff --git a/README.md b/README.md
index f55484d..84cccf1 100644
--- a/README.md
+++ b/README.md
@@ -112,7 +112,7 @@ There are implementations for the following logging libraries:
 - **github.com/sirupsen/logrus**: [logrusr](https://github.com/bombsimon/logrusr)
 - **github.com/wojas/genericr**: [genericr](https://github.com/wojas/genericr) (makes it easy to implement your own backend)
 - **logfmt** (Heroku style [logging](https://www.brandur.org/logfmt)): [logfmtr](https://github.com/iand/logfmtr)
-- **github.com/rs/zerolog**: [zerologr](https://github.com/screenleap/zerologr)
+- **zerolog**: [zerologr](https://github.com/hn8/zerologr)
 
 ## FAQ
 
diff --git a/funcr/funcr.go b/funcr/funcr.go
index 965fd75..b7acfc0 100644
--- a/funcr/funcr.go
+++ b/funcr/funcr.go
@@ -170,6 +170,10 @@ const (
 func prettyWithFlags(value interface{}, flags uint32) string {
 	// Handling the most common types without reflect is a small perf win.
 	switch v := value.(type) {
+	case logr.Marshaler:
+		// Replace the value with what the value wants to get logged.
+		// That then gets handled below via reflection.
+		value = v.MarshalLog()
 	case bool:
 		return strconv.FormatBool(v)
 	case string:
@@ -177,7 +181,7 @@ func prettyWithFlags(value interface{}, flags uint32) string {
 			return v
 		}
 		// This is empirically faster than strings.Builder.
-		return strconv.Quote(v)
+		return `"` + v + `"`
 	case int:
 		return strconv.FormatInt(int64(v), 10)
 	case int8:
@@ -204,10 +208,6 @@ func prettyWithFlags(value interface{}, flags uint32) string {
 		return strconv.FormatFloat(float64(v), 'f', -1, 32)
 	case float64:
 		return strconv.FormatFloat(v, 'f', -1, 64)
-	case complex64:
-		return `"` + strconv.FormatComplex(complex128(v), 'f', -1, 64) + `"`
-	case complex128:
-		return `"` + strconv.FormatComplex(v, 'f', -1, 128) + `"`
 	}
 
 	buf := bytes.NewBuffer(make([]byte, 0, 256))
@@ -233,10 +233,6 @@ func prettyWithFlags(value interface{}, flags uint32) string {
 		return strconv.FormatFloat(float64(v.Float()), 'f', -1, 32)
 	case reflect.Float64:
 		return strconv.FormatFloat(v.Float(), 'f', -1, 64)
-	case reflect.Complex64:
-		return `"` + strconv.FormatComplex(complex128(v.Complex()), 'f', -1, 64) + `"`
-	case reflect.Complex128:
-		return `"` + strconv.FormatComplex(v.Complex(), 'f', -1, 128) + `"`
 	case reflect.Struct:
 		buf.WriteRune('{')
 		for i := 0; i < t.NumField(); i++ {
diff --git a/logr.go b/logr.go
index 4e84ab7..3751180 100644
--- a/logr.go
+++ b/logr.go
@@ -477,3 +477,21 @@ type CallStackHelperLogSink interface {
 	// call site information.
 	GetCallStackHelper() func()
 }
+
+// Marshaler is an optional interface that logged values may choose to
+// implement. Loggers with structured output, such as JSON, should
+// log the object return by the MarshalLog method instead of the
+// original value.
+type Marshaler interface {
+	// MarshalLog can be used to:
+	//   - ensure that structs are not logged as strings when the original
+	//     value has a String method: return a different type without a
+	//     String method
+	//   - select which fields of a complex type should get logged:
+	//     return a simpler struct with fewer fields
+	//   - log unexported fields: return a different struct
+	//     with exported fields
+	//
+	// It may return any value of any type.
+	MarshalLog() interface{}
+}


Test patch:
diff --git a/example_marshaler_secret_test.go b/example_marshaler_secret_test.go
new file mode 100644
index 0000000..1e59af3
--- /dev/null
+++ b/example_marshaler_secret_test.go
@@ -0,0 +1,47 @@
+/*
+Copyright 2021 The logr Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package logr_test
+
+import (
+	"github.com/go-logr/logr"
+)
+
+// ComplexObjectRef contains more fields than it wants to get logged.
+type ComplexObjectRef struct {
+	Name      string
+	Namespace string
+	Secret    string
+}
+
+func (ref ComplexObjectRef) MarshalLog() interface{} {
+	return struct {
+		Name, Namespace string
+	}{
+		Name:      ref.Name,
+		Namespace: ref.Namespace,
+	}
+}
+
+var _ logr.Marshaler = ComplexObjectRef{}
+
+func ExampleMarshaler_secret() {
+	l := NewStdoutLogger()
+	secret := ComplexObjectRef{Namespace: "kube-system", Name: "some-secret", Secret: "do-not-log-me"}
+	l.Info("simplified", "secret", secret)
+	// Output:
+	// "level"=0 "msg"="simplified" "secret"={"Name":"some-secret","Namespace":"kube-system"}
+}
diff --git a/example_marshaler_test.go b/example_marshaler_test.go
new file mode 100644
index 0000000..934b574
--- /dev/null
+++ b/example_marshaler_test.go
@@ -0,0 +1,69 @@
+/*
+Copyright 2021 The logr Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package logr_test
+
+import (
+	"fmt"
+
+	"github.com/go-logr/logr"
+	"github.com/go-logr/logr/funcr"
+)
+
+// NewStdoutLogger returns a logr.Logger that prints to stdout.
+func NewStdoutLogger() logr.Logger {
+	return funcr.New(func(prefix, args string) {
+		if prefix != "" {
+			_ = fmt.Sprintf("%s: %s\n", prefix, args)
+		} else {
+			fmt.Println(args)
+		}
+	}, funcr.Options{})
+}
+
+// ObjectRef references a Kubernetes object
+type ObjectRef struct {
+	Name      string `json:"name"`
+	Namespace string `json:"namespace,omitempty"`
+}
+
+func (ref ObjectRef) String() string {
+	if ref.Namespace != "" {
+		return ref.Namespace + "/" + ref.Name
+	}
+	return ref.Name
+}
+
+func (ref ObjectRef) MarshalLog() interface{} {
+	// We implement fmt.Stringer for non-structured logging, but we want the
+	// raw struct when using structured logs.  Some logr implementations call
+	// String if it is present, so we want to convert this struct to something
+	// that doesn't have that method.
+	type forLog ObjectRef // methods do not survive type definitions
+	return forLog(ref)
+}
+
+var _ logr.Marshaler = ObjectRef{}
+
+func ExampleMarshaler() {
+	l := NewStdoutLogger()
+	pod := ObjectRef{Namespace: "kube-system", Name: "some-pod"}
+	l.Info("as string", "pod", pod.String())
+	l.Info("as struct", "pod", pod)
+	// Output:
+	// "level"=0 "msg"="as string" "pod"="kube-system/some-pod"
+	// "level"=0 "msg"="as struct" "pod"={"name":"some-pod","namespace":"kube-system"}
+}
diff --git a/funcr/funcr_test.go b/funcr/funcr_test.go
index 1c201c9..735cbd6 100644
--- a/funcr/funcr_test.go
+++ b/funcr/funcr_test.go
@@ -36,115 +36,84 @@ func ptrstr(s string) *string {
 }
 
 func TestPretty(t *testing.T) {
-	cases := []struct {
-		val interface{}
-		exp string // used in cases where JSON can't handle it
-	}{
-		{val: "strval"},
-		{val: substr("substrval")},
-		{val: true},
-		{val: false},
-		{val: int(93)},
-		{val: int8(93)},
-		{val: int16(93)},
-		{val: int32(93)},
-		{val: int64(93)},
-		{val: int(-93)},
-		{val: int8(-93)},
-		{val: int16(-93)},
-		{val: int32(-93)},
-		{val: int64(-93)},
-		{val: uint(93)},
-		{val: uint8(93)},
-		{val: uint16(93)},
-		{val: uint32(93)},
-		{val: uint64(93)},
-		{val: uintptr(93)},
-		{val: float32(93.76)},
-		{val: float64(93.76)},
-		{
-			val: complex64(93i),
-			exp: `"(0+93i)"`,
+	cases := []interface{}{
+		"strval",
+		substr("substrval"),
+		true,
+		false,
+		int(93),
+		int8(93),
+		int16(93),
+		int32(93),
+		int64(93),
+		int(-93),
+		int8(-93),
+		int16(-93),
+		int32(-93),
+		int64(-93),
+		uint(93),
+		uint8(93),
+		uint16(93),
+		uint32(93),
+		uint64(93),
+		uintptr(93),
+		float32(93.76),
+		float64(93.76),
+		ptrint(93),
+		ptrstr("pstrval"),
+		[]int{9, 3, 7, 6},
+		[4]int{9, 3, 7, 6},
+		struct {
+			Int    int
+			String string
+		}{
+			93, "seventy-six",
 		},
-		{
-			val: complex128(93i),
-			exp: `"(0+93i)"`,
+		map[string]int{
+			"nine": 3,
 		},
-		{val: ptrint(93)},
-		{val: ptrstr("pstrval")},
-		{val: []int{9, 3, 7, 6}},
-		{val: [4]int{9, 3, 7, 6}},
-		{
-			val: struct {
-				Int    int
-				String string
-			}{
-				93, "seventy-six",
-			},
+		map[substr]int{
+			"nine": 3,
 		},
-		{
-			val: map[string]int{
-				"nine": 3,
-			},
+		fmt.Errorf("error"),
+		struct {
+			X int `json:"x"`
+			Y int `json:"y"`
+		}{
+			93, 76,
 		},
-		{
-			val: map[substr]int{
-				"nine": 3,
-			},
+		struct {
+			X []int
+			Y map[int]int
+			Z struct{ P, Q int }
+		}{
+			[]int{9, 3, 7, 6},
+			map[int]int{9: 3},
+			struct{ P, Q int }{9, 3},
 		},
-		{val: fmt.Errorf("error")},
-		{
-			val: struct {
-				X int `json:"x"`
-				Y int `json:"y"`
-			}{
-				93, 76,
-			},
+		[]struct{ X, Y string }{
+			{"nine", "three"},
+			{"seven", "six"},
 		},
-		{
-			val: struct {
-				X []int
-				Y map[int]int
-				Z struct{ P, Q int }
-			}{
-				[]int{9, 3, 7, 6},
-				map[int]int{9: 3},
-				struct{ P, Q int }{9, 3},
-			},
-		},
-		{
-			val: []struct{ X, Y string }{
-				{"nine", "three"},
-				{"seven", "six"},
-			},
-		},
-		{
-			val: struct {
-				A *int
-				B *int
-				C interface{}
-				D interface{}
-			}{
-				B: ptrint(1),
-				D: interface{}(2),
-			},
+		struct {
+			A *int
+			B *int
+			C interface{}
+			D interface{}
+		}{
+			B: ptrint(1),
+			D: interface{}(2),
 		},
 	}
 
 	for i, tc := range cases {
-		ours := pretty(tc.val)
-		want := ""
-		if tc.exp != "" {
-			want = tc.exp
-		} else {
-			jb, err := json.Marshal(tc.val)
-			if err != nil {
-				t.Errorf("[%d]: unexpected error: %v", i, err)
-			}
-			want = string(jb)
+		ours := pretty(tc)
+		std, err := json.Marshal(tc)
+		if err != nil {
+			t.Errorf("[%d]: unexpected error: %v", i, err)
 		}
-		if ours != want {
-			t.Errorf("[%d]: expected %q, got %q", i, want, ours)
+		if ours != string(std) {
+			t.Errorf("[%d]: expected %q, got %q", i, std, ours)
 		}
 	}
 }