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

Rename log.message to log.original #106

Merged
merged 3 commits into from
Aug 29, 2018

Conversation

ruflin
Copy link
Member

@ruflin ruflin commented Aug 28, 2018

log.message was confused with message. To bring it in line with #102 the field log.message was renamed to log.original.

@ruflin ruflin added the review label Aug 28, 2018
log.message was confused with message. To bring it in line with elastic#102 the field log.message was renamed to log.original.
@ruflin ruflin force-pushed the log.message-to-log.original branch from c306a6d to 5547ed7 Compare August 28, 2018 11:08
@webmat
Copy link
Contributor

webmat commented Aug 28, 2018

Love this wording instead of log.message indeed. It's less ambiguous.

schemas/log.yml Outdated
type: keyword
phase: 1
example: "Sep 19 08:26:10 localhost My log"
index: false
# TODO: Best would be to disable ignore_above completely
ignore_above: 32766
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm hesitant about the value of ignore_above, however. ignore_above does not truncate the value itself, it's a safety valve to avoid indexing values that are unexpectedly long in this field.

In other words, I don't think we would be likely to do exact match search or aggregations on an original log message that's this long. Log messages that are really short would be much more likely to be used this way, and as such I think 1024 would be enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree with that. Removed the ignore_above.

@@ -21,15 +21,16 @@
description: >
Offset of the beginning of the log event.
example: 12
- name: message
- name: original
type: keyword
phase: 1
example: "Sep 19 08:26:10 localhost My log"
index: false
Copy link
Member Author

Choose a reason for hiding this comment

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

@webmat As we have index: false ignore_above should not play a role anyways?

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

LGTM

Not familiar with index:false, but I assume you're right, it would not have mattered. But actually, are you sure we want this to be index:false? I guess so: aggregating on short log messages is kind of an edge case...

Do we go with the new PR merging policies of Beats for this? Or do you want me to merge it?

@ruflin
Copy link
Member Author

ruflin commented Aug 29, 2018

I don't expect this field to be used for aggregation at all.

I would prefer to keep here the merging as we had, so please merge :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants