Refactor input.Event similar to outputs.Data #3823

vjsamuel · 2017-03-28T08:21:23Z

Kindly review this change as this is a feature that we're looking for when we use filebeat :) If the code looks good, I can also fix the test cases.

#3785

cc: @ruflin

elasticmachine · 2017-03-28T08:22:02Z

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

elasticmachine · 2017-03-28T08:22:07Z

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

ruflin · 2017-03-28T08:23:30Z

filebeat/f.yml

@@ -0,0 +1,143 @@
+###################### Filebeat Configuration Example #########################


that was probably commited by accident. You can use filebeat.dev.yml and it will be ignored automatically by gitignore.

removed it. thanks for the heads up. i ll use that.

ruflin

In general I like the change and I think it goes in the right direction.

One thing I worry is that we introduce now fields in the common.MapStr which could conflict with fields that are already in there.

The other part I have to check in more detail is if all the state updates still work as expected. Based on the code I get the impression that states are not always updated.

ruflin · 2017-03-28T08:24:38Z

filebeat/prospector/config.go

@@ -43,6 +44,7 @@ type prospectorConfig struct {
 	Pipeline             string             `config:"pipeline"`
 	Module               string             `config:"_module_name"`  // hidden option to set the module name
 	Fileset              string             `config:"_fileset_name"` // hidden option to set the fileset name
+	Filters              processors.PluginConfig `config:"filters"`


Probably we should call it processors, as all of them will be available. It's already causing confusion on the metricbeat side ...

ha ha! let me do that.

ruflin · 2017-03-28T08:26:07Z

filebeat/prospector/prospector.go

+
+	//run the filters before sending to
+	eventMap = p.filters.Run(eventMap)
+	if eventMap != nil {


Assuming the event was "removed" by the filter, this would probably mean no state update is sent anymore? Or when exactly is eventMap empty?

eventMap would be empty in case of drop_event. But i did see that the file was traversed till the end:

2017/03/28 08:17:17.442705 log.go:114: INFO Reader was closed: /var/log/alf.log. Closing. 2017/03/28 08:17:17.443151 log.go:306: DBG Stopping harvester for file: /var/log/alf.log 2017/03/28 08:17:17.443163 log.go:315: DBG Closing file: /var/log/alf.log 2017/03/28 08:17:17.443169 log.go:183: DBG Update state: /var/log/alf.log, offset: 0 2017/03/28 08:17:17.443130 log.go:183: DBG Update state: /var/log/displaypolicyd.stdout.log, offset: 1155 2017/03/28 08:17:17.443030 log.go:315: DBG Closing file: /var/log/jamf.log 2017/03/28 08:17:17.443198 log.go:183: DBG Update state: /var/log/jamf.log, offset: 1190762

But since these events do goto registrar, the registry file wouldn't be updated. But this is a scenario where I configure my prospector with:

filters: drop_event:

In this case, no event would pass upto registrar.

… a holder obj

ruflin · 2017-03-29T07:18:36Z

jenkins, test it

ruflin

Thanks a lot for the contribution. I had again a closer look and left some minor comments.

For the PR I suggest to split it up in 2 parts:

PR 1: All changes except the few lines needed to add the processor. This is pure refactoring / setting the foundation for the processor
PR 2: Add processors with changelog entry and system tests. We should add the tests directly with the same PR to make sure the new feature we introduce works as expected. This also makes backporting it easier.

We can add the docs for it in a third step.

@urso Would be nice to get your feedback on this one too.

ruflin · 2017-03-29T09:35:13Z

filebeat/input/event.go

+	State    file.State
+}
+
+type EventHolder struct {


Can we rename EventHolder to Data to be consistent with https://github.com/elastic/beats/blob/master/libbeat/outputs/outputs.go#L20 ? I'm ok with keeping Metadata as here it is at the moment only about Metadata

ruflin · 2017-03-29T09:35:47Z

filebeat/input/event.go

 	Pipeline     string
 	Fileset      string
 	Module       string
+	InputType    string
+	DocumentType string
+	common.EventMetadata


lets move the common.EventMetadata to the the first line of EventMeta

ruflin · 2017-03-29T09:38:12Z

filebeat/input/event.go

-	JSONConfig   *reader.JSONConfig
-	State        file.State
-	Data         common.MapStr // Use in readers to add data to the event
+	EventMeta


So currently we have EventMeta twice, means two copies of the data? What would be the affect if we not have it in the Event? I think we will need in the future processors to have access to the meta data too, but do we need this in the first step?

Event will be used in harvesters and when it reaches the processor, we do a event.GetEventHolder(), which would (you are right), copy the metadata from Event to EventHolder and generate the MapStr. But after that since Event object goes out of scope, it would be garbage collected if I am not wrong. Would that be an issue? Today when we generate the common.MapStr, we copy all the log related fields over. Now with this change, we additionally copy over the meta fields as well. Please correct me if I'm wrong.

ruflin · 2017-03-29T09:40:52Z

filebeat/prospector/prospector.go

+	var ok bool
+	if eventHolder.Event != nil {
+		//processor might decide to drop the event
+		ok = p.outlet.OnEvent(&eventHolder)


Lets move the ok = p.outlet.OnEvent(&eventHolder) after the if / else and only have if eventHolder.Event == nil { Bytes = 0} . This should be the same logic. Then we also don't need the definition of ok.

will add this back in the next PR. removed it for now.

vjsamuel · 2017-03-29T14:45:02Z

@ruflin, i have removed the processor code and renamed the object from EventHolder to Data.

Also do I submit the PR for processor code immediately or do I wait for this to be approved and merged?

ruflin · 2017-03-30T07:23:55Z

jenkins, test it

vjsamuel · 2017-03-30T08:11:22Z

thank you! :)

vjsamuel force-pushed the prospector_level_filters branch from 9f0a1dd to f30822b Compare March 28, 2017 08:22

ruflin reviewed Mar 28, 2017

View reviewed changes

Adding support for filebeat prospector level filters

d63d93b

vjsamuel force-pushed the prospector_level_filters branch from f30822b to d63d93b Compare March 28, 2017 08:25

ruflin reviewed Mar 28, 2017

View reviewed changes

ruflin added Filebeat Filebeat in progress Pull request is currently in progress. labels Mar 28, 2017

vjsamuel changed the title ~~Prospector level filters~~ Prospector level processors Mar 29, 2017

vjsamuel added 2 commits March 28, 2017 23:17

Incorporating review comments and moving common.MapStr, metadata into…

dd89989

… a holder obj

Updating test cases and gofmt for prospector level processors

c78db55

vjsamuel force-pushed the prospector_level_filters branch from d98b53f to c78db55 Compare March 29, 2017 06:43

ruflin mentioned this pull request Mar 29, 2017

Filebeat Prospector Processors Improvements #3837

Closed

3 tasks

ruflin added review and removed in progress Pull request is currently in progress. labels Mar 29, 2017

ruflin reviewed Mar 29, 2017

View reviewed changes

Renaming EventHolder to Data

3a620e6

vjsamuel force-pushed the prospector_level_filters branch from ca55dc4 to 3a620e6 Compare March 29, 2017 14:44

vjsamuel changed the title ~~Prospector level processors~~ Refactor input.Event similar to outputs.Data Mar 29, 2017

Changing name of GetEventHolder to GetData

33c9e1f

ruflin approved these changes Mar 30, 2017

View reviewed changes

ruflin merged commit 1c2d335 into elastic:master Mar 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor input.Event similar to outputs.Data #3823

Refactor input.Event similar to outputs.Data #3823

vjsamuel commented Mar 28, 2017 •

edited

Loading

elasticmachine commented Mar 28, 2017

elasticmachine commented Mar 28, 2017

ruflin Mar 28, 2017

vjsamuel Mar 28, 2017

ruflin left a comment

ruflin Mar 28, 2017

vjsamuel Mar 28, 2017

ruflin Mar 28, 2017

vjsamuel Mar 28, 2017

ruflin commented Mar 29, 2017

ruflin left a comment

ruflin Mar 29, 2017

ruflin Mar 29, 2017

vjsamuel Mar 29, 2017

ruflin Mar 29, 2017

vjsamuel Mar 29, 2017

ruflin Mar 29, 2017

vjsamuel Mar 29, 2017

vjsamuel commented Mar 29, 2017

ruflin commented Mar 30, 2017

vjsamuel commented Mar 30, 2017

		@@ -0,0 +1,143 @@
		###################### Filebeat Configuration Example #########################

Refactor input.Event similar to outputs.Data #3823

Refactor input.Event similar to outputs.Data #3823

Conversation

vjsamuel commented Mar 28, 2017 • edited Loading

elasticmachine commented Mar 28, 2017

elasticmachine commented Mar 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin commented Mar 29, 2017

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vjsamuel commented Mar 29, 2017

ruflin commented Mar 30, 2017

vjsamuel commented Mar 30, 2017

vjsamuel commented Mar 28, 2017 •

edited

Loading