Skip to content

Conversation

@vispariana
Copy link
Contributor

This adds the ECS standard "log.origin.function" field to the caller fields, alongside "log.origin.file.line" and "log.origin.file.name".

Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, @vispariana! I don't think it's the encoder's responsibility to be determining the function name. Otherwise looks good.

encoder.go Outdated
func encodeCaller(c *caller, enc zapcore.PrimitiveArrayEncoder) {
// this function can only be called internally so we have full control over it
// and can ensure that enc is always of type zapcore.ArrayEncoder
c.Function = runtime.FuncForPC(c.PC).Name()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the encoder should be responsible for determining the function name, only for encoding it if it is recorded.

Zap will already record Function if WithCaller(true) is used: https://github.com/uber-go/zap/blob/a55bdc32f526699c3b4cc51a2cc97e944d02fbbf/logger.go#L374-L382

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. Thanks for reviewing. Encoder tests manually marshal the EntryCaller, and EntryCaller's Function fields gets filled with zap's unexported check() method, so the function field was empty in tests. I fixed the tests.

@cla-checker-service
Copy link

cla-checker-service bot commented Dec 17, 2022

💚 CLA has been signed

Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect! Thanks for your contribution and for making the update.

@axw axw merged commit d41e5d0 into elastic:main Dec 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants