Separate bulk upload behavior from CollectorSet by tsullivan · Pull Request #19691 · elastic/kibana

tsullivan · 2018-06-05T23:24:43Z

Part of #19534
Closes #19567

This PR prepares the way for the CollectorSet object to be moved to kbn_server in core and instantiated before the Kibana plugins are.

Changes

takes out a lot of behavior from CollectorSet and moves it to a class called BulkUploader
simplifies kibana monitoring init by taking out the indirection that startCollectorSet / createCollectorSet had
removes start() method from CollectorSet and calls the collector objects' init() functions from CollectorSet.register()
removes cleanup method from collectorSet since that was doing work for bulk uploading
keeps start/stop methods in the new BulkUploader class

Testing:

One of the most important things to ensure with this PR is that Kibana monitoring and telemetry still works.

For Kibana monitoring, the Kibana instance dashboards should still show up-to-date operational information. Stopping and restarting Elasticsearch should work, meaning that no (new) server errors are logged, and when Elasticsearch starts again, the dashboards keep updating on their own.
For telemetry, we must make sure the /api/_xpack/usage HTTP endpoint still shows all the same information as it does in the master branch.

Kibana Ops monitoring resumes even if Elasticsearch was down for a minute (Kibana server did not restart):

pickypg

Let's see if we can work in the start/stop logic for the collector set (or just an enabled/disabled state).

pickypg · 2018-06-07T21:50:14Z

x-pack/plugins/monitoring/server/kibana_monitoring/bulk_uploader.js

Is there a reason that this method was added as part of the constructor, while the other two below get called out separately? The old version was its own class method.

The only reason is because I don't want this class to have a this.collectorSet, which it would need if start() wasn't able to reach the collectorSet via enclosure. It's not a big deal, just my personal preference.

What do you think about that reason?

I'm fine with the reasoning. Perhaps add a comment to note that's why?

pickypg · 2018-06-07T22:54:53Z

x-pack/plugins/monitoring/server/kibana_monitoring/index.js

This is a dangerous race condition. If Kibana starts up before/while Elasticsearch is starting (or happens to be non-responsive for some reason during this time period), then Kibana monitoring is guaranteed to be disabled until Kibana is restarted. Instead, I think that we would be better off stopping/starting the collector based on this logic (or, continuing to blindly start it). Since we can listen to the xpack_main.info object, we should be safe to do that.

Also, by not returning the collectorSet, we force other plugins to validate the CollectorSet's existence before using it in addition to the monitoring plugin being enabled.

In the next version of this code, collectorSet will be moved to src/server/usage in core Kibana. There, it will be initiated before any of the plugins and core plugins are. Plugins will access it like:

const { collectorSet } = server.usage;

This part of the code looks like a miss in the refactoring - the CollectorSet constructor only needs server passed in, and it needs that for logging.

collectorSet doesn't need to have conditional instantiation/startup, but the bulk uploader's start method probably does

pickypg · 2018-06-07T22:57:16Z

x-pack/plugins/monitoring/init.js

Since initKibanaMonitoring (as this PR writes it) can return undefined, doesn't this fail hard in that scenario?

tsullivan · 2018-06-08T01:01:28Z

x-pack/plugins/monitoring/init.js

Per the previous comments on this area, I still need to handle stopping/restarting the uploader when xpack_main plugin goes red/back to green, and check the info before starting again

- takes out a lot of behavior from CollectorSet and moves it to a class called BulkUploader - simplifies kibana monitoring init by taking out the indirection that startCollectorSet / createCollectorSet had - removes start() method from CollectorSet and calls the collector objects' init() functions from CollectorSet.register() - removes cleanup method from collectorSet since that was doing work for bulk uploading

…nditional

tsullivan · 2018-06-08T23:13:57Z

I'm backing off on my comment about breaking out some of the changes to a smaller PR. When I started going that way, the work it took to get those small changes working with no regressions were almost as big as this one.

tsullivan · 2018-06-08T23:38:28Z

@pickypg I updated code per your feedback about needing a start / stop. I only added a stop method to BulkUploader, the CollectorSet doesn't need start / stop methods.

If Elasticsearch is down, we'll have to handle that possibility in the API route handlers that call collectorSet.bulkFetch() using try/catch. Even right now though, most APIs do not act well responding appropriately if Elasticsearch is down - as you can see from my Client Response Time screenshot.

tsullivan · 2018-06-08T23:46:40Z

x-pack/plugins/xpack_main/server/routes/api/v1/xpack_usage.js

        });
      } catch(err) {
-        req.log(['error'], err);
+        req.log(['error'], err); // FIXME doesn't seem to log anything useful if ES times out


The FIXME comment here seems to be the case for any API that relies on ES, but I want to look into it more later

tsullivan · 2018-06-09T00:00:41Z

x-pack/plugins/monitoring/init.js

+   */
+  collectorSet.register(getOpsStatsCollector(server));
+  collectorSet.register(getKibanaUsageCollector(server));
+  collectorSet.register(getSettingsCollector(server));


Registering a collector doesn't need to be delayed on anything. It's only when fetch is called on them that we want to try to make sure Elasticsearch is up. The bulkUploader handles this by watching elasticsearch plugin status. This isn't happening robustly from the API caller yet, but I have some plans to make that better in a later PR.

tsullivan · 2018-06-09T00:01:34Z

x-pack/plugins/monitoring/init.js

+      );
    }
-
-    monitoringPlugin.status.green('Ready');


It was not a good idea for an earlier PR to add this - having the monitoring plugin status always be green was supposed to be a feature

tsullivan · 2018-06-09T00:04:55Z

x-pack/plugins/monitoring/server/kibana_monitoring/collectors/get_ops_stats_collector.js

    type: KIBANA_STATS_TYPE,
-    init,
-    fetch: buffer.flush,
-    fetchAfterInit: true,


I intended to pull the removal of fetchAfterInit and cleanup as properties of the Collector constructor argument object, but getting that working without regressions of its own turned out to be a huge change that would have been temporary.

spalger · 2018-06-20T00:06:41Z

x-pack/plugins/monitoring/init.js

+      ['info', LOGGING_TAG, KIBANA_MONITORING_LOGGING_TAG],
+      'Internal collection for Kibana monitoring will is disabled per configuration.'
+    );
+  } else if (!xpackMainInfo) {


server.plugins.xpack_main.info is a service, so if it's missing it doesn't mean that X-Pack info couldn't be fetched, but that the plugin has changed in some way. I think this block can just be deleted.

spalger · 2018-06-20T00:09:36Z

x-pack/plugins/monitoring/server/kibana_monitoring/bulk_uploader.js

+   * @return undefined
+   */
+  start(collectorSet) {
+    this._log.info('Starting monitoring stats collection');


Feels like this should go after the check for this._timer

not sure why this comment didn't become outdated /shrug

spalger · 2018-06-20T00:10:24Z

x-pack/plugins/monitoring/server/kibana_monitoring/bulk_uploader.js

+
+    if (!monitoringBulkEnabled) {
+      // debug level to not spam the logs
+      this._log.debug(`Skipping fetch and upload of monitoring stats due to monitoring bulk endpoint not being available`);


Any reason we shouldn't call this.stop()?

Continuing to check at an interval protects the possibility that the bulk endpoint in ES can be disabled with a dynamic setting (not sure if that is possible), or that the ES configuration changes without triggering an ES plugin state change in Kibana. That seems like it is possible with hot-swapping ES for one with a different configuration.

@tsullivan The endpoint will not change based on the license, nor can it be changed dynamically (the endpoint either exists, or it doesn't because xpack.monitoring.enabled: false is explicitly set). However, calling this.stop() would require the xpack_main plugin to go from green to another state, then back to green for the service to be restarted, which is not a requirement for a license change.

Relative to the next comment, I think that is the superior option and then we can add this logic there.

spalger · 2018-06-20T00:19:42Z

x-pack/plugins/monitoring/init.js

+  if (kibanaCollectionEnabled && xpackMainInfo) {
+    const bulkUploader = initBulkUploader(monitoringPlugin.kbnServer, server, xpackMainInfo);
+
+    xpackMainPlugin.status.on('green', async () => { // any time xpack_main turns green


Would be great to switch this over to xpackInfo.onLicenseInfoChange() once #20018 is merged

With that listener support, I think we can safely do that and I think we can then back out the license checking code from the BulkUploader itself per collection and put it back into such a listener.

spalger · 2018-06-20T00:21:18Z

x-pack/plugins/monitoring/server/kibana_monitoring/bulk_uploader.js

+    const data = await collectorSet.bulkFetch(this._callClusterWithInternalUser);
+    const usableData = data.filter(d => Boolean(d) && !isEmpty(d.result));
+    const payload = usableData.map(({ result, type }) => {
+      if (!isEmpty(result)) {


This condition is redundant because of the .filter() right?

I think it would also become more readable to just change this to

const payload = data.filter(d => Boolean(d) && !isEmpty(d.result)).map(({ result, type }) => [{ index: { _type: type } }, result]);

…rset

tsullivan · 2018-06-20T21:29:31Z

unrelated visualization test failure (Vega)

jenkins test this

tsullivan · 2018-06-20T22:48:43Z

different unrelated failures this time:

22:07:57            ?- ? fail: "dashboard app using legacy data dashboard state Directly modifying url updates dashboard state for embeddable config color parameters on a visualization updates a pie slice color on a soft refresh"
22:08:44        ?- ? fail: "dashboard app using legacy data dashboard state "after all" hook"
``

tsullivan · 2018-06-20T22:48:49Z

jenkins test this

elasticmachine · 2018-06-20T23:55:35Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

spalger

One last thing, but otherwise looks good!

spalger · 2018-06-21T00:12:08Z

x-pack/plugins/monitoring/init.js

-    monitoringPlugin.status.green('Ready');
+  // xpack main plugin status goes red if ES client lost connection
+  xpackMainPlugin.status.on('red', () => {
+    bulkUploader.handleConnectionLost();


I think this is unnecessary and will cause problems because we are stopping the bulkUploader without starting again. Instead I think the bulk uploader should just try to talk to elasticsearch and fail if it can't. If we really want to get rid of the errors while Kibana is a red status we can check xpackMainPlugin.status.state !== 'red' before logging about errors talking to elasticsearch or something, but I suggest avoiding toggling the bulkUploader based on plugin status.

stopping the bulkUploader without starting again

It will get restarted when the the xpack main plugin status change causes the license info to change, but yeah - that is a pretty indirect signal.

we can check xpackMainPlugin.status.state !== 'red'

I would have liked to not log avoidable errors, and checking the xpack main plugin state feels like something that should be handled by the platform with. Earlier in this code, I was passing xpackMainInfo into the bulk uploader constructor, and I really like that I'm not doing that in the latest version.

It sounds like checking the status to avoid logging errors is a nice-to-have. I like the idea of simplifying the code, so I'll just remove this status handler.

elasticmachine · 2018-06-21T19:24:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

tsullivan · 2018-06-21T20:09:02Z

spencer gave verbal lgtm

* Separate bulk upload behavior from CollectorSet - takes out a lot of behavior from CollectorSet and moves it to a class called BulkUploader - simplifies kibana monitoring init by taking out the indirection that startCollectorSet / createCollectorSet had - removes start() method from CollectorSet and calls the collector objects' init() functions from CollectorSet.register() - removes cleanup method from collectorSet since that was doing work for bulk uploading * remove cleanup and fetchAfterInit methods * test for bulk_uploader class * improve test for collector_set * fix reporting * expose collectorSet if there actually is a collectorSet * comment for enclosed function * make collectorSet creation/expose unconditional, bulkUploader more conditional * fix collector_set tests * lifecycle events * stab at collectorSet error logging from the API call * clean up comments * clean up comments * fix BulkUploader mocha test * check kibanaCollectionEnabled config before registering bulk upload and the plugin status listeners * no singleton timer object * just log a warning if bulk uploader start called twice * normal quotes * check if bulk is enabled inside of the _fetchAndUpload method * log for stopping bulk stats * call bulkUploader.start with the collectorSet object * call bulkUploader.start with the collectorSet object * roll back change for module scoped variable * oops I broke init * init and logging: if / elseif / elseif * remove unnecessary check/log * help log * remove redundant, use data.filter.map * use xpackInfo.onLicenseInfoChange not xpackMainPlugin.status.on('green') * help logging * fix unit test * remove handler that stops upload when connection is lost

tsullivan added WIP Work in progress v6.4.0 Team:Monitoring Stack Monitoring team Feature:Telemetry labels Jun 5, 2018

tsullivan requested a review from pickypg June 5, 2018 23:24

pickypg requested changes Jun 7, 2018

View reviewed changes

tsullivan commented Jun 8, 2018

View reviewed changes

tsullivan added 9 commits June 8, 2018 10:39

remove cleanup and fetchAfterInit methods

b6ab6aa

improve test for collector_set

f1cca8c

test for bulk_uploader class

acc5719

fix reporting

bf07330

expose collectorSet if there actually is a collectorSet

46e7885

comment for enclosed function

586534a

make collectorSet creation/expose unconditional, bulkUploader more co…

32f79cf

…nditional

fix collector_set tests

1eb28de

tsullivan force-pushed the usage/separate-bulk-behavior-from-collectorset branch from 45c3f1a to 3046ff0 Compare June 8, 2018 21:58

tsullivan added 2 commits June 8, 2018 16:35

lifecycle events

aaa3804

stab at collectorSet error logging from the API call

486c64e

tsullivan force-pushed the usage/separate-bulk-behavior-from-collectorset branch from 3046ff0 to 486c64e Compare June 8, 2018 23:36

tsullivan added review v7.0.0 and removed WIP Work in progress labels Jun 8, 2018

tsullivan commented Jun 8, 2018

View reviewed changes

clean up comments

e796f7f

tsullivan commented Jun 9, 2018

View reviewed changes

init and logging: if / elseif / elseif

c07076c

elastic deleted a comment from elasticmachine Jun 19, 2018

spalger reviewed Jun 20, 2018

View reviewed changes

tsullivan added 7 commits June 20, 2018 11:45

remove unnecessary check/log

03530a4

help log

6805edf

remove redundant, use data.filter.map

b77a43f

Merge branch 'master' into usage/separate-bulk-behavior-from-collecto…

48d1eea

…rset

use xpackInfo.onLicenseInfoChange not xpackMainPlugin.status.on('green')

bd190b7

help logging

19464d6

fix unit test

8d2c2e6

elastic deleted a comment from elasticmachine Jun 20, 2018

spalger suggested changes Jun 21, 2018

View reviewed changes

remove handler that stops upload when connection is lost

64b543c

tsullivan merged commit d0595fd into elastic:master Jun 21, 2018

tsullivan deleted the usage/separate-bulk-behavior-from-collectorset branch June 21, 2018 20:08

tsullivan mentioned this pull request Jun 21, 2018

[6.x] Separate bulk upload behavior from CollectorSet (#19691) #20129

Merged

tsullivan mentioned this pull request Jun 26, 2018

[Telemetry] move x-pack/monitoring collector classes to src/server/usage #20248

Merged

Conversation

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

Uh oh!

Changes

Testing:

Uh oh!

pickypg left a comment

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!

tsullivan Jun 8, 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!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsullivan Jun 9, 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!

tsullivan Jun 9, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsullivan commented Jun 20, 2018

Uh oh!

tsullivan commented Jun 20, 2018

Uh oh!

tsullivan commented Jun 20, 2018

Uh oh!

elasticmachine commented Jun 20, 2018

💚 Build Succeeded

Uh oh!

spalger left a comment

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

tsullivan commented Jun 5, 2018 •

edited

Loading

tsullivan Jun 8, 2018 •

edited

Loading

tsullivan commented Jun 8, 2018 •

edited

Loading

tsullivan commented Jun 8, 2018 •

edited

Loading

tsullivan Jun 9, 2018 •

edited

Loading

tsullivan Jun 9, 2018 •

edited

Loading