[Stats] Add metrics collector and stats API by tsullivan · Pull Request #17773 · elastic/kibana

tsullivan · 2018-04-18T16:28:12Z

Closes #17567
Depends on #17788

This adds a MetricsCollector module that's intended to be wrapped in the ops event handler. Currently the ops event handler is implemented with the even-better service (a fork of good from the HapiJS framework). The metrics collector class isn't a service and doesn't need start and stop methods.

The module accumulates the metrics data it receives via the collect() method. Importing the module doesn't access a singleton, so any function that needs access to the accumulated data (using getStats()) needs to have the collector passed to it.

The metrics collector instance is passed down to a new route for metrics stats: /api/stats, and this PR adds an API integration test for the new API route.

The intention for the new stats API is to eventually replace the existing /api/status route. That change can happen in the next major version as it would be a non-backwards compatible change. Therefore, the Metrics class, which was in use for the existing /api/status data, has been moved to the metrics collector's module directory, and the paths that reference it have been updated in the existing code.

Release notes: Added a new /api/stats route to expose metrics for Kibana Metricbeat.

tsullivan · 2018-04-18T16:41:07Z

Since this will give metrics an accumulator that remembers previous values, the next step we can do is collect CPU Utilization with process.cpuUsage: https://nodejs.org/dist/latest-v8.x/docs/api/process.html#process_process_cpuusage_previousvalue

elasticmachine · 2018-04-18T17:20:58Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-04-18T18:23:23Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-04-18T21:47:08Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

ruflin · 2018-04-19T12:17:43Z

src/server/status/metrics_collector/metrics_collector.js

+1 on moving metrics out of status API with 7.0. I think there is value in having different API endpoints to expose different metrics and states like ES does.

ruflin · 2018-04-19T12:20:33Z

src/server/status/routes/api/register_stats.js

Does this fetch the cluster_uuid on each request from ES?

it does. thoughts on a different endpoint if we need the cluster uuid? it looks like these are all independent of the cluster except for uuid, and it adds a network request to in memory data

Yeah, hm. I like the idea of not making that request to ES if the user doesn't care about cluster_uuid in the first place.

How keep the same endpoint, and keep it as a GET, but add an optional query param? /api/stats?with_cluster_uuid or something?

@ruflin does it make a difference on the Beats side whether fetching the cluster_uuid is on or off by default? I could make a separate endpoint completely, but it seems like over-engineering - it'd have to be named and documented reasonably.

I could get behind something like an ?extended flag that would include elasticsearch data. Saved object counts, uuid, can't think of anything else.

++ for extended. Thanks!

@tsullivan We can make the field optional and then it's fetched when it's there and otherwise ignored. I like it to have in the same document as it means we don't have to fire an additional http request.

If we have an extended mode an additional endpoint could make more sense, but param for 1 endpoint also works.

@tsullivan For the ES cluster_id: Can KB be connected to more then one cluster in the future? And could we perhaps cache the ES cluster uuid locally and only update it if KB for example reconnets to ES and check if it's still the same (if there is such a thing like reconnect).

ruflin · 2018-04-19T12:20:52Z

src/server/status/routes/api/register_stats.js

Looks like a non async call which I like.

jbudz · 2018-04-19T17:19:58Z

src/server/status/metrics_collector/metrics_collector.js

how would you feel if the hapi specifics were separated with this a class solely for accumulating events? I ask because it'll let us swap out hapi events one by one as we move away from it.

it's pretty much there, maybe renaming hapiEvent to stats is enough. in the future we could allow for many collect calls that only accumulate the fields sent in.

++ for changing to stats

jbudz · 2018-04-19T17:20:53Z

src/server/status/metrics_collector/sum_accumulate.js

i think this is specific enough to make it part of the metrics collector class. thoughts?

++ it was separate for testability, but it could be a static method of the class for the same idea

elasticmachine · 2018-04-19T18:54:47Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

tsullivan · 2018-04-19T19:13:51Z

Depends on #17788 (cleanup changes broken out)

elasticmachine · 2018-04-19T20:05:40Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-04-19T21:05:55Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-04-19T21:58:02Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-04-19T23:48:03Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-04-20T19:08:13Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

tsullivan · 2018-04-20T20:11:33Z

@jbudz I've added an optional query param for extended. The cluster_uuid isn't fetched by default.

Gets the cluster_uuid:

curl --silent 'https://elastic:changeme@spicy.local:5601/api/stats?extended' -H 'kbn-version: 7.0.0-alpha1'
curl --silent 'https://elastic:changeme@spicy.local:5601/api/stats?extended=true' -H 'kbn-version: 7.0.0-alpha1'

Doesn't get the cluster_uuid:

curl --silent 'https://elastic:changeme@spicy.local:5601/api/stats' -H 'kbn-version: 7.0.0-alpha1'
curl --silent 'https://elastic:changeme@spicy.local:5601/api/stats?extended=false' -H 'kbn-version: 7.0.0-alpha1'

elasticmachine · 2018-04-24T17:20:56Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

jbudz · 2018-04-25T17:48:31Z

src/server/status/metrics_collector/metrics.js

nit, in theory this may end up with a slightly different number. thoughts on setting this to process.uptime_ms?

yeah, good idea. It makes the TODO make more sense. 0abf0ff

tsullivan · 2018-04-25T17:50:01Z

rebased to master, fixed conflicts from #17788

jbudz · 2018-04-25T17:50:32Z

src/server/status/metrics_collector/metrics.js

-        avg_in_millis: get(hapiEvent, ['responseTimes', port, 'avg']),
-        max_in_millis: get(hapiEvent, ['responseTimes', port, 'max'])
+        // TODO: rename to use `_ms` suffix per beats naming conventions
+        avg_in_millis: isNaN(avgInMillis) ? undefined : avgInMillis, // convert NaN to undefined


thoughts on getting this over with now? if it's a new api we don't have to worry about breaking changes yet

This module is still used by the status API though, so a change in the field name here would break that.

tsullivan · 2018-04-25T18:17:17Z

src/server/status/metrics_collector/metrics.test.js

+      sinon.stub(metrics, 'captureEvent').returns({ 'a': [{ 'b': 2 }, { 'd': 4 }], process: { uptime_ms: 1980 } });
      sinon.stub(metrics, 'captureCGroupsIfAvailable').returns({ 'a': [{ 'c': 3 }, { 'e': 5 }] });
      sinon.stub(Date.prototype, 'toISOString').returns('2017-04-14T18:35:41.534Z');
-      sinon.stub(process, 'uptime').returns(5000);


this was existing code, but it looks like stubbing uptime this way actually affects all the tests. That's why this had to be moved down to parses the hapi event

elasticmachine · 2018-04-25T20:01:53Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-04-25T22:47:11Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

tsullivan · 2018-04-25T23:38:41Z

22:26:52        ?- ? fail: "discover app discover app query load query should show query name"
22:26:52        ?        tryForTime timeout: [POST http://localhost:9515/session/b28072116d3c83dc16daf1b559fff01b/element / {"using":"partial link text","value":"Query # 1"}] no such element: Unable to locate element: {"method":"partial link text","selector":"Query # 1"}

please test this

tsullivan · 2018-04-25T23:41:15Z

"please test this" doesn't work, looks like

jenkins test this

elasticmachine · 2018-04-26T00:42:01Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

tsullivan · 2018-04-26T01:43:59Z

Preliminary changes have been pulled out and merged, this is rebased on top, and it's all green now. Ready for last review

pickypg

Functionality LGTM, but I think we can simplify the sumAccumulate method to make it a little more readable for anyone else that may have to read this code.

pickypg · 2018-04-26T16:58:48Z

src/server/status/metrics_collector/metrics_collector.js

+  static sumAccumulate(property, accum, next) {
+    const accumValue = accum[property];
+    const nextValue = next[property];
+    if (Object.prototype.toString.call(nextValue) === '[object Object]') {


I believe the ideal approach to looking for basic Objects is checking the constructor (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/constructor):

nextValue && nextValue.constructor === Object

I wonder if this function should be structured differently:

if (nextValue === null || nextValue === undefined) { // return ? } else if (nextValue.constructor === Object) { // recurse } else if (nextValue.constructor === Number) { // sum } else { // return raw value or drop it since we don't know what it is? }

This should avoid string manipulation for the comparison and it should read a little cleaner. Also, we're ignoring Arrays, but since the OS / process stats report system load that should be okay.

this is a good idea, since we should not try to accumulate null

pickypg · 2018-04-26T17:01:43Z

src/server/status/routes/api/register_stats.js

+      },
+      async handler(req, reply) {
+        const { extended } = req.query;
+        const isExtended = extended !== undefined && extended !== 'false';


Like ES APIs! I like it.

pickypg · 2018-04-26T17:03:05Z

src/server/status/routes/api/register_stats.js

+            const { cluster_uuid: uuid } = await callWithRequest(req, 'info', { filterPath: 'cluster_uuid', });
+            clusterUuid = uuid;
+          } catch (err) {
+            clusterUuid = undefined; // fallback from anonymous access or auth failure, redundant for explicitness


You don't really need to remove it, but it's implicitly undefined regardless of this line if we are catching an error.

yeah, I'm leaving it to be redundant for explicitness, and also because it would fail lint to have an empty catch block

I'm pretty sure that the linter will succeed given a comment, but this works too.

elasticmachine · 2018-04-30T18:28:09Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

pickypg

LGTM

* [Stats] Add metrics collector and stats API * uptime_ms in the process namespace * make uptime_in_millis always equal process.uptime_ms * fix api integration test * fix api integration test better * fix false positive with last change * change object detection, add fallbacks to return undefined

tsullivan · 2018-05-01T03:47:02Z

6.x/6.4.0: #18677

* [Stats] Add metrics collector and stats API * uptime_ms in the process namespace * make uptime_in_millis always equal process.uptime_ms * fix api integration test * fix api integration test better * fix false positive with last change * change object detection, add fallbacks to return undefined

tsullivan added review release highlight Team:Operations Kibana-Operations Team v7.0.0 v6.4.0 labels Apr 18, 2018

tsullivan requested review from epixa, jbudz, pickypg and tylersmalley April 18, 2018 16:28

tsullivan mentioned this pull request Apr 18, 2018

Stats API #17304

Closed

tsullivan force-pushed the stats/add-metrics-collector-and-route branch from edb307f to d873a04 Compare April 18, 2018 20:51

tsullivan mentioned this pull request Apr 19, 2018

[Status] Organize metrics stuff into metrics_collector, tests into __test__ directories, remove mock-fs for the cgroup test #17788

Merged

ruflin reviewed Apr 19, 2018

View reviewed changes

jbudz reviewed Apr 19, 2018

View reviewed changes

tsullivan force-pushed the stats/add-metrics-collector-and-route branch 2 times, most recently from 6b6dbf1 to 97fbf02 Compare April 19, 2018 18:01

tsullivan mentioned this pull request Apr 19, 2018

[Kibana Module] Add additional metric fields elastic/beats#6746

Merged

jbudz reviewed Apr 25, 2018

View reviewed changes

tsullivan added 2 commits April 25, 2018 10:48

[Stats] Add metrics collector and stats API

35a3279

uptime_ms in the process namespace

634dfc8

tsullivan force-pushed the stats/add-metrics-collector-and-route branch from 845c336 to 634dfc8 Compare April 25, 2018 17:48

jbudz reviewed Apr 25, 2018

View reviewed changes

make uptime_in_millis always equal process.uptime_ms

0abf0ff

tsullivan commented Apr 25, 2018

View reviewed changes

tsullivan added 2 commits April 25, 2018 11:26

fix api integration test

dbd3466

fix api integration test better

7a990a9

fix false positive with last change

dac1b94

pickypg requested changes Apr 26, 2018

View reviewed changes

tsullivan added 2 commits April 30, 2018 09:24

Merge branch 'master' into stats/add-metrics-collector-and-route

f4c3e19

change object detection, add fallbacks to return undefined

053e6a7

pickypg approved these changes Apr 30, 2018

View reviewed changes

tsullivan merged commit c10dc75 into elastic:master May 1, 2018

tsullivan deleted the stats/add-metrics-collector-and-route branch May 1, 2018 03:46

tsullivan mentioned this pull request May 1, 2018

[6.x] [Stats] Add metrics collector and stats API (#17773) #18677

Merged

tsullivan mentioned this pull request May 10, 2018

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

Closed

Conversation

tsullivan commented Apr 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tsullivan commented Apr 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Apr 18, 2018

💚 Build Succeeded

Uh oh!

elasticmachine commented Apr 18, 2018

💚 Build Succeeded

Uh oh!

elasticmachine commented Apr 18, 2018

💔 Build Failed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsullivan Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Apr 19, 2018

💚 Build Succeeded

Uh oh!

tsullivan commented Apr 19, 2018

Uh oh!

elasticmachine commented Apr 19, 2018

💚 Build Succeeded

Uh oh!

elasticmachine commented Apr 19, 2018

💚 Build Succeeded

Uh oh!

elasticmachine commented Apr 19, 2018

💚 Build Succeeded

Uh oh!

elasticmachine commented Apr 19, 2018

💚 Build Succeeded

Uh oh!

elasticmachine commented Apr 20, 2018

💚 Build Succeeded

Uh oh!

tsullivan commented Apr 20, 2018

Gets the cluster_uuid:

Doesn't get the cluster_uuid:

Uh oh!

elasticmachine commented Apr 24, 2018

💚 Build Succeeded

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsullivan commented Apr 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

tsullivan commented Apr 18, 2018 •

edited

Loading

tsullivan commented Apr 18, 2018 •

edited

Loading

tsullivan Apr 19, 2018 •

edited

Loading