Retrofit the Bulk Uploader types combiner [ch2198] #22030

tsullivan · 2018-08-15T19:19:33Z

Replaces #21356
Resolves #21238

This PR allows the bulk uploader to handle new types of data without needing to update any legacy code. It helps remove future maintenance on monitoring/server/kibana_monitoring, i.e. internal monitoring stats upload.

Changes:

Flag src/server/status/collectors/get_ops_stats_collector.js with internalIgnore: true to make it used only for the HTTP stats API. Bulk Uploader uses a different ops stats collector that's going to stay in Monitoring.
Give collector instances a formatForBulkUpload method, which allows them to customize how they get combined into the payload for internal data upload. Customizations are optional, default will work most of the time.
- Add formatForBulkUpload methods to the collectors created ingetKibanaUsageCollector and getReportingUsageCollector
Add getFilteredCollectorSet method to the collectorSet instance, to get a collectorSet instance that has only one kind of collector registered.
Remove BulkUploader.combineStatsLegacy

To test:

Index .monitoring-kibana-* data when running master
Index .monitoring-kibana-* data when running this branch
Compare 2 documents created with the different branches; they should have the same shape. http://jsondiff.com/ helps compare

Look at the kibana_stats and kibana_settings documents when comparing

fix usage collector, add comments to formatForBulk remove unnecessary customizations

tsullivan · 2018-08-16T00:23:40Z

src/server/usage/classes/collector.js

    this.type = type;
    this.init = init;
    this.fetch = fetch;

-    this.log = getCollectorLogger(server);
+    const defaultFormatterForBulkUpload = result => ({ type, payload: result });
+    this._formatForBulkUpload = formatForBulkUpload || defaultFormatterForBulkUpload;


I'm doing some testing, and it looks like I want to override this in the UsageCollector class. Usage collectors should organize their data in the kibana_stats type in the usage namespace. - c940ee1

Ah, that's snazzy!

elasticmachine · 2018-08-16T02:26:21Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

chrisronline · 2018-08-17T16:04:22Z

So when I run master for kibana_settings, I don't see the xpack: { default_admin_email } in the document (I don't have one set FYI). However, in your branch, I do see one.

Is that expected?

tsullivan · 2018-08-17T16:34:29Z

However, in your branch, I do see one.

I'm guessing you see the value as null. That is expected to happen once when Kibana first starts up.

The issue of seeing no value for default_admin_email is addressed in #22091, because that shouldn't be happening.

chrisronline · 2018-08-17T16:40:36Z

@tsullivan So I should ignore that difference right now? Because the two documents (from master and your branch) are not identical

elasticmachine · 2018-08-17T17:45:10Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

tsullivan · 2018-08-17T23:13:50Z

@chrisronline I merged in master after the blocker bug fix, please take another look at the settings collector, because I had to resolve a conflict there.

Let me know if you are still seeing differences in the shape of the data.

elasticmachine · 2018-08-18T00:50:58Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

chrisronline · 2018-08-20T13:27:53Z

@tsullivan Great, functionally this LGTM! Going to review the code now

chrisronline · 2018-08-20T13:35:28Z

src/server/status/collectors/get_ops_stats_collector.js

@@ -44,6 +44,7 @@ export function getOpsStatsCollector(server, kbnServer) {
        kibana: getKibanaInfoForStats(server, kbnServer),
        ...kbnServer.metrics // latest metrics captured from the ops event listener in src/server/status/index
      };
-    }
+    },
+    internalIgnore: true, // Ignore this one from internal uploader. A different stats collector is used there.


I'm wondering if this can be more verbose to avoid ambiguity - maybe ignoreForInternalUploader?

chrisronline · 2018-08-20T13:39:15Z

src/server/usage/classes/collector_set.js

@@ -73,12 +72,12 @@ export class CollectorSet {
   * Call a bunch of fetch methods and then do them in bulk
   * @param {Array} collectors - an array of collectors, default to all registered collectors
   */
-  bulkFetch(callCluster, collectors = this._collectors) {
-    if (!Array.isArray(collectors)) {
+  bulkFetch(callCluster, collectors = this) {


Should we rename collectors to collectorSet to avoid any potential confusion? By collectors, I'd assume we're doing with an array of collectors versus a typed CollectorSet object

chrisronline · 2018-08-20T13:40:06Z

src/server/usage/classes/collector_set.js

@@ -90,10 +89,19 @@ export class CollectorSet {
          this._log.warn(`Unable to fetch data from ${collectorType} collector`);
        });
    });
+    return Promise.all(fetchPromises);


Just wondering, is there a specific reason we keep using promises versus using async/await?

Not really. This lets us not await each fetch one at a time. I could be wrong, but using Promise.props inside the loop has it run all the fetches at the same time.

chrisronline · 2018-08-20T13:40:54Z

src/server/usage/classes/collector_set.js


    /*
     * Helper Factory methods
     * Define as instance properties to allow enclosing the server object
     */
    this.makeStatsCollector = options => new Collector(server, options);
    this.makeUsageCollector = options => new UsageCollector(server, options);
+    this._makeCollectorSetFromArray = collectorsArray => new CollectorSet(server, collectorsArray);


Why is this an instanced variable versus just static function defined at the top?

Having it here let's us enclose the server object that the constructor is called with

chrisronline · 2018-08-20T13:44:12Z

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

+     */
+    formatForBulkUpload: result => {
+      return {
+        type: 'kibana_stats',


Should we use the constants here?

Yeah, this can use KIBANA_STATS_TYPE_MONITORING from the monitoring constants.

chrisronline · 2018-08-20T13:44:50Z

x-pack/plugins/reporting/server/usage/get_reporting_usage_collector.js

+     */
+    formatForBulkUpload: result => {
+      return {
+        type: 'kibana_stats',


Ditto on the constant

chrisronline · 2018-08-20T13:45:46Z

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

+
+    // convert the raw data to a nested object by taking each payload through
+    // its formatter, organizing it per-type
+    const typesNested = rawData.reduce((accum, { type, result }) => {


Can we include a sample of what the data structure looks like before and after each transformation? I find that helps understand intent a lot

chrisronline

Overall, looking great! I had a few questions and suggestions!

elasticmachine · 2018-08-20T22:18:30Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

chrisronline

LGTM! Tested and works great!

* Retrofit the Bulk Uploader types combiner [ch2198] fix usage collector, add comments to formatForBulk remove unnecessary customizations * override default format for bulk upload for usage type collectors * rename to ignoreForInternalUploader * collectors -> collectorSet * use constant for kibana_stats type * example of data formatting for bulk in function comment

tsullivan · 2018-08-21T19:30:37Z

6.5.0/6.x: #22223

* Retrofit the Bulk Uploader types combiner [ch2198] fix usage collector, add comments to formatForBulk remove unnecessary customizations * override default format for bulk upload for usage type collectors * rename to ignoreForInternalUploader * collectors -> collectorSet * use constant for kibana_stats type * example of data formatting for bulk in function comment

Closes https://github.com/elastic/kibana-canvas/issues/128 Makes a summary of Canvas usage that looks like: ``` { "kibana_canvas": { "elements": { "per_page": { "avg": 2, "max": 3, "min": 1 }, "total": 4 }, "functions": { "in_use": [ "filters", "demodata", "markdown", "render", "image", "pointseries", "plot", "getCell", "repeatImage" ], "per_element": { "avg": 4, "max": 5, "min": 2 }, "total": 16 }, "pages": { "per_workpad": { "avg": 1, "max": 1, "min": 1 }, "total": 2 }, "workpads": { "total": 2 } } } ``` To complete end-to-end Telemetry functionality, More work is going on with Monitoring and Telemetry to have every registered collector automatically include the their data in the overall payload. There's ongoing work to implement that in core Kibana, but none of it blocks the PR from being merged. - Having the Canvas usage stats automatically added to `.monitoring-kibana-*` documents depends on this PR: elastic#22030 - That PR can be pulled to help test this PR. The result will be Canvas usage stats in the `kibana_stats` documents in `.monitoring-kibana-*`. - Having Canvas usage stats automatically included in the telemetry payload that gets sent to Elastic depends on this unstarted issue: elastic#21239 - Without that additional work, the way to test this PR is to check for the `usage.kibana_canvas` data in the`localhost:5601/api/stats?extended=true` API response. That pending work in Kibana core does not block this PR from getting merged.

tsullivan added v7.0.0 Team:Monitoring Stack Monitoring team labels Aug 15, 2018

tsullivan force-pushed the monitoring/collector-format branch from 568949f to 0305d48 Compare August 15, 2018 21:30

tsullivan changed the title ~~[WIP]~~ Retrofit the Bulk Uploader types combiner [ch2198] Aug 15, 2018

elastic deleted a comment from elasticmachine Aug 15, 2018

tsullivan changed the title ~~Retrofit the Bulk Uploader types combiner [ch2198]~~ [WIP] Retrofit the Bulk Uploader types combiner [ch2198] Aug 15, 2018

tsullivan force-pushed the monitoring/collector-format branch 3 times, most recently from eb78627 to 8778c55 Compare August 15, 2018 22:12

tsullivan added review v6.5.0 labels Aug 15, 2018

tsullivan requested a review from chrisronline August 15, 2018 22:21

tsullivan changed the title ~~[WIP] Retrofit the Bulk Uploader types combiner [ch2198]~~ Retrofit the Bulk Uploader types combiner [ch2198] Aug 15, 2018

tsullivan force-pushed the monitoring/collector-format branch from 8778c55 to 4597441 Compare August 15, 2018 22:42

Retrofit the Bulk Uploader types combiner [ch2198]

fb76429

fix usage collector, add comments to formatForBulk remove unnecessary customizations

tsullivan force-pushed the monitoring/collector-format branch from 4597441 to fb76429 Compare August 15, 2018 23:12

elastic deleted a comment from elasticmachine Aug 15, 2018

override default format for bulk upload for usage type collectors

c940ee1

tsullivan commented Aug 16, 2018

View reviewed changes

chrisronline closed this Aug 17, 2018

chrisronline reopened this Aug 17, 2018

Merge branch 'master' into monitoring/collector-format

ee1fa5d

chrisronline reviewed Aug 20, 2018

View reviewed changes

chrisronline suggested changes Aug 20, 2018

View reviewed changes

tsullivan added 5 commits August 20, 2018 13:10

Merge branch 'master' into monitoring/collector-format

5a7525a

rename to ignoreForInternalUploader

5213838

collectors -> collectorSet

7f18337

use constant for kibana_stats type

3a11d4f

example of data formatting for bulk in function comment

d4b73c7

chrisronline approved these changes Aug 21, 2018

View reviewed changes

tsullivan merged commit 8718d1e into elastic:master Aug 21, 2018

tsullivan deleted the monitoring/collector-format branch August 21, 2018 19:29

tsullivan mentioned this pull request Aug 21, 2018

[6.x] Retrofit the Bulk Uploader types combiner [ch2198] (#22030) #22223

Merged

tsullivan mentioned this pull request Aug 21, 2018

[Monitoring] Retrofit the Bulk Uploader types combiner to require less maintenance as metrics/telemetry KPIs grow #21238

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retrofit the Bulk Uploader types combiner [ch2198] #22030

Retrofit the Bulk Uploader types combiner [ch2198] #22030

tsullivan commented Aug 15, 2018 •

edited

Loading

tsullivan Aug 16, 2018

chrisronline Aug 20, 2018

elasticmachine commented Aug 16, 2018

chrisronline commented Aug 17, 2018

tsullivan commented Aug 17, 2018

chrisronline commented Aug 17, 2018

elasticmachine commented Aug 17, 2018

tsullivan commented Aug 17, 2018

elasticmachine commented Aug 18, 2018

chrisronline commented Aug 20, 2018

chrisronline Aug 20, 2018

chrisronline Aug 20, 2018

chrisronline Aug 20, 2018

tsullivan Aug 20, 2018

chrisronline Aug 20, 2018 •

edited

Loading

tsullivan Aug 20, 2018

chrisronline Aug 20, 2018

tsullivan Aug 20, 2018

chrisronline Aug 20, 2018

chrisronline Aug 20, 2018

chrisronline left a comment

elasticmachine commented Aug 20, 2018

chrisronline left a comment

tsullivan commented Aug 21, 2018

Retrofit the Bulk Uploader types combiner [ch2198] #22030

Retrofit the Bulk Uploader types combiner [ch2198] #22030

Conversation

tsullivan commented Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Aug 16, 2018

💚 Build Succeeded

chrisronline commented Aug 17, 2018

tsullivan commented Aug 17, 2018

chrisronline commented Aug 17, 2018

elasticmachine commented Aug 17, 2018

💚 Build Succeeded

tsullivan commented Aug 17, 2018

elasticmachine commented Aug 18, 2018

💚 Build Succeeded

chrisronline commented Aug 20, 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

chrisronline Aug 20, 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

chrisronline left a comment

Choose a reason for hiding this comment

elasticmachine commented Aug 20, 2018

💚 Build Succeeded

chrisronline left a comment

Choose a reason for hiding this comment

tsullivan commented Aug 21, 2018

tsullivan commented Aug 15, 2018 •

edited

Loading

chrisronline Aug 20, 2018 •

edited

Loading