feat: add option `HooksWithFields`

[Laisky]

add new Option HooksWithFields.

It's similar with Hooks, but let hooks can access fields.

fully backward compatible.

[codecov[bot]]

Codecov Report

Merging #760 into master will increase coverage by 0.26%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #760      +/-   ##
==========================================
+ Coverage   98.09%   98.35%   +0.26%     
==========================================
  Files          42       43       +1     
  Lines        2150     2372     +222     
==========================================
+ Hits         2109     2333     +224     
+ Misses         33       32       -1     
+ Partials        8        7       -1     

Impacted Files	Coverage Δ
options.go	100.00% <100.00%> (ø)
zapcore/hook.go	100.00% <100.00%> (ø)
field.go	100.00% <0.00%> (ø)
encoder.go	100.00% <0.00%> (ø)
buffer/buffer.go	100.00% <0.00%> (ø)
zapcore/error.go	93.75% <0.00%> (ø)
zapcore/field.go	100.00% <0.00%> (ø)
zapcore/sampler.go	100.00% <0.00%> (ø)
zapcore/json_encoder.go	100.00% <0.00%> (ø)
... and 4 more

---

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 a6015e1...568f3b1. Read the comment docs.

[Laisky]

@prashantv Thank you for your explaining. But It seems hard to load internal fields from Core, so I add a new fields in Core, is this OK?

If it's OK, I'll write test cases later.

[prashantv]

@prashantv Thank you for your explaining. But It seems hard to load internal fields from Core, so I add a new fields in Core, is this OK?

Since Core is an interface, adding a new method to it is not backwards compatible. Any existing core implementations will no longer satisfy the interface with the new method, so we cannot make this change in zap 1.0.

Changing cores to track all fields also has a performance penalty, so we wouldn't want to pay this cost in all cases -- we should instead only pay the cost when the user uses a hook that requires access to fields.

Instead of adding a new method to the Core interface, you could store the fields as part of the hookWithFields core. That avoids any backwards incompatible changes, while limiting the performance penalty of storing all fields to users of the hook.

[Laisky]

@prashantv It seems that there's no way to get Core.enc without change Core interface, so hooks cannot get included fields that stored in Core.enc.

Do you have any idea that how to get included fields in hooks? or we have to give up included fields in hooks.

[prashantv]

It's not possible to get the fields out of existing cores, however, the wrapped core can store fields.

However, this will be limited to only access fields added after the wrapped core is added.

[Laisky]

@prashantv I updated the code, it can achieve this:

logger := zap.NewExample(zap.HooksWithFields(func(e zapcore.Entry, fs []zapcore.Field) error {
    fmt.Println(len(fs)) 
    return nil
}))

logger.With(zap.String("key", "val")).Info("test")

output:

1

[CLAassistant]

All committers have signed the CLA.

[titus12]

Laisky, Please submit

[brenu]

I guess I can make these requested changes starting from where Laisky stopped, and open it as a different PR. What do you guys think?

[prashantv]

I guess I can make these requested changes starting from where Laisky stopped, and open it as a different PR. What do you guys think?

Since this change hasn't been updated in a few months, that makes sense to me. Please credit @Laisky in your new PR as well (assuming it's based on this PR).