Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Objects and ObjectValues field constructors #1071

Merged
merged 4 commits into from Mar 22, 2022
Merged

Add Objects and ObjectValues field constructors #1071

merged 4 commits into from Mar 22, 2022

Conversation

abhinav
Copy link
Contributor

@abhinav abhinav commented Mar 19, 2022

This pull request adds two new field constructors for slices, parameterized
over the types of the values in the slices:

func Objects[T zapcore.ObjectMarshaler](key string, value []T) Field

This turns a slice of any Zap marshalable object into a Zap field.

func ObjectValues[T any, P objectMarshalerPtr[T]](key string, values []T) Field

This is a variant of Objects for the case where *T implements
zapcore.ObjectMarshaler but we have a []T.

Together, these two field constructors obviate the need for writing
ArrayMarshaler implementations for slices of objects:

// Before //////////////

type userArray []*User

func (uu userArray) MarshalLogArray(enc zapcore.ArrayEncoder) error {
    for _, u := range uu {
        if err := enc.AppendObject(u); err != nil {
            return err
        }
    }
    return nil
}

var users []*User = ..
zap.Array("users", userArray(users))

// Now /////////////////

var users []*User = ..
zap.Objects("users", users)

Backwards compatibility

Both new field constructors are hidden behind a build tag, but use of type
parameters requires us to bump the go directive in the go.mod file to 1.18.

Note that this does not break Zap on Go 1.17.
Per the documentation for the go directive,

If an older Go version builds one of the module’s packages and encounters a
compile error, the error notes that the module was written for a newer Go
version.

It only breaks our ability to run go mod tidy inside Zap from Go 1.17,
which is okay because with the go directive, we're stating that we're
developing Zap while on Go 1.18.

Linting

staticcheck support for type parameters is expected in a few weeks.
Meanwhile, this disables staticcheck for the make lint target.
(I considered switching linting to Go 1.17 until then, but that isn't
enough because the formatting linter expects valid Go code--which type
parameters aren't in 1.17.)

Add a new Zap field constructor that encodes an array of zero or more
Zap-marshalable objects into a Zap array.

Usage:

    // Given,
    //
    // func (*User) MarshalLogObject(zapcore.ObjectEncoder) error

    zap.Objects("foo", []*User{u1, u2, u3})

Before this, users would have to resort to constructing a `[]*User`
wrapper, and use Zap's `Array` constructor:

    // Before:

    type users []*User

    func (uu users) MarshalLogArray(enc zapcore.ArrayEncoder) error a
        for _, u := range uu {
            if err := enc.AppendObject(u); err != nil {
                return err
            }
        }
        return nil
    }

    zap.Array("foo", users{u1, u2, u3})

Credit to ztstewart for the suggestion.

Refs GO-618

Reviewed-At: D7294727
Reviewed-By: ztstewart
@codecov
Copy link

codecov bot commented Mar 19, 2022

Codecov Report

Merging #1071 (bff2745) into master (34cb012) will increase coverage by 0.10%.
The diff coverage is 100.00%.

Current head bff2745 differs from pull request most recent head 854d895. Consider uploading reports for the commit 854d895 to get more accurate results

@@            Coverage Diff             @@
##           master    #1071      +/-   ##
==========================================
+ Coverage   98.19%   98.30%   +0.10%     
==========================================
  Files          48       49       +1     
  Lines        2111     2120       +9     
==========================================
+ Hits         2073     2084      +11     
+ Misses         29       28       -1     
+ Partials        9        8       -1     
Impacted Files Coverage Δ
array_go118.go 100.00% <100.00%> (ø)
zapcore/sampler.go 100.00% <0.00%> (+3.77%) ⬆️

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 34cb012...854d895. Read the comment docs.

In the previous change, we added a Objects field constructor to support
logging arrays of any object supported by zap.Object.

    func (*User) MarshalLogObject(...) error { ... }
    var users []*User = ...
    zap.Object("users", users)

That solution is incomplete because it does not cover the case when,
instead of a `[]*User`, we have a `[]User` despite the method being on
`*User`.

This change adds a similar ObjectValues field constructor that handles the
case where we have a `[]T` but the MarshalLogObject is on `*T`.

This works by declaring a new type constraint objectMarshalerPtr.
`objectMarshalerPtr[T]` is true for a type that is a pointer to `T`,
and implements zapcore.ObjectMarshaler.

Note that we still do not cover the case where the method is on the value
receiver (`func (User) MarshalLogObject(..) error`) and we have a
`[]*User`. We can add that when it becomes necessary.

Reviewed-At: D7295739
Reviewed-By: ztstewart
@abhinav abhinav force-pushed the abg/objects branch 2 times, most recently from bb9b132 to a648660 Compare Mar 22, 2022
@abhinav abhinav marked this pull request as ready for review Mar 22, 2022
Copy link
Contributor

@sywhang sywhang left a comment

neat :) left a minor nit on the comment but LGTM otherwise.

array_go118.go Outdated Show resolved Hide resolved
kishoresenji and others added 2 commits Mar 22, 2022
Minor optimization to avoid copying the struct to a variable.

Refs GO-618

Reviewed-At: D7297933
Reviewed-By: abhinav
staticcheck is expected to have support for generics-based code in a few
weeks. Disable it until then.
@abhinav abhinav merged commit 15be647 into master Mar 22, 2022
6 checks passed
@abhinav abhinav deleted the abg/objects branch Mar 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants