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

Add Ingest pipeline loading to setup #6814

Merged
merged 8 commits into from
Apr 11, 2018

Conversation

kvch
Copy link
Contributor

@kvch kvch commented Apr 10, 2018

1. Update pipeline config option

The yet unreleased flag --update-pipelines is removed. Instead of this a new config option is introduced which does the same thing. Its name is filebeat.overwrite_pipelines. By default it is set to false.

# By default Ingest pipelines are not updated. If this option is enabled Filebeat updates
# pipelines everytime a new Elasticsearch connection is established.
filebeat.update_pipelines: false

2. Load pipelines using setup

Furthermore, it's possible to load Ingest pipelines using the command setup. Please note, only configured pipelines are loaded. To make sure users don't lose events pipeline loading before sending messages is kept.

Closes #6808

@kvch kvch added in progress Pull request is currently in progress. Filebeat Filebeat labels Apr 10, 2018
@@ -55,3 +56,7 @@ type BeatConfig struct {
// SetupMLCallback can be used by the Beat to register MachineLearning configurations
// for the enabled modules.
type SetupMLCallback func(*Beat, *common.Config) error

// SetupMLCallback can be used by the Beat to register MachineLearning configurations

Choose a reason for hiding this comment

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

comment on exported type UpdatePipelinesCallback should be of the form "UpdatePipelinesCallback ..." (with optional leading article)

"github.com/elastic/beats/libbeat/logp"
)

// PipelineLoader factory builds and returns a PipelineLoader

Choose a reason for hiding this comment

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

comment on exported type PipelineLoaderFactory should be of the form "PipelineLoaderFactory ..." (with optional leading article)

var result string
for module, filesets := range reg.registry {
var filesetNames string
for name, _ := range filesets {

Choose a reason for hiding this comment

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

should omit 2nd value from range; this loop is equivalent to for name := range ...

@kvch kvch force-pushed the fix/filebeat/move-pipeline-loading-to-setup branch 2 times, most recently from 43af07b to 32da1d8 Compare April 10, 2018 13:08
@kvch kvch changed the title [WIP] Move pipeline loading to setup Add Ingest pipeline loading to setup Apr 10, 2018
@kvch kvch added review and removed in progress Pull request is currently in progress. labels Apr 10, 2018
@kvch kvch force-pushed the fix/filebeat/move-pipeline-loading-to-setup branch from f568a5b to c5081f6 Compare April 10, 2018 13:22
@ruflin
Copy link
Member

ruflin commented Apr 10, 2018

  • Pipelines are not only loaded on the first start but also reconnect it seems. I like that as it's the same behaviour as templates for example. Also by default we don't seem to overwrite it. So if a pipeline with the same id already exists, nothing happens.
  • Should we call it overwrite instead of update or forceUpdate because that is what it does?

@@ -286,6 +286,10 @@ filebeat.inputs:
# This option is not supported on Windows.
#filebeat.registry_file_permissions: 0600

# By default Ingest pipelines are not updated. If this option is enabled Filebeat updates
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe clarify to something like: "By default Ingest pipelines are not updated if a pipeline with the same ID already exists."

@kvch
Copy link
Contributor Author

kvch commented Apr 10, 2018

@ruflin You got me. Overwrite does feel a bit more descriptive than update does. I am renaming it.

@kvch
Copy link
Contributor Author

kvch commented Apr 10, 2018

Renamed and edited the PR comment.

@kvch kvch force-pushed the fix/filebeat/move-pipeline-loading-to-setup branch from 2624215 to 6a151b5 Compare April 10, 2018 17:43
@kvch
Copy link
Contributor Author

kvch commented Apr 10, 2018

The failing tests are unrelated.

@urso
Copy link

urso commented Apr 10, 2018

We already have a setup namespace in our configs for templates and dashboards. How about moving the pipeline update setting into this namespace as well?

@ruflin
Copy link
Member

ruflin commented Apr 11, 2018

@urso I was thinking the same at first again but then was not sure as so far it is only a filebeat feature. I expect us to have it as a global feature soon as other beats will also potentially have pipelines. I'm ok with merging as is as the config option will not potentially conflict with other option, should be fairly easy to migrate to something else and all the code is currently in Filebeat and not libbeat.

@kvch
Copy link
Contributor Author

kvch commented Apr 11, 2018

@urso As I understand the role of Ingest pipelines in Beats, they are tied to Filebeat. From architectural point of view I don't think it's a good idea to move the config option to instance.Beat, because it is a Filebeat specific config option.
Also, setup namespace could imply that it has something to do with the command setup. But it's not true. When users call setup this option is doesn't change anything, pipelines are overwritten. This flag only changes the behaviour of Filebeat when run is called.

On the other hand I see that from users' point of view it might be more clear to put it under setup namespace.

@urso
Copy link

urso commented Apr 11, 2018

@ruflin changing a config options name is never an easy change, as it's a BC breaking. As you noticed, I wonder if we will ever have ingest node pipelines for use by other beats. Besides Ingest Node pipelines, I wonder if we will ever have Logstash filter configs in beats to be configured by any beat.

But I'm fine with merging the PR as is.

@urso urso merged commit 2619e13 into elastic:master Apr 11, 2018
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.

5 participants