[Kibana Module] Add additional metric fields #6746

tsullivan · 2018-04-03T17:49:50Z

Adds a new Kibana module for stats. This is based on the existing status module, which will be deprecated in Kibana once stats becomes established.

Depends on elastic/kibana#17773

tsullivan · 2018-04-03T17:54:46Z

This is a work-in-progress change as it'll need some extra work after this first commit to implement the schema I'm going for:

avg_in_millis is sometimes an integer, and sometimes a null value. If there were no requests in the measurement window, then the value is null
requests.status_codes is a dictionary with keys for the status code. Only the status codes that have been measured in the window are present as keys.
- Example:
```
"requests": {
   "total": 3,
   "disconnects": 0,
   "status_codes": {
      "200": 3
   }
}
```

These need to be handled in the schema to avoid error log messages. Advice is welcome! :)

ruflin · 2018-04-03T18:40:57Z

To make certain fields optional, you can pass an optional flag. See here for an example: https://github.com/elastic/beats/blob/master/metricbeat/module/apache/status/data.go#L22

Also metricbeat tests will required you to document the new fields here (https://github.com/elastic/beats/blob/master/metricbeat/module/kibana/status/_meta/fields.yml) and then run make update.

houndci-bot · 2018-04-19T23:28:20Z

metricbeat/module/kibana/stats/data.go

+	}
+)
+
+type OverallMetrics struct {


exported type OverallMetrics should have comment or be unexported

tsullivan · 2018-04-19T23:30:14Z

To make certain fields optional, you can pass an optional flag

I'm going to ensure that all the fields are predictable by the API, so I won't really need to do this.

Also metricbeat tests will required you to document the new fields here

Thanks for the tip. I made some changes and ran make update

ruflin · 2018-04-23T08:44:24Z

metricbeat/module/kibana/stats/_meta/data.json

+    },
+    "kibana": {
+        "stats": {
+            "cluster_uuid": "G279hqGeSDqjj_OgBq6wUw",


If this is the elasticsearch cluster uuid, we should put it under elasticsearch.cluster.id as we do in other events. This makes it possible to correlate all events from one cluster. I can take care of that in a follow up PR.

It would affect the Kibana PR as well as this one, so wouldn't it make sense to do this before merging, to reduce the follow-up needed?

The Kibana PR still depends on another one that no one has had time to review yet, so we still have some time to adjust the data structure.

Correct me if I'm wrong about this affecting the Kibana PR. Maybe as an alternative the solution is to map incoming cluster_uuid value to elasticsearch.cluster.id in metricbeat/module/kibana/stats/data.go? If yes, then I'd agree to taking care of it in a followup PR.

ruflin · 2018-04-23T08:47:42Z

metricbeat/module/kibana/stats/stats.go

+
+// New create a new instance of the MetricSet
+func New(base mb.BaseMetricSet) (mb.MetricSet, error) {
+	cfgwarn.Beta("The kibana metricset is beta")


We have an interesting case here we haven't had in Beats so far, that we release a metricset for an endpoint which is not available yet.

Is the stats endpoint going into 6.4?

Yes, it's planned for 6.4: elastic/kibana#17773

ruflin · 2018-04-23T08:48:18Z

metricbeat/module/kibana/stats/data.go

+				"state": c.Str("state"),
+			}),
+		}),
+		"uptime_in_millis": c.Int("uptime_in_millis"),


We should follow our metricbeat conventions here for the units: https://www.elastic.co/guide/en/beats/devguide/current/event-conventions.html

Just for my clarity, you're suggesting that the change would just be a naming switch in this file, correct?

"uptime_in_millis": c.Int("uptime_ms"),

?

I guess this idea wouldn't work:

2018-04-23T17:44:15.601-0700 ERROR schema/schema.go:41 Error on field 'uptime_in_millis': Missing field: uptime_in_millis, Error: Key uptime_ms not found 2018-04-23T17:44:15.601-0700 ERROR schema/schema.go:41 Error on field 'avg_in_millis': Missing field: avg_in_millis, Error: key avg_ms not found 2018-04-23T17:44:15.601-0700 ERROR schema/schema.go:41 Error on field 'max_in_millis': Missing field: max_in_millis, Error: Key max_ms not found

@ruflin is there a way we can pivot on the naming within the beat module? It would help us avoid some messy work in Kibana around not introducing breaking API changes.

ruflin · 2018-04-23T08:48:59Z

metricbeat/module/kibana/stats/_meta/fields.yml

+          fields:
+            - name: free_in_bytes
+              type: long
+              description: >


For the bytes you can add format: bytes so the Kibana index pattern will automatically contain the unit.

ruflin · 2018-04-23T08:50:30Z

@tsullivan There seems to be a panic in the tests at the moment, probably because metrics or something that you access doesn't exist in the event.

I suggest we get this PR in as soon as the stats endpoint is merged in Kibana and then can do follow up PR's to clean up the naming and add integration tests with the actual endpoints.

tsullivan · 2018-04-23T21:56:30Z

then can do follow up PR's to clean up the naming and add integration tests with the actual endpoints.

Sounds good to me. I will remove the added integration tests for now.

The feedback around naming looks like things I should work on in elastic/kibana#17773 and then update here to reflect the change before anything gets merged. What do you think?

ruflin · 2018-04-24T08:15:59Z

@tsullivan For the naming: We could follow the same approach here as we do with the ES endpoints to have an XPack version and a MB version data structure for the next 6.x release. Like this the naming in Kibana is consistent and we can still follow the MB naming scheme to be in line with all other endpoints.

I'm also ok to get it in with the Kibana naming so I can add it as being X-Pack in a follow up PR and add the MB naming part.

tsullivan · 2018-05-01T03:48:43Z

elastic/kibana#17773 is merged, making this PR testable against the master branch of Kibana!

ruflin · 2018-05-02T08:03:22Z

@tsullivan That is great. We need to modify the testing somehow as so far all metricbeat modules are tested against stable releases. But @jsoriano introduced for other modules to test it against multiple versions which we can also do for Kibana. Main difference is that we will have to build our own image here (or reuse the one from testing/environments). CI will probably not very happy as Kibana takes forever to start and 2 instances makes it worse, but it should work. @tsullivan Let me see how we can make that happen in the repo and then rebase this PR on top of it.

tsullivan · 2018-05-03T01:16:29Z

@tsullivan Let me see how we can make that happen in the repo and then rebase this PR on top of it.

Sounds like a plan. Let me know when this branch should be rebased.

houndci-bot · 2018-05-08T07:53:47Z

metricbeat/module/kibana/stats/data.go

+	}
+)
+
+type OverallMetrics struct {


exported type OverallMetrics should have comment or be unexported

houndci-bot · 2018-05-08T07:53:47Z

metricbeat/module/elasticsearch/node_stats/data_xpack.go

@@ -0,0 +1,220 @@
+package node_stats


don't use an underscore in package name

houndci-bot · 2018-05-08T07:53:47Z

libbeat/processors/add_host_metadata/config.go

@@ -0,0 +1,12 @@
+package add_host_metadata


don't use an underscore in package name

ruflin · 2018-05-23T12:39:59Z

Ready for a first round of reviews. Tests are currently only run against a file as 6.4 is not out yet. It would be possible to add an additional Kibana version to our test env but already 1 instance of KB puts enough load on our test environment and it would also require an additional version of ES for compatibility.

jsoriano · 2018-05-23T14:35:21Z

metricbeat/module/kibana/testing.go

@@ -0,0 +1,32 @@
+package kibana


Move common testing code to kibana/mtest as discussed in #7106 (comment)?

jsoriano · 2018-05-23T14:36:00Z

metricbeat/module/kibana/testing.go

+	return map[string]interface{}{
+		"module":     "kibana",
+		"metricsets": []string{metricset},
+		"hosts":      []string{GetEnvHost() + ":" + GetEnvPort()},


net.JoinHostPort() :)

changed. We have this all over the place. Probably time to extract this method more generic.

This is based on the existing `status` module, which will be deprecated in Kibana once `stats` becomes established. The stats endpoint will be available in Kibana 6.4 and newer.

tsullivan requested a review from ruflin April 3, 2018 17:49

ruflin added in progress Pull request is currently in progress. module Metricbeat Metricbeat labels Apr 3, 2018

rayafratkina added the monitoring label Apr 4, 2018

monicasarbu removed the monitoring label Apr 11, 2018

tsullivan force-pushed the kibana-stats branch from 3b742a8 to 4a7971a Compare April 19, 2018 23:28

houndci-bot reviewed Apr 19, 2018

View reviewed changes

ruflin reviewed Apr 23, 2018

View reviewed changes

This was referenced May 8, 2018

Fetch logs from the Elastic Stack with Filebeat #7036

Closed

Monitor the Elastic Stack with Metricbeat #7035

Closed

houndci-bot reviewed May 8, 2018

View reviewed changes

tsullivan mentioned this pull request May 10, 2018

Allow plugins to register their own stats for an _xpack/usage API elastic/kibana#12504

Closed

ruflin force-pushed the kibana-stats branch from 258a123 to a6a6e98 Compare May 23, 2018 08:28

ruflin added review and removed in progress Pull request is currently in progress. labels May 23, 2018

ruflin requested a review from jsoriano May 23, 2018 12:40

jsoriano reviewed May 23, 2018

View reviewed changes

Add stats metricset to Kibana module

5b2fb25

This is based on the existing `status` module, which will be deprecated in Kibana once `stats` becomes established. The stats endpoint will be available in Kibana 6.4 and newer.

ruflin force-pushed the kibana-stats branch from a4c5f80 to 5b2fb25 Compare May 24, 2018 10:16

jsoriano approved these changes May 24, 2018

View reviewed changes

jsoriano merged commit 8fa913c into elastic:master May 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Kibana Module] Add additional metric fields #6746

[Kibana Module] Add additional metric fields #6746

tsullivan commented Apr 3, 2018 •

edited

Loading

tsullivan commented Apr 3, 2018

ruflin commented Apr 3, 2018

houndci-bot Apr 19, 2018

tsullivan commented Apr 19, 2018

ruflin Apr 23, 2018

tsullivan Apr 23, 2018

tsullivan Apr 23, 2018

ruflin Apr 23, 2018

tsullivan Apr 23, 2018

ruflin Apr 23, 2018

tsullivan Apr 23, 2018

tsullivan Apr 23, 2018

tsullivan Apr 24, 2018

ruflin Apr 23, 2018

ruflin commented Apr 23, 2018

tsullivan commented Apr 23, 2018

ruflin commented Apr 24, 2018

tsullivan commented May 1, 2018

ruflin commented May 2, 2018

tsullivan commented May 3, 2018

houndci-bot May 8, 2018

houndci-bot May 8, 2018

houndci-bot May 8, 2018

ruflin commented May 23, 2018

jsoriano May 23, 2018

ruflin May 24, 2018

jsoriano May 23, 2018

ruflin May 24, 2018

[Kibana Module] Add additional metric fields #6746

[Kibana Module] Add additional metric fields #6746

Conversation

tsullivan commented Apr 3, 2018 • edited Loading

tsullivan commented Apr 3, 2018

ruflin commented Apr 3, 2018

Choose a reason for hiding this comment

tsullivan commented Apr 19, 2018

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 commented Apr 23, 2018

tsullivan commented Apr 23, 2018

ruflin commented Apr 24, 2018

tsullivan commented May 1, 2018

ruflin commented May 2, 2018

tsullivan commented May 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin commented May 23, 2018

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

tsullivan commented Apr 3, 2018 •

edited

Loading