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

Don't stop Filebeat when modules + logstash are used together #3929

Merged
merged 1 commit into from Apr 10, 2017

Conversation

tsg
Copy link
Contributor

@tsg tsg commented Apr 5, 2017

To simplify the migration from Ingest Node to Logstash, we replace the
hard error ("Filebeat modules require an Elasticsearch output defined") with
a warning.

@tsg tsg added needs_backport PR is waiting to be backported to other branches. review v5.4.0 labels Apr 5, 2017
@tsg tsg requested a review from dedemorton April 5, 2017 22:24
@tsg
Copy link
Contributor Author

tsg commented Apr 5, 2017

@dedemorton This is the PR to make FBM continue if only the Logstash output is configured, with a warning. Can you review the warn message? I was thinking we could even link to a longer explanation in the docs.

@@ -75,7 +75,10 @@ func New(b *beat.Beat, rawConfig *common.Config) (beat.Beater, error) {
func (fb *Filebeat) modulesSetup(b *beat.Beat) error {
esConfig := b.Config.Output["elasticsearch"]
if esConfig == nil || !esConfig.Enabled() {
return fmt.Errorf("Filebeat modules configured but the Elasticsearch output is not configured/enabled")
logp.Warn("Filebeat modules enabled but the Elasticsearch output is not configured/enabled." +
" This means I can't load the Ingest Node pipelines." +
Copy link
Member

Choose a reason for hiding this comment

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

I stumbled over the I and would replace it by Filebeat

@tsg tsg force-pushed the no_error_on_fbm_and_logstash branch from daf41b7 to 061174c Compare April 6, 2017 08:47
@ruflin
Copy link
Member

ruflin commented Apr 6, 2017

jenkins, retest it

Copy link
Contributor

@dedemorton dedemorton left a comment

Choose a reason for hiding this comment

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

LGTM - see commment

@@ -75,7 +75,10 @@ func New(b *beat.Beat, rawConfig *common.Config) (beat.Beater, error) {
func (fb *Filebeat) modulesSetup(b *beat.Beat) error {
esConfig := b.Config.Output["elasticsearch"]
if esConfig == nil || !esConfig.Enabled() {
return fmt.Errorf("Filebeat modules configured but the Elasticsearch output is not configured/enabled")
Copy link
Contributor

@dedemorton dedemorton Apr 6, 2017

Choose a reason for hiding this comment

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

LGTM (couple of minor changes)

Filebeat modules are enabled, but the Elasticsearch output is not configured/enabled. This means Filebeat can't load the Ingest Node pipelines. If you have already loaded the pipelines, you can ignore this warning.

Not sure if this covers LS users, tho, because they really don't need to load the pipelines at all, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

An alternate version (in case you want to shift the focus away from the idea that ES is required):

Filebeat is unable to load the Ingest Node pipelines for the configured modules because the Elasticsearch output is not configured/enabled. If you have already loaded the Ingest Node pipelines or are using Logstash pipelines, you can ignore this warning.

To simplify the migration from Ingest Node to Logstash, we replace the
hard error ("Filebeat modules require an Elasticsearch output defined") with
a warning.
@tsg tsg force-pushed the no_error_on_fbm_and_logstash branch from 061174c to 890ef36 Compare April 10, 2017 07:41
@tsg
Copy link
Contributor Author

tsg commented Apr 10, 2017

@dedemorton thanks, i used your second version.

@monicasarbu monicasarbu merged commit 12c69b3 into elastic:master Apr 10, 2017
@monicasarbu monicasarbu added the Filebeat Filebeat label Apr 10, 2017
tsg added a commit to tsg/beats that referenced this pull request Apr 12, 2017
…c#3929)

To simplify the migration from Ingest Node to Logstash, we replace the
hard error ("Filebeat modules require an Elasticsearch output defined") with
a warning.
(cherry picked from commit 12c69b3)
@tsg tsg removed the needs_backport PR is waiting to be backported to other branches. label Apr 12, 2017
ruflin pushed a commit that referenced this pull request Apr 12, 2017
…#3999)

To simplify the migration from Ingest Node to Logstash, we replace the
hard error ("Filebeat modules require an Elasticsearch output defined") with
a warning.
(cherry picked from commit 12c69b3)
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

4 participants