Add ability to configure autodiscover with JSON hints #7372
Conversation
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
| @@ -64,6 +65,16 @@ func getStringAsList(input string) []string { | |||
| return list | |||
| } | |||
|
|
|||
| func GetHintAsConfig(hints common.MapStr, key, config string) []common.MapStr { | |||
There was a problem hiding this comment.
exported function GetHintAsConfig should have comment or be unexported
| @@ -64,6 +65,16 @@ func getStringAsList(input string) []string { | |||
| return list | |||
| } | |||
|
|
|||
| func GetHintAsConfig(hints common.MapStr, key, config string) []common.MapStr { | |||
There was a problem hiding this comment.
exported function GetHintAsConfig should have comment or be unexported
ruflin
added
module
review
Filebeat
| func GetHintAsConfig(hints common.MapStr, key, config string) []common.MapStr { | ||
| if str := GetHintString(hints, key, config); str != "" { | ||
| cfg := []common.MapStr{} | ||
| if err := json.Unmarshal([]byte(str), &cfg); err == nil { |
There was a problem hiding this comment.
Should we have a debug log when there is an error? like this we could also use err != nil and then a continue which is what I expected first when I read the code.
There was a problem hiding this comment.
Thank you for this code @vjsamuel! This is looking great!
I have some doubts about the annotation name, what do you think about using justco.elastic.logs and co.elastic.metrics? Maybe co.elastic.logs.raw... I'm not sure.
That would make clear that you are overriding any other possible hint. Also, we need to avoid the word inputs, as Filebeat modules would also work with this code.
I have some concerns around security, as this effectively allows users to spawn any conf. So far current hints are mostly for untrusted users. any opinions here?
Docs need to be updated (https://github.com/elastic/beats/blob/master/metricbeat/docs/autodiscover-hints.asciidoc and https://github.com/elastic/beats/blob/master/filebeat/docs/autodiscover-hints.asciidoc)
| @@ -64,6 +66,19 @@ func getStringAsList(input string) []string { | |||
| return list | |||
| } | |||
|
|
|||
| // GetHintAsConfig can read a hint in the form of a stringified JSON and return a common.MapStr | |||
| func GetHintAsConfig(hints common.MapStr, key, config string) []common.MapStr { | |||
There was a problem hiding this comment.
this should probably be GetHintAsConfigs, as it returns a list
| // GetHintAsConfig can read a hint in the form of a stringified JSON and return a common.MapStr | ||
| func GetHintAsConfig(hints common.MapStr, key, config string) []common.MapStr { | ||
| if str := GetHintString(hints, key, config); str != "" { | ||
| cfg := []common.MapStr{} |
There was a problem hiding this comment.
Would it make sense to accept both a list or a single config {...}? I guess in most cases the user wants just a config, with some special exceptions where you may want more
Allow filebeat to configure an array of inputs using
co.elastic.logs/inputs: [stringified json]andco.elastic.metrics/modulesto do the same for metricbeat.