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

Make adding the hostname to the event optional #340

Closed
wants to merge 4 commits into from

Conversation

robbavey
Copy link
Contributor

@robbavey robbavey commented Jul 9, 2018

Add option to make the population of the 'host' field in the event with
the value from beat.hostname optional. This field is intentionally added
as a deprecated field, as the this field will be removed in future versions
of the beats input, when the ability to populate host field in this way
will be removed altogether.

@robbavey robbavey force-pushed the host_deprecate branch 2 times, most recently from a887c01 to 674d6e5 Compare July 9, 2018 21:47
@robbavey robbavey self-assigned this Jul 16, 2018
Add `add_hostname` flag - when enabled, the `host` field will be populated
by the value supplied by the beat in the `hostname` field if that field is present.

The flag defaults to true to retain backwards compatibility with earlier versions of
the beats input.

The field is intentionally added as a deprecated field - auto populating the `host` field in
this way will be removed in future versions of the beats input.
@@ -82,6 +82,10 @@ class LogStash::Inputs::Beats < LogStash::Inputs::Base
#
config :ssl_certificate_authorities, :validate => :array, :default => []

# Flag to determine whether to add host information (provided by the beat in the 'hostname' field) to the event
config :add_hostname, :validate => :boolean, :default => true, :deprecated => 'Host field will not be automatically populated by future version of the Beats input'

Choose a reason for hiding this comment

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

Doesn't the default value of true conflict with the documented default value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gah, you're right. Good catch, will change

Copy link

@danhermann danhermann left a comment

Choose a reason for hiding this comment

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

LGTM

And changelog update.
@robbavey robbavey requested a review from karenzone July 23, 2018 19:50
@robbavey
Copy link
Contributor Author

@karenzone Would you mind checking the doc changes?

Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

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

Builds cleanly without errors. I left a couple of comments with regards to wording. Otherwise, LGTM

[id="plugins-{type}s-{plugin}-add_hostname"]
===== `add_hostname`

added[5.2.0, Field was added to allow users to not automatically add the `host` field in events.]
Copy link
Contributor

Choose a reason for hiding this comment

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

What about "Field was added to allow users to control whether or not the host field is automatically added to events."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 - I like that

===== `add_hostname`

added[5.2.0, Field was added to allow users to not automatically add the `host` field in events.]
deprecated[5.2.0, In future versions of this plugin, this setting will be removed, and the 'hosts' field will not be added to events.]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we say deprecated[5.2.1 if the field will be removed for future releases?
To summarize suggestion, "Added in 5.2.0. Deprecated in 5.2.1"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intent is to deprecate it immediately - the setting is a stop gap until we make the breaking change that will remove the setting.

Improved wording and fixed up versions for added and deprecated sections
Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

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

LGTM

@elasticsearch-bot
Copy link

Rob Bavey merged this into the following branches!

Branch Commits
master 737f5c1, 3242487, 861a833, 87051f0

elasticsearch-bot pushed a commit that referenced this pull request Jul 24, 2018
elasticsearch-bot pushed a commit that referenced this pull request Jul 24, 2018
And changelog update.

Fixes #340
elasticsearch-bot pushed a commit that referenced this pull request Jul 24, 2018
Improved wording and fixed up versions for added and deprecated sections

Fixes #340
fbacchella pushed a commit to fbacchella/netty-beats that referenced this pull request Aug 12, 2018
Add `add_hostname` flag - when enabled, the `host` field will be populated
by the value supplied by the beat in the `hostname` field if that field is present.

The flag defaults to true to retain backwards compatibility with earlier versions of
the beats input.

The field is intentionally added as a deprecated field - auto populating the `host` field in
this way will be removed in future versions of the beats input.

Fixes logstash-plugins#340
fbacchella pushed a commit to fbacchella/netty-beats that referenced this pull request Aug 12, 2018
fbacchella pushed a commit to fbacchella/netty-beats that referenced this pull request Aug 12, 2018
And changelog update.

Fixes logstash-plugins#340
fbacchella pushed a commit to fbacchella/netty-beats that referenced this pull request Aug 12, 2018
Improved wording and fixed up versions for added and deprecated sections

Fixes logstash-plugins#340
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.

4 participants