Skip to content

Conversation

@webmat
Copy link
Contributor

@webmat webmat commented Sep 4, 2019

This is meant to help clarify the difference between log and event.

Relates to #525 and address some concerns voiced in #516.

Copy link
Contributor

@MikePaquette MikePaquette left a comment

Choose a reason for hiding this comment

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

Suggested some wording tweaks, then LGTM.

@webmat
Copy link
Contributor Author

webmat commented Sep 25, 2019

Ok, just updated this PR with Mike's comments.

I think this clarification about the log field set definition will really help users understand the difference between the what goes in log and what goes in event & elsewhere in ECS.

Let's review whether this new proposed definition fits well with the fields under log.*.

What works well with the new definition

  • log.original: There is also a lot of confusion between the fields log.original and event.original. I think we should deprecate one of the two, as I cannot imagine anyone deciding to store two full, but slightly different original copies of the original log event. After this PR, I think it would make most sense to keep log.original as the one to use. event.* is not for the low level details, this is now the clearly stated purpose of log.*.
  • log.logger: I think this makes sense. This is the class name of the logger instance that emitted the event, in the context of application logging. 👍
  • log.origin.file.* and log.origin.function: I was initially on the fence here. I've been pushing back on these in Add log.origin fields #563, but I've come around. This is a really low level detail about the source of an application log. :-)

What does not work as well with the new definition

  • log.level: after A new take on adding missing Syslog fields to ECS. #525, this field will be clearly mapped to the source's severity, not the transport's severity (when they differ). However "log level" is an expression so widespread that I think it's ok to have it under here.

I'll merge this tomorrow, unless @ruflin & @MikePaquette chime back in with additional comments / suggestions.

@webmat webmat mentioned this pull request Sep 25, 2019
Copy link
Contributor

@MikePaquette MikePaquette left a comment

Choose a reason for hiding this comment

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

LGTM

@webmat webmat merged commit 699c812 into elastic:master Sep 26, 2019
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.

3 participants