Added 'query' parameter to metricbeats global configuration #8292

sayden · 2018-09-12T16:05:39Z

Initial PR on mb package which fixes issue #8286 but I guess it will be better to implement in the HTTP helper. The thing is that the URI parsing of a metricbeat occurs in the mb package so to implement it outside of it could lead to ugly-and-not-so-predictable code

I've tested in local successfully but I haven't found a test that already covers Build() method.

Ideas welcome :)

jsoriano

Thanks for identifying and fixing this issue, it mostly LGTM, could you add a test case to TestURLHostParserBuilder test in url_test.go that would have failed without this change?

jsoriano · 2018-09-12T17:48:49Z

Oh, and add a reference to this setting in metricbeat/docs/metricbeat-options.asciidoc

exekias · 2018-09-13T08:56:28Z

Nice! this looks like the right solution 🎉 Please add also a changelog entry. I'm adding the needs_backport label as this will need to be backported to 6.x at least

sayden · 2018-09-13T09:24:57Z

I have been digging into the code and I found a way to implement it within the HTTP helper instead that on the mb package. I don't know what do you think but I think it's a bit less intrusive than to put it directly into the mb (all in all, it's very HTTP specific and maybe it wouldn't make sense for a module that uses UDP or RPC).

What do you think?

jsoriano · 2018-09-13T09:50:38Z

@sayden but is the HTTPHelper affected by this issue? I mean, if HTTPHelper receives a correctly built HostData, does it have any issue with query params?
If not, I think that we should solve it in the parts that build an incorrect HostData, as seems to be in URLHostParserBuilder.

sayden · 2018-09-13T10:12:12Z

HTTPHelper is indirectly affected because it's the URLHostParserBuilder the one which always receives an empty query. As soon as no query params are present in the host, this line https://github.com/elastic/beats/blob/master/metricbeat/helper/http.go#L94 won't receive an escaped uri.

In this function, https://github.com/elastic/beats/blob/master/metricbeat/helper/http.go#L102 the http.Request can be modified to include query params too.

I really don't have a strong preference so... as you wish.

sayden · 2018-09-13T15:03:10Z

Ok, after a quick chat with @jsoriano I'm going with the base Metricset option

exekias

I revisited this code now, I'm wondering if this could get fixed from NewHostDataFromURL, as there you get access to the url.URL object, which contains the RawQuery

exekias · 2018-09-14T08:13:13Z

metricbeat/mb/mb.go

+func (q QueryParams) String() (s string) {
+	for k, v := range q {
+		if v == nil {
+			v = "null"


why setting null?, I think URL params support empty values

I wasn't sure I have to admit but Go was setting literally <nil> so I thought that null was a bit more universal.

I have just checked it and it seems that empty values are supported. Thanks for the thumbs up!

exekias · 2018-09-14T08:13:51Z

metricbeat/mb/mb.go

+// String returns the values in common query params format (key=value&key2=value2) which is the way that the url
+// package expects this params (without the initial '?')
+func (q QueryParams) String() (s string) {
+	for k, v := range q {


You could also use url.Values for this

exekias · 2018-09-14T08:18:27Z

metricbeat/_meta/fields.common.yml

@@ -38,3 +38,7 @@
      example: elasticsearch
      description: >
        Name of the service metricbeat fetches the data from.
+
+    - name: query


fields.yml spec is used to define the index template for the data we push: https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-templates.html

I believe we are not reporting the query as for this change, so we don't need to configure a mapping for it

sayden · 2018-09-14T09:44:07Z

Ok, as requested by @exekias I'm gonna improve the code with the suggestions.

I have also checked that the contents of a url query param can also be an array (with syntax array[]=value1&array[]=value2 or array=value1&array=value2. I have never seen it but it is actually possible.

I have also checked that the implementation can't be done in NewHostDataFromURL because the RawQuery field is set only if you have set previously the QueryParams field in URLHostParserBuilder. It will arrive empty if not.

Edit: Added a one more common way to set array in the query. It seems it's not standarized at all

sayden · 2018-09-14T12:57:28Z

I was actually wondering if it is better to proceed with a string value for the query configuration field or use the YAML format to build a query like in the following example:

metricbeat.modules:
- module: http
  metricsets:
   - json
   - server
  period: 1s
  hosts: ["localhost:8080"]
  namespace: "json_namespace"
  path: "/home"
  query:
    key: value
    key2: 11
    key3: 12.3
    key4: true
    key5: -4
    ar:
    - ar1
    - ar2
    - ar3
    ar2:
    - 13
    - -14.3
    - -15
    - 33
output.console:
  pretty: true

Instead of

query: "ar=ar1&ar=ar2&ar=ar3&ar2=13&ar2=-14.300000&ar2=-15&ar2=33&key=value&key2=11&key3=12.300000&key4=true&key5=-4"

What do you guys think @exekias @jsoriano ?

exekias · 2018-09-17T08:35:56Z

That looks better to me, wdyt @jsoriano?

jsoriano · 2018-09-17T09:26:59Z

It looks good to me too.

houndci-bot · 2018-09-19T15:13:48Z

libbeat/processors/add_process_metadata/test_provider.go

+// specific language governing permissions and limitations
+// under the License.
+
+package add_process_metadata
