Metricbeat/HTTP: Support array in http/json metricset #6480

dolftax · 2018-02-27T07:01:41Z

Currently (before this commit) the http/json metricset in Metricbeat
only can query information from http endpoints which expose
map[string]interface{}. For endpoints which expose an array on the
root level, the json metricset does not work.

A config option is added response.isarray | bool. If
someone configures array but a non array json object
is returned, an error is logged.

Event response is unified to []map[string]interface{} even
if the response is map[string]interface{}.

Signed-off-by: Jaipradeesh [email protected]

Ref: #6472

elasticmachine · 2018-02-27T07:01:43Z

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?

dolftax · 2018-02-27T07:04:20Z

@ruflin WIP. Yet to check thoroughly. Any thoughts on the approach?

ruflin · 2018-02-27T07:22:26Z

metricbeat/docs/modules/http.asciidoc

@@ -38,6 +38,7 @@ metricbeat.modules:
  #request.enabled: false
  #response.enabled: false
  #dedot.enabled: false
+  #response.isarray: false


I'm thinking if we should make this json.is_array or json.response.is_array. Now that we have multiple metricsets in the http module not all settings apply to all metricsets I think.

I'm aware that previous configs do not follow the convention :-(

ruflin · 2018-02-27T07:23:27Z

metricbeat/module/http/json/_meta/data.json

@@ -1,4 +1,4 @@
-{
+[{


This part is not going to change. In case of an array, each document will be sent as a single vent in the previous format.

ruflin · 2018-02-27T07:25:21Z

metricbeat/module/http/json/json.go

+		return nil, err
+	}
+
+	switch body := jsonBody.(type) {


Let's separate the processing and not only do a "dynamic" switch to figure out if there is an error. The way I imagine is that we have jsonBody and jsonBodyArray (or similar) and directly decode in what we want in the end. Like this I would assume we don't have to do any type checks and directly get the errors.

ruflin · 2018-02-27T07:27:28Z

It would be great if for the tests we could spin up a small mock http server in Golang as some other modules do. And then have some example json files with both variants to test against.

christiangalsterer · 2018-02-27T08:16:13Z

metricbeat/module/http/json/json.go

+	defer response.Body.Close()
+
+	var jsonBody interface{}
+	var event []common.MapStr


event should be named events as this now an array of events and eventObj can then be named event again.

@christiangalsterer glad you stumbled over the PR.

houndci-bot · 2018-02-28T10:13:17Z

metricbeat/module/http/_meta/test/main.go

+	fmt.Fprint(w, `[{"hello1":"world1"}, {"hello2": "world2"}]`)
+}
+
+func serveJsonObj(w http.ResponseWriter, r *http.Request) {


func serveJsonObj should be serveJSONObj

houndci-bot · 2018-02-28T10:13:18Z

metricbeat/module/http/_meta/test/main.go

 	err := http.ListenAndServe(":8080", nil)
 	if err != nil {
 		log.Fatal("ListenAndServe: ", err)
 	}
 }

-func serve(w http.ResponseWriter, r *http.Request) {
+func serveJsonArr(w http.ResponseWriter, r *http.Request) {


func serveJsonArr should be serveJSONArr

dolftax · 2018-02-28T15:15:08Z

➜  metricbeat git:(metricbeat-http-json-arrays) go test --tags=integration github.com/elastic/beats/metricbeat/module/http/json/... -v                   

=== RUN   TestFetch
--- PASS: TestFetch (1.14s)
	json_integration_test.go:24: http/json event: [{"_namespace":"testnamespace","hello":"world"}]
	json_integration_test.go:32: http/json event: [{"_namespace":"testnamespace","hello1":"world1"} {"_namespace":"testnamespace","hello2":"world2"}]
=== RUN   TestData
--- SKIP: TestData (1.18s)
	data_generator.go:43: skip data generation tests
PASS
ok  	github.com/elastic/beats/metricbeat/module/http/json	2.330s

@ruflin This PR LGTM. Let me know if any changes are required.

ruflin · 2018-03-04T05:00:16Z

metricbeat/module/http/json/json.go

 	} else {
-		event = jsonBody
+		event = jsonBody.(common.MapStr)


We probably should do some type conversion checks on both lines?

Sorry, I don't understand what you mean by 'both lines'. Elaborate please? :)

Sorry, I meant on line 106 and 108 as both to a type converstion to common.MapStr.

Did assertion because common.DeDotJSON accepts and emits interface{}. Type conversion would fail.

If types are mismatched, line 160 and 170 should error out.

One more miscommunication on my end. I meant type assertions. I was looking for:

event, ok = jsonBody.(common.MapStr) if !ok { ... }

If for whatever reason the assertion does not work, we would have a panic otherwise. At the same time I see it should not happen here.

ruflin · 2018-03-04T05:01:37Z

metricbeat/module/http/json/json_integration_test.go

 	event, err := f.Fetch()
 	if !assert.NoError(t, err) {
 		t.FailNow()
 	}

 	t.Logf("%s/%s event: %+v", f.Module().Name(), f.Name(), event)
+
+	f = mbtest.NewEventsFetcher(t, getConfig("array"))


Can we split this up in 2 tests? Like TestFetchObject and TestFetchArray? This will make it eaiser to spot what failed if something fails

ruflin · 2018-03-04T05:02:38Z

metricbeat/module/http/json/json_integration_test.go

 }

-func getConfig() map[string]interface{} {
+func getConfig(jsonType string) map[string]interface{} {


you could make this a bool called isArray. But that also works. Do you expect other types here in the future?

If confirming to JSON standards, yes. A string, number, object, array, true, false and null could be valid JSON. I would say to leave it this way than adding a boolean between array or not.

Interesting point about the other types. I wonder when a request for that will pop up :-) Lets go with your version.

ruflin · 2018-03-04T05:04:21Z

metricbeat/docs/modules/http.asciidoc

@@ -37,6 +37,7 @@ metricbeat.modules:
  #method: "GET"
  #request.enabled: false
  #response.enabled: false
+  #response.is_array: false


I was thinking again about the naming and I think we should go here with json.is_array. It's the json metricset so a prefix is needed, and as it's always about processing of the body / response anyways, we don't need this in.

json.is_array makes sense to me. Will amend the change.

dolftax · 2018-03-09T06:09:38Z

Build failed due to fetch (thus build) error. Re-trigger should fix it.

ruflin

Sorry for the slow response. PR looks almost complete. Left a few minor last comments.

ruflin · 2018-03-12T06:29:16Z

metricbeat/module/http/json/json.go

 	} else {
-		event = jsonBody
+		event = jsonBody.(common.MapStr)


Sorry, I meant on line 106 and 108 as both to a type converstion to common.MapStr.

ruflin · 2018-03-12T06:30:25Z

metricbeat/module/http/json/json.go

+	}
+	defer response.Body.Close()
+
+	var jsonBody common.MapStr


As jsonBody and jsonBodyArray are "local" to the if / else parts, these could be defined inside the if clause.

ruflin · 2018-03-12T06:32:05Z

metricbeat/module/http/json/json_integration_test.go

 }

-func getConfig() map[string]interface{} {
+func getConfig(jsonType string) map[string]interface{} {


Interesting point about the other types. I wonder when a request for that will pop up :-) Lets go with your version.

dolftax · 2018-03-12T08:29:34Z

Sorry about the merge commit. Please squash before merging into master :)

ruflin · 2018-03-12T12:35:47Z

@jaipradeesh This seems to conflict with the master branch. Could you rebase on master and push again?

I'm good with the changes. @christiangalsterer could you also have a look again?

Currently (before this commit) the http/json metricset in Metricbeat only can query information from http endpoints which expose map[string]interface{}. For endpoints which expose an array on the root level, the json metricset does not work. A config option is added `response.isarray | bool`. If someone configures array but a non array json object is returned, an error is logged. Event response is unified to []map[string]interface{} even if the response is map[string]interface{}. Signed-off-by: Jaipradeesh <[email protected]>

ruflin · 2018-03-13T06:55:23Z

jenkins, test it

ruflin · 2018-03-13T10:00:14Z

@jaipradeesh Thanks a lot for your contribution.

…#8974) Cherry-pick of PR #8951 to 6.x branch. Original message: Motivated by https://discuss.elastic.co/t/how-to-query-json-url-with-http-module/155572 and follow up doc PR to #6480. This PR documents the `json.is_array` setting supported by the `http/json` metricset. The setting was introduced in #6480 but not documented at the time.

dolftax force-pushed the metricbeat-http-json-arrays branch from 3816fdc to 672d0f7 Compare February 27, 2018 07:03

ruflin reviewed Feb 27, 2018

View reviewed changes

ruflin added module review labels Feb 27, 2018

christiangalsterer mentioned this pull request Feb 27, 2018

Support arrays in in http/json metricset #6472

Closed

christiangalsterer reviewed Feb 27, 2018

View reviewed changes

dolftax force-pushed the metricbeat-http-json-arrays branch from 672d0f7 to bdf6e18 Compare February 28, 2018 10:13

houndci-bot reviewed Feb 28, 2018

View reviewed changes

dolftax force-pushed the metricbeat-http-json-arrays branch 5 times, most recently from dad33c5 to 9936bde Compare February 28, 2018 14:33

ruflin reviewed Mar 4, 2018

View reviewed changes

dolftax force-pushed the metricbeat-http-json-arrays branch from 9936bde to 9183920 Compare March 4, 2018 09:38

ruflin reviewed Mar 12, 2018

View reviewed changes

dolftax force-pushed the metricbeat-http-json-arrays branch from c8c1cc2 to 1bf2901 Compare March 12, 2018 08:29

dolftax force-pushed the metricbeat-http-json-arrays branch from 1bf2901 to d89f6eb Compare March 12, 2018 14:25

ruflin merged commit 4e9f33b into elastic:master Mar 13, 2018

dolftax deleted the metricbeat-http-json-arrays branch March 13, 2018 10:34

This was referenced Nov 6, 2018

Add documentation for json.is_array setting #8951

Merged

Cherry-pick #8951 to 6.x: Add documentation for json.is_array setting #8974

Merged

Benny-Git mentioned this pull request Dec 21, 2018

String array support in http/json metricset #9743

Closed

live-wire mentioned this pull request Jan 17, 2019

Improving Top-level-Array support in metricbeat [ http module ] #10136

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Metricbeat/HTTP: Support array in http/json metricset #6480

Metricbeat/HTTP: Support array in http/json metricset #6480

dolftax commented Feb 27, 2018

elasticmachine commented Feb 27, 2018

dolftax commented Feb 27, 2018 •

edited

Loading

ruflin Feb 27, 2018

ruflin Feb 27, 2018

ruflin Feb 27, 2018

ruflin commented Feb 27, 2018

christiangalsterer Feb 27, 2018 •

edited

Loading

ruflin Feb 28, 2018

houndci-bot Feb 28, 2018

dolftax Feb 28, 2018

houndci-bot Feb 28, 2018

dolftax Feb 28, 2018

dolftax commented Feb 28, 2018

ruflin Mar 4, 2018

dolftax Mar 4, 2018 •

edited

Loading

ruflin Mar 12, 2018

dolftax Mar 12, 2018

ruflin Mar 12, 2018

ruflin Mar 4, 2018

ruflin Mar 4, 2018

dolftax Mar 4, 2018

ruflin Mar 12, 2018

ruflin Mar 4, 2018

dolftax Mar 4, 2018

dolftax commented Mar 9, 2018 •

edited

Loading

ruflin left a comment

ruflin Mar 12, 2018

ruflin Mar 12, 2018

ruflin Mar 12, 2018

dolftax commented Mar 12, 2018

ruflin commented Mar 12, 2018

ruflin commented Mar 13, 2018

ruflin commented Mar 13, 2018

Metricbeat/HTTP: Support array in http/json metricset #6480

Metricbeat/HTTP: Support array in http/json metricset #6480

Conversation

dolftax commented Feb 27, 2018

elasticmachine commented Feb 27, 2018

dolftax commented Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin commented Feb 27, 2018

christiangalsterer Feb 27, 2018 • edited Loading

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

dolftax commented Feb 28, 2018

Choose a reason for hiding this comment

dolftax Mar 4, 2018 • edited Loading

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

dolftax commented Mar 9, 2018 • edited Loading

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

dolftax commented Mar 12, 2018

ruflin commented Mar 12, 2018

ruflin commented Mar 13, 2018

ruflin commented Mar 13, 2018

dolftax commented Feb 27, 2018 •

edited

Loading

christiangalsterer Feb 27, 2018 •

edited

Loading

dolftax Mar 4, 2018 •

edited

Loading

dolftax commented Mar 9, 2018 •

edited

Loading