filebeat: expand double wildcards in prospector #3980

7AC · 2017-04-10T19:29:33Z

Expand double wildcards into standard glob patterns, up to a maximum
depth of 16 levels after the wildcard.

Resolves #2084

tsg · 2017-04-10T20:11:54Z

filebeat/prospector/prospector_log.go

+// glob detects the use of "**" and expands it to standard glob patterns up to a max depth
+func glob(g string) ([]string, error) {
+	var globs []string
+	if filepath.Base(g) == "**" {


Does this mean it only detects ** at the end of the path? Does /foo/**/bar work?

Yeah, I didn't realize that was also a use case. Let me fix it, thanks

tsg · 2017-04-10T21:51:18Z

I'm 👍 with this approach, since I think it's the simplest implementation that gets us what most people want. I'm on the fence about making the 16 configurable or not, but I guess it would be good to be able to set it to 0 in which case the feature is disabled (for BWC).

ruflin

I like the approach that by default we have the old behaviour.

This PR should also have changelog entry and docs.

ruflin · 2017-04-11T07:35:28Z

filebeat/prospector/prospector_log.go

@@ -112,17 +114,69 @@ func (l *Log) Run() {
 	}
 }

+// globPattern detects the use of "**" and expands it to standard glob patterns up to a max depth
+func (l *Log) globPatterns(pattern string) ([]string, error) {


Perhaps we could move this to a separate package and pass the max depth to the function. Reason is like this we could also use it in other places and it does not further "bloat" the prospector code.

Yeah I like that

ruflin · 2017-04-11T07:38:50Z

filebeat/prospector/config.go

+	Module                 string                  `config:"_module_name"`  // hidden option to set the module name
+	Fileset                string                  `config:"_fileset_name"` // hidden option to set the fileset name
+	Processors             processors.PluginConfig `config:"processors"`
+	DoubleWildcardMaxDepth uint8                   `config:"double_wildcard_max_depth"`


Where is the default set for this one?

The config name seems to be a bit long

I would like to have this feature disabled by default and people who need it enable it specifically. This also makes it easier for us to debug it in the future in case of errors, because we know someone enabled it.

I like the idea of defining the max depth as in case someone just needs one or two levels, this would have positive performance implications.

I just assumed the golang type default (0) is used as fallback? Totally agree on the config name: how about double_star_pattern_depth? Maybe not shorter but simpler..

ruflin · 2017-04-11T07:41:26Z

filebeat/prospector/prospector_log.go

+	patternList := strings.Split(pattern, "/")
+	for i, dir := range patternList {
+		if len(patterns) > 0 {
+			if dir == "**" {


That means test/**/hello/**/abc.log is not going to work?

Yes, we were discussing with @tsg and if we allow that the complexity will escalate quickly

ruflin · 2017-04-11T07:43:17Z

filebeat/prospector/prospector_log_test.go

+		pattern         string
+		expectedMatches []string
+	}{
+		{


Could you add tests which have a deeper depth like 4 or 5?

urso · 2017-04-11T09:17:17Z

filebeat/prospector/prospector_log.go

+	}
+	var patterns []string
+	isAbs := filepath.IsAbs(pattern)
+	patternList := strings.Split(pattern, "/")


what about windows users having \ in their path?

Good catch, I should use os.PathSeparator instead

urso · 2017-04-11T09:18:27Z

filebeat/prospector/prospector_log.go

+		}
+	}
+	if len(patterns) == 0 {
+		patterns = []string{pattern}


we have any kind of error if pattern is empty?

Let me check on that, I guess we should

Ok so right now we're not doing any validation on the pattern, with this change that wouldn't change wrt an empty pattern. Do we want to start validating those?

I'd say no, since for example, I use "" in the filebeat modules to disable a fileset on a particular version.

urso · 2017-04-11T09:21:44Z

filebeat/prospector/prospector_log_test.go

+		{
+			"foo/**/bazz",
+			[]string{},
+		},


some more tests to add:

double / like foo//bar

foo\bar

foo\**\bar

c:\foo\**\bar

I couldn't add the last test since I end up joining all the paths to a root that has a volume in it on windows, via ioutil.TempDir()

I created a new test just for cases like this

andrewkroh · 2017-04-12T15:04:06Z

filebeat/input/file/glob.go

+		if len(patterns) > 0 {
+			if dir == "**" {
+				err := fmt.Sprintf("glob(%s) failed: cannot specify multiple ** within a pattern", pattern)
+				logp.Err(err)


I don't recommend logging errors and returning them. Do one or the other. It's probably best to let the caller make the decision on how to handle the error (which might be to log it).

andrewkroh · 2017-04-12T15:10:53Z

filebeat/docs/reference/configuration/filebeat-options.asciidoc

+===== double_star_pattern_depth
+
+The maximum depth for `**` patterns. This setting is disabled by default, i.e.,
+`**` is not expanded. When it's set to a positive value, a single `**` per


Why the limit in single ** per path? With a bit of recursion in the expansion logic I think this could be handled elegantly.

What worried me and @tsg was how the complexity could easily escalate here

I was also worried about the number of generated globs. With a max depth of 16, and using ** twice, you need to generate 16*16 patterns, right? If you use ** three times that's already 4096 globs to evaluate, which might be problematic.

That is a bit mitigated by the fact that we default to zero (disabled) now, but I'd still wait for an actual request/use case for it before opening up to multiple **.

@7AC See my comment above. I'm ok with having it fixed at first but that we take into account now, it could appear in the future.

urso · 2017-04-13T09:43:34Z

filebeat/input/file/glob.go

+	}
+	var patterns []string
+	isAbs := filepath.IsAbs(pattern)
+	patternList := strings.Split(pattern, string(os.PathSeparator))


os.PathSeparator might not be safe. windows users sometimes use / for paths (if they didn't get \ working due to not using single-quote), which is working with go as well.

Ah interesting, didn't know that. So the windows case is basically a superset, let me handle that too.

ruflin · 2017-04-18T13:54:41Z

filebeat/_meta/common.full.p2.yml

@@ -82,6 +82,11 @@ filebeat.prospectors:
  # This is especially useful for multiline log messages which can get large.
  #max_bytes: 10485760

+  # Maximum depth for "**" patterns. This setting is disabled by default, i.e.,


I somehow expect we will add more config options in the future. So I would suggest to make this a namespace with different config options below. Suggestion:

recursive_glob.enabled: false # set this to true to get the defaults for the below options recursive_glob.depth: 16 # max depth of expanding ** recursive_glob.max_count: 1 # max number of ** in a single path

This could also solve the discussion about how many patterns we allow per path.

I named it recursive_glob because that what its doing from my perspective. Not sure what the user would look for when searching for such a config option. I had also recursive_path because we call it paths in the config above.

I renamed to recursive_glob_depth for now, I don't think we should encourage users to use more than one per path so the namespace seems overkill to me.

ruflin · 2017-04-18T13:57:55Z

filebeat/prospector/config.go

+		DocumentType:  "log",
+		ScanFrequency: 10 * time.Second,
+		InputType:     cfg.DefaultInputType,
+		CleanRemoved:  true,


What happened to all the other default configs?

I removed all the ones that have default values for the golang type (including the new one)

I strongly prefer to have all values in here, even if they are the golang defaults. This makes it very easy (and future proof) to lookup what our defaults are without having to think about what the default in Golang is.

ruflin · 2017-04-24T09:14:57Z

filebeat/filebeat.full.yml

@@ -211,6 +211,10 @@ filebeat.prospectors:
  # This is especially useful for multiline log messages which can get large.
  #max_bytes: 10485760

+  # Maximum depth for "**" patterns. When it's set to a positive value, "**"
+  # patterns are expanded up to the specified depth.
+  #recursive_glob_depth: 16


Assuming we don't want to make it configurable at first, could we go with recursive_glob.enabled as config option instead? This will allow us to extend it in the future if needed. I'm aware that just recursive_glob: true would feel more natural at first, but this will prevent us from using it as a namespace in the future.

Sounds good

ruflin · 2017-04-24T19:00:32Z

filebeat/filebeat.full.yml

+  ### Recursive glob configuration
+
+  # Expand "**" patterns into regular glob patterns.
+  #recursive_glob.enabled: true


We normally have the default value in the full config, which is false.

ruflin · 2017-04-24T19:01:56Z

filebeat/docs/reference/configuration/filebeat-options.asciidoc


 Filebeat starts a harvester for each file that it finds under the specified
 paths. You can specify one path per line. Each line begins with a dash (-).

+===== recursive_glob
+
+*`enabled`*:: Enable expanding `**` into recursive glob patterns. With this feature enabled,


Should we mark this beta first to get some users to test it first? At the same time it rolls out with the 6 alpha release, so we get already some testing.

How do I do that?

You can add []beta and it gets expanded with a message automatically in the docs. But lets leave it out. We can still add it.

ruflin · 2017-04-24T19:07:08Z

filebeat/prospector/prospector_log.go

-		matches, err := filepath.Glob(glob)
+	for _, path := range l.config.Paths {
+		depth := uint8(0)
+		if l.config.recursiveGlob.enabled {


I expected to see here recursiveGlob.Enabled() as this come with ucfg for each "object". Problem is that if we want that we would have to do unpack each time the scan runs which is not a good idea. So this looks good to me.

ruflin · 2017-04-24T19:08:18Z

filebeat/prospector/config.go

@@ -45,6 +46,11 @@ type prospectorConfig struct {
 	Module               string                  `config:"_module_name"`  // hidden option to set the module name
 	Fileset              string                  `config:"_fileset_name"` // hidden option to set the fileset name
 	Processors           processors.PluginConfig `config:"processors"`
+	recursiveGlob        *recursiveGlobConfig    `config:"recursive_glob"`


why not

recursiveGlob bool `config:"recursive_glob.enabled" `

ruflin

LGTM. Could you add a CHANGELOG entry, squash the commits and add a commit message?

Expand double wildcards into standard glob patterns, up to a maximum depth of 8 levels after the wildcard. Resolves #2084

dedemorton · 2017-04-25T16:18:25Z

filebeat/docs/reference/configuration/filebeat-options.asciidoc


 Filebeat starts a harvester for each file that it finds under the specified
 paths. You can specify one path per line. Each line begins with a dash (-).

+===== recursive_glob
+
+*`enabled`*:: Enable expanding `**` into recursive glob patterns. With this feature enabled,


@7AC This won't render correctly in asciidoc because it will try to evaluate the asterisks before the backticks. Wherever you have asterisks in an example, use plus signs (+) instead of backticks. For example: +**+. (So replace the backticks with plus signs in the examples.)

fixed it thank you!

dedemorton · 2017-04-25T16:39:38Z

filebeat/docs/reference/configuration/filebeat-options.asciidoc


 Filebeat starts a harvester for each file that it finds under the specified
 paths. You can specify one path per line. Each line begins with a dash (-).

+===== recursive_glob
+
+*`enabled`*:: Enable expanding +**+ into recursive glob patterns. With this feature enabled,


@7AC I think my explanation of the behavior of plus signs was a little off in my previous comment. The backticks are OK for the doc build, but the content doesn't render for users who preview the file in GitHub. We aren't optimizing for viewing in GitHub, though, so technically you can use the backticks. But you should be consistent in this entire description (either use backticks or plus signs for any example that contains asterisks in the path). The asterisk doesn't get evaluated either way.

dedemorton · 2017-04-25T16:41:23Z

filebeat/docs/reference/configuration/filebeat-options.asciidoc


 Filebeat starts a harvester for each file that it finds under the specified
 paths. You can specify one path per line. Each line begins with a dash (-).

+===== recursive_glob


@7AC You need to add the section ID above this section or the doc build will fail. So add this to line 50:

[[recursive_glob]]

…card

mbhuie · 2017-10-24T22:54:58Z

Is this available in 5.6.3 release?

7AC added the review label Apr 10, 2017

tsg reviewed Apr 10, 2017

View reviewed changes

ruflin added Filebeat Filebeat in progress Pull request is currently in progress. labels Apr 11, 2017

ruflin requested changes Apr 11, 2017

View reviewed changes

urso reviewed Apr 11, 2017

View reviewed changes

andrewkroh reviewed Apr 12, 2017

View reviewed changes

urso reviewed Apr 13, 2017

View reviewed changes

ruflin reviewed Apr 18, 2017

View reviewed changes

ruflin reviewed Apr 24, 2017

View reviewed changes

ruflin approved these changes Apr 25, 2017

View reviewed changes

filebeat: expand double wildcards in prospector

c5edf00

Expand double wildcards into standard glob patterns, up to a maximum depth of 8 levels after the wildcard. Resolves #2084

dedemorton reviewed Apr 25, 2017

View reviewed changes

Giuseppe Valente and others added 2 commits April 25, 2017 09:43

Fix doc formatting

11d102c

Merge branch 'master' into fix/filebeat_prospector_expand_double_wild…

451eaac

…card

ruflin merged commit aaee997 into elastic:master Apr 26, 2017

ruflin mentioned this pull request Apr 26, 2017

filebeat wildcard for directories #2084

Closed

filebeat: expand double wildcards in prospector #3980

filebeat: expand double wildcards in prospector #3980

Conversation

7AC commented Apr 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsg commented Apr 10, 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

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

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

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

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

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 left a comment

Choose a reason for hiding this comment

dedemorton Apr 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dedemorton Apr 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbhuie commented Oct 24, 2017

dedemorton Apr 25, 2017 •

edited

Loading

dedemorton Apr 25, 2017 •

edited

Loading