feat: add support for custom ReflectType encoder #1039

psrajat · 2021-12-14T15:24:23Z

Hi Team!
This fixes #1034

CLAassistant · 2021-12-14T15:24:29Z

All committers have signed the CLA.

codecov · 2021-12-14T17:14:12Z

Codecov Report

Merging #1039 (c65627f) into master (10b1fe4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1039   +/-   ##
=======================================
  Coverage   98.20%   98.21%           
=======================================
  Files          47       48    +1     
  Lines        2067     2071    +4     
=======================================
+ Hits         2030     2034    +4     
  Misses         29       29           
  Partials        8        8

Impacted Files	Coverage Δ
zapcore/encoder.go	`87.09% <ø> (ø)`
zapcore/json_encoder.go	`100.00% <100.00%> (ø)`
zapcore/reflected_encoder.go	`100.00% <100.00%> (ø)`
zapcore/sampler.go	`100.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10b1fe4...c65627f. Read the comment docs.

abhinav · 2021-12-14T17:12:07Z

zapcore/encoder.go

@@ -331,6 +332,9 @@ type EncoderConfig struct {
 	// Unlike the other primitive type encoders, EncodeName is optional. The
 	// zero value falls back to FullNameEncoder.
 	EncodeName NameEncoder `json:"nameEncoder" yaml:"nameEncoder"`
+	// Configure the encoder for interface{} type objects
+	// If not provided, objects are encoded using json.Encoder
+	NewReflectedEncoder func(io.Writer) ReflectedEncoder `json:"newReflectedEncoder" yaml:"newReflectedEncoder"`


Since this a function that is not JSON/YAML encodeable, we'll have to use - here.

Suggested change

NewReflectedEncoder func(io.Writer) ReflectedEncoder `json:"newReflectedEncoder" yaml:"newReflectedEncoder"`

NewReflectedEncoder func(io.Writer) ReflectedEncoder `json:"-" yaml:"-"`

abhinav · 2021-12-14T17:13:21Z

zapcore/json_encoder.go

+		var newReflectedEncoder func(io.Writer) ReflectedEncoder
+		if enc.NewReflectedEncoder == nil {
+			newReflectedEncoder = GetDefaultReflectedEncoder()
+		} else {
+			newReflectedEncoder = enc.NewReflectedEncoder
+		}


We should do this in NewJSONEncoder? We can set NewReflectedEncoder in there once if it's nil, and then we don't have to check it on the hot path.

@abhinav
This change was breaking TestJSONEncoderObjectFields test cases in json_encoder_impl_test.go so had to fix it by manually assigning default reflected encoder.
LMK if there are any concerns.

abhinav · 2021-12-14T17:18:55Z

zapcore/json_encoder.go

+		if enc.NewReflectedEncoder == nil {
+			newReflectedEncoder = GetDefaultReflectedEncoder()
+		} else {
+			newReflectedEncoder = enc.NewReflectedEncoder


Would you please add a test for the custom reflection encoder case?
We don't want us to introduce a new dependency for that,
so a fake implementation of the function will suffice.

abhinav · 2021-12-14T17:19:17Z

zapcore/reflected_encoder.go

+)
+
+// A ReflectedEncoder handles the serialization of Field which have FieldType = ReflectType and writes them to the underlying data stream
+// The underlying data stream is provided by zap during initialization. See EncoderConfig.NewReflectedEncoder


Suggested change

// The underlying data stream is provided by zap during initialization. See EncoderConfig.NewReflectedEncoder

// The underlying data stream is provided by Zap during initialization. See EncoderConfig.NewReflectedEncoder.

abhinav · 2021-12-14T17:19:31Z

zapcore/reflected_encoder.go

+// A ReflectedEncoder handles the serialization of Field which have FieldType = ReflectType and writes them to the underlying data stream
+// The underlying data stream is provided by zap during initialization. See EncoderConfig.NewReflectedEncoder
+type ReflectedEncoder interface {
+	// Encode encodes and writes to the underlying data stream


Suggested change

// Encode encodes and writes to the underlying data stream

// Encode encodes and writes to the underlying data stream.

abhinav · 2021-12-14T17:23:19Z

zapcore/reflected_encoder.go

+	Encode(interface{}) error
+}
+
+func GetDefaultReflectedEncoder() func(writer io.Writer) ReflectedEncoder {


We would prefer not to export this function. We also don't need the wrapper type because json.Encoder satisfies the ReflectedEncoder interface.

So something like the following should suffice:

func defaultReflectedEncoder(w io.Writer) ReflectedEncoder { enc := json.NewEncoder(w) enc.SetEscapeHTML(false) return enc }

* fix comments * don't export default ReflectedEncoder fn * move newReflectEncoder nil check to NewJsonEncoder()

The defaultReflectedEncoder function can satisfy the signature for `NewReflectedEncoder`, so we don't need to return a closure. And the returned `json.Encoder` matches the ReflectedEncoder interface so we don't need the wrapper type either.

assertOutput manually sets up a jsonEncoder. This means that any config initalization we do in the constructor has to be replicated in assertOutput. Instead, use the constructor and cast to `*jsonEncoder`.

zapcore/json_encoder_test.go

sywhang · 2021-12-15T00:39:25Z

zapcore/json_encoder_test.go

+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			buf, err := enc.EncodeEntry(zapcore.Entry{


Suggested change

for _, tt := range tests {

t.Run(tt.name, func(t *testing.T) {

buf, err := enc.EncodeEntry(zapcore.Entry{

for _, tt := range tests {

tt := tt

t.Run(tt.name, func(t *testing.T) {

t.Parallel()

buf, err := enc.EncodeEntry(zapcore.Entry{

Oh, we can do that, although for that the encoder has to be instantiated per-test.
(Encoder isn't safe for concurrent use.)
I'll do it.

Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com>

abhinav · 2021-12-15T01:37:43Z

Thanks!

This release v1.20.0 of Zap with a couple new features for customizing the JSON encoder. Namely, - support skipping newlines between log statements (#989) - support changing the reflection JSON encoder (#1039) Refs GO-1085

feat: add support for custom ReflectType encoder

f9e2251

psrajat mentioned this pull request Dec 14, 2021

feat: Custom interface{} object Encoder #1034

Closed

add license

ba0bbf8

abhinav reviewed Dec 14, 2021

View reviewed changes

psrajat and others added 4 commits December 15, 2021 00:09

Add Tests

ce7215a

* fix comments * don't export default ReflectedEncoder fn * move newReflectEncoder nil check to NewJsonEncoder()

fix TestJSONEncoderObjectFields test

31bc967

test/assertOutput: Use NewJSONEncoder to instantiate

17016b9

assertOutput manually sets up a jsonEncoder. This means that any config initalization we do in the constructor has to be replicated in assertOutput. Instead, use the constructor and cast to `*jsonEncoder`.

sywhang approved these changes Dec 15, 2021

View reviewed changes

abhinav and others added 2 commits December 14, 2021 16:56

Rename TestJSONCustomReflectionEncoder

bd74de7

Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com>

TestJSONCustomReflectedEncoder: parallelize

c65627f

abhinav approved these changes Dec 15, 2021

View reviewed changes

abhinav merged commit 369c1bd into uber-go:master Dec 15, 2021

abhinav mentioned this pull request Jan 4, 2022

Release v1.12.0 #1045

Merged

github-actions bot mentioned this pull request Apr 10, 2022

Link Checker Report scylladb/scylla-manager#3102

Closed

github-actions bot mentioned this pull request Apr 17, 2022

Link Checker Report scylladb/scylla-manager#3121

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for custom ReflectType encoder #1039

feat: add support for custom ReflectType encoder #1039

psrajat commented Dec 14, 2021

CLAassistant commented Dec 14, 2021 •

edited

codecov bot commented Dec 14, 2021 •

edited

abhinav Dec 14, 2021

abhinav Dec 14, 2021

psrajat Dec 14, 2021

abhinav Dec 14, 2021

abhinav Dec 14, 2021

abhinav Dec 14, 2021

abhinav Dec 14, 2021

sywhang Dec 15, 2021

abhinav Dec 15, 2021

abhinav commented Dec 15, 2021

	NewReflectedEncoder func(io.Writer) ReflectedEncoder `json:"newReflectedEncoder" yaml:"newReflectedEncoder"`
	NewReflectedEncoder func(io.Writer) ReflectedEncoder `json:"-" yaml:"-"`

	// The underlying data stream is provided by zap during initialization. See EncoderConfig.NewReflectedEncoder
	// The underlying data stream is provided by Zap during initialization. See EncoderConfig.NewReflectedEncoder.

	// Encode encodes and writes to the underlying data stream
	// Encode encodes and writes to the underlying data stream.

feat: add support for custom ReflectType encoder #1039

feat: add support for custom ReflectType encoder #1039

Conversation

psrajat commented Dec 14, 2021

CLAassistant commented Dec 14, 2021 • edited

codecov bot commented Dec 14, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinav commented Dec 15, 2021

CLAassistant commented Dec 14, 2021 •

edited

codecov bot commented Dec 14, 2021 •

edited