Add minimal viable version for ECS encoder for zap.

[simitt]

Adds minimal required ECS information to every log output.
Uses ECS version 1.5.0.

related to #elastic/ecs-logging#15

[axw]

Looks good, but I think we should also consider how errors will be encoded.

By overriding the zapcore.Encoder, we only have limited means of influencing the output structure. The standard zapcore.Core implementation is a bit too opinionated about the structure of errors:

https://github.com/uber-go/zap/blob/9646482274d01f82b74200da5598b5964c2ea828/zapcore/error.go#L28-L61

I think we might need to provide a zapcore.Core if we want to format errors according to ECS. This would also give us an opportunity to enrich the errors, i.e. to set error.id, error.code, error.type; and at some point, ideally to set error causes.

[simitt]

@axw thank you for the detailed feedback!

Regarding the error handling, I was working on another branch on adding ecszap types and methods returning a zapcore.Field to allow for more extensive support. You can have a look at additional-ecs-fields/ecs.go for the generated fields and at the test example. The example currently doesn't deal with full errors, but it shows the bigger picture. The idea would be that users only use the zap.XXX fields for custom, non ECS fields, but for everything else set the ecszap fields, that are strongly typed.

What do you think about this solution? I will also look a bit into providing a full zapcore.Core, but so far haven't found a reason for not moving forward with the encoder.

I tried to keep this PR small with a minimum initial support, therefore I created a second branch with more extensive support.

[axw]

@simitt I think that'll lead to an unfriendly API, forcing users to create an ECS structure for every error they log.

We could provide an ecszap.Error function which would be analogous to zap.Error, but does the work of extracting error information into the ECS object structure. The main issue I have with this is that it's much more invasive. If we provided a Core, then users could leave all their log calls the same, only changing the Core, along the lines of:

logger := zap.New(ecszap.NewCore(os.Stdout, zap.DebugLevel))
...
logger.With(zap.Error(err)).Error("boom")

If we didn't format the error objects automatically, then I think users would need to update all logging call sites.

(BTW I'm definitely on board with creating an MVP, but this probably affects the public API. If we go the Core route, then I'm not sure it would make sense to expose an encoder at all.)

[simitt]

@axw I am glad we are having the discussion now, as I fully agree that we should decide on which route to go before merging in the MVP.

I'll close this for now, and revisit.

[axw]

Thanks, I think that works pretty well. I don't love having to copy the code, but I suppose it's not the end of the world if we do have to. I noted a possible alternative, but I may have missed something.

It occurs to me that we probably do need to expose the encoder, and make it possible to wrap another Core. I'm thinking of the libbeat/logp, which has Core implementations for Syslog and Windows Event Log. I think we would need to wrap the syslog/eventlog Core with ecszap.Core, and the syslog/eventlog would call the ecszap-provided encoder.

e.g.

cfg := ecszap.NewDefaultEncoderConfig()
encoder := ecszap.NewEncoder(cfg)

// This could also be a syslog/eventlog Core. The important thing is that we provide it an ecszap encoder, and later wrap the core so that errors don't get mangled.
core := zapcore.NewCore(encoder, os.Stdout, zap.DebugLevel)

logger := zap.New(ecszap.WrapCore(core), zap.AddCaller())
defer logger.Sync()

It becomes a little more fragile, as a user might create the encoder but forget to wrap the underlying core. We could continue to provide and primarily recommend the all-in-one NewCore constructor, but also provide these other bits for more advanced composition.

@simitt @urso WDYT?

[axw]

I was just looking at the zapcore code, and the only thing I saw that made it difficult to reuse the JSONEncoder (i.e. without copying the source code) is Caller. Did I miss any others?

Caller is tricky because CallerEncoder takes a PrimitiveArrayEncoder, which doesn't provide a means of encoding an object.

However, it's called with a *zapcore.jsonEncoder: https://github.com/uber-go/zap/blob/9646482274d01f82b74200da5598b5964c2ea828/zapcore/json_encoder.go#L367

So I think we could just type-assert it to ArrayEncoder, and use AppendObject with the caller details. Kinda dirty, but since we're not making the underlying encoder configurable it should be of minimal risk, and then we could avoid duplicating all the code. Not sure if it's worthwhile or not.

[simitt]

The ecs caller implementation sets 2 keys ("log.origin.file.line", "log.origin.file.name"), whereas the zapcore implementation only one, and all the key setting logic is private. But I agree, we could work around that with some kind of hack.

I did not find a way to work around the error encoding though. EncodeEntry calls addFields which is implemented in the ecszap package, allowing for dedicated error handling. I didn't find a solution avoiding to copy all the encoder code but still allowing to encode errors in an ecs compliant way. Please let me know if you have any ideas for this.

[axw]

@simitt and I discussed this live, but for posterity:

• I had in mind that for caller we would just encode one key, but with an object type. e.g. "log.origin": {"file.line": 123, "file.name": "foo.go"}. In this case, we can stick with the one caller key which the zapcore JSON encoder makes configurable, and use the type-assertion hack to encode an object value.
• For errors, before handing off to the zapcore JSON encoder, we can pre-process the fields to replace any ErrorType fields with an ObjectType that encodes the error according to ECS

[axw]

LGTM!

[urso]

With code copied directly from zap, do we need to add some license info to the file header?

[urso]

I'd be in favor if we make timestamp encoding configurable. One might want to use nanoseconds, milliseconds precision or Text output with timezone.

[simitt]

#9