[Maps] Enh/gis telemetry #29346

kindsun · 2019-01-25T15:03:52Z

Resolves #26715. Adds telemetry for the following:

Total count of maps
Min, max, avg count of layers per map
Min, max, avg of layers by type
Min, max, avg of layers by EMS region
Min, max, avg of data sources per map

elasticmachine · 2019-01-25T15:47:50Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

… mgr state in usage collector

elasticmachine · 2019-01-29T17:26:56Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-01-29T18:35:43Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-01-29T18:39:42Z

Pinging @elastic/kibana-gis

kindsun · 2019-01-29T18:42:52Z

@alexfrancoeur I'm in the process of writing tests for this PR now, would be good to verify this covers what you requested in #26715. I think the only thing we aren't getting are basemaps since basemap at this point is kind of an informal designation of a tilemap layer. Here's a sample of the saved JSON data currently:

"maps":{
   "maps_total_count":2,
   "attributes_per_map":{
      "data_sources_count":{
         "min":3,
         "max":4,
         "avg":3.5
      },
      "layers_count":{
         "min":3,
         "max":4,
         "avg":3.5
      },
      "layer_types_count":{
         "tile":{
            "min":1,
            "max":2,
            "avg":1.5
         },
         "vector":{
            "min":2,
            "max":2,
            "avg":2
         }
      },
      "ems_vector_layers_count":{
         "world_countries":{
            "min":1,
            "max":1,
            "avg":1
         },
         "canada_provinces":{
            "min":1,
            "max":1,
            "avg":1
         }
      }
   }
}

alexfrancoeur · 2019-01-29T23:29:51Z

😮 @aaronjcaldwell, this is perfect. The data and structure look good to me 👍

elasticmachine · 2019-01-31T01:46:17Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

thomasneirynck

neato, some minor comments

thomasneirynck · 2019-01-31T16:48:32Z

x-pack/plugins/gis/index.js

-        xpackMainPlugin.info
-          .feature(thisPlugin.id)
-          .registerLicenseCheckResultsGenerator(checkLicense);
+      const thisPlugin = this;


no need to reassign, this is used in same lexical scope

thomasneirynck · 2019-01-31T16:53:44Z

x-pack/plugins/gis/server/maps_telemetry/maps_telemetry.js

+    type: 'gis-map'
+  });
+
+  const savedMaps = _.get(gisMapsSavedObject, 'saved_objects')


rename to layerLists

Funny, that's exactly what it was before but I changed it to savedMaps thinking that was more differentiating and clear here. I'll change it back though

thomasneirynck · 2019-01-31T16:57:13Z

x-pack/plugins/gis/server/maps_telemetry/maps_telemetry.js

+  // Count of EMS Vector layers used
+  const emsLayersCount = savedMaps.map(lList => _(lList)
+    .countBy(layer => {
+      const isEmsFile = _.get(layer, 'sourceDescriptor.type') === 'EMS_FILE';


extract this constant EMS_FILE to constant (e.g. in `common/constants.js), so it can be referenced from single spot (right now, constant is defined here: https://github.com/thomasneirynck/kibana/blob/9520a812981bf58f37dfa40758af6e99d1af6a0f/x-pack/plugins/gis/public/shared/layers/sources/ems_file_source/ems_file_source.js#L16)

thomasneirynck · 2019-01-31T16:58:14Z

x-pack/plugins/gis/server/maps_telemetry/maps_usage_collector.js

+import { TASK_ID, scheduleTask, registerMapsTelemetryTask } from './telemetry_task';
+
+export function initTelemetryCollection(server) {
+  const { taskManager } = server;


fluffy line. can just use server.taskManager like you do elsewhere in the file.

thomasneirynck · 2019-01-31T16:58:42Z

x-pack/plugins/gis/server/maps_telemetry/maps_usage_collector.js

+}
+
+export function registerMapsUsageCollector(server) {
+  const { usage } = server;


… to support testing

# Conflicts: # x-pack/plugins/maps/index.js

elasticmachine · 2019-02-01T18:32:31Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-02-01T19:31:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

tsullivan · 2019-02-01T20:05:55Z

x-pack/plugins/maps/server/maps_telemetry/maps_usage_collector.js

+            bool: {
+              filter: {
+                term: {
+                  _id: TASK_ID


taskManager really needs a get method :P

Yeah, that'd be nice 👍

tsullivan · 2019-02-01T20:08:12Z

I'm still looking through it, but just a thought on the output. And this is maybe for @alexfrancoeur

Since the info is coming from a cache in ES, would it make sense to include a field for how old the cache is? Something like maps.cache_age_ms ?

I haven't done it for Telemetry for visualizations by type, but I kind of wish I brought up this quesiton then.

tsullivan · 2019-02-01T20:27:36Z

x-pack/plugins/maps/index.js

+          if (license && license.gis && !routesInitialized) {
+            routesInitialized = true;
+            initRoutes(server, license.uid);
+            initTelemetryCollection(server);


It might not be necessary to keep initTelemetryCollection waiting for xpack_info license information, or the routesInitialized

As long as scheduling the built-in task waits for afterPluginsInit, which it does: https://github.com/elastic/kibana/pull/29346/files#diff-25f6c55fe3ca39b275e87048eb946ca8R16 I think you'd be able to call initTelemetryCollection directly from init

I was just thinking there was no point in registering for maps telemetry if there are no maps. Just one less thing to poll for the telemetry service. Do you disagree?

The reasoning sounds fine. I'd be a little cautious about the timing of when your afterPluginsInit declaration. If that happens too late in the server init, the Kibana server may have already processed all the callbacks in its queue: https://github.com/elastic/kibana/blob/f0cc432/src/server/plugins/wait_for_plugins_init.js

Looks like there would be an error thrown if that happens, but to be safe, I'd put that in up-front setup logic

thomasneirynck

awesome. don't know enough about telemetry itself, thanks @tsullivan for verifying that part

lgtm, but small suggestion for cleanup, while we're in the code. remove gis-* usage too.

thomasneirynck · 2019-02-01T21:30:55Z

x-pack/plugins/maps/index.js

-        xpackMainPlugin.info
-          .feature(thisPlugin.id)
-          .registerLicenseCheckResultsGenerator(checkLicense);
+      const thisPlugin = this;


thisPlugin variable is not necessary

thomasneirynck · 2019-02-01T21:38:14Z

x-pack/plugins/maps/server/maps_telemetry/maps_telemetry.js

+
+async function getSavedObjects(savedObjectsClient) {
+  const gisMapsSavedObject = await savedObjectsClient.find({
+    type: 'gis-map'


needs to be map

nice catch!

alexfrancoeur · 2019-02-01T22:08:46Z

@tsullivan I hadn't thought about that, definitely not opposed to introducing this metric. As long as it doesn't impact delivery time for FF 😄

# Conflicts: # x-pack/plugins/maps/index.js

kindsun · 2019-02-04T17:01:07Z

@alexfrancoeur @tsullivan I've added a timeCaptured field to the telemetry. I figured that would cover what we're looking for and prevent an unnecessary date difference calculation on every polling cycle (i.e.- every few seconds).

elasticmachine · 2019-02-04T17:40:22Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-02-04T18:42:28Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

kindsun · 2019-02-04T18:59:54Z

retest

elasticmachine · 2019-02-04T19:41:58Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

alexfrancoeur · 2019-02-04T20:01:20Z

Sounds good, thanks @aaronjcaldwell!

tsullivan

Offered some food for thought about setup logic, but take it as you like :)

This code LGTM: I reviewed the diffs only

tsullivan · 2019-02-04T21:52:54Z

x-pack/plugins/maps/index.js


 export function maps(kibana) {

  return new kibana.Plugin({
-    require: ['kibana', 'elasticsearch', 'xpack_main', 'tile_map'],
+    require: ['kibana', 'elasticsearch', 'xpack_main', 'tile_map', 'task_manager'],


Hi, just to let you know, if someone needed to disable task manager, they would also have to disable the Maps plugin.

You could avoid that by putting the task stuff in x-pack/plugins/oss_telemetry

After some offline conversation and some code digging, I've decided to leave this as is for now with the understanding that we may need to rethink this dependency in the future. There shouldn't be a timing issue as the plugin spec requires dependencies be both enabled and initialized (

kibana/src/plugin_discovery/plugin_spec/plugin_spec.js

Line 35 in 94d99d6

* @param {Array} [opts.require] the other plugins that this plugin

) however there is the issue of disabling the task manager effectively disabling the map. It may be possible to do a conditional require, however the condition would usually be on xpack.task_manager.enabled which is obtained from the server config at init time, trying to get this info at the point of require feels hacky. This might be a bigger issue of how to elegantly handle conditional requires in a broader way moving forward and could likely be tackled in the plugin spec.

# Conflicts: # x-pack/plugins/maps/index.js

elasticmachine · 2019-02-05T04:36:08Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-02-05T16:43:11Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Add basic framework for maps telemetry gathering * Update mappings to cover required maps usage metrics * Update telemetry to harvest from saved maps * Consolidate. Clean up * Harvest telemetry daily at midnight using task manager. Retrieve task mgr state in usage collector * Organize, clean up * Reorg * Add usage collector tests. Add test utils * Add maps telemetry tests to verify values. Multiple structure changes to support testing * Review feedback * More review feedback * Add capture date/time to telemetry * Change date to 'type: date' * Review feedback * Fix merge issue

* [Maps] Enh/gis telemetry (#29346) * Add basic framework for maps telemetry gathering * Update mappings to cover required maps usage metrics * Update telemetry to harvest from saved maps * Consolidate. Clean up * Harvest telemetry daily at midnight using task manager. Retrieve task mgr state in usage collector * Organize, clean up * Reorg * Add usage collector tests. Add test utils * Add maps telemetry tests to verify values. Multiple structure changes to support testing * Review feedback * More review feedback * Add capture date/time to telemetry * Change date to 'type: date' * Review feedback * Fix merge issue * Update telemetry test to account for maps

kindsun added 5 commits January 18, 2019 15:00

Add basic framework for maps telemetry gathering

4e3c22c

Merge remote-tracking branch 'upstream/master' into enh/gisTelemetry

3330518

Update mappings to cover required maps usage metrics

1e67ebc

Update telemetry to harvest from saved maps

06a122a

Consolidate. Clean up

aef2b52

Merge remote-tracking branch 'upstream/master' into enh/gisTelemetry

baaf22d

thomasneirynck changed the title ~~Enh/gis telemetry~~ [Maps] Enh/gis telemetry Jan 28, 2019

Harvest telemetry daily at midnight using task manager. Retrieve task…

475580b

… mgr state in usage collector

Organize, clean up

bb4e105

kindsun added enhancement New value added to drive a business result v7.0.0 v6.7.0 [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation labels Jan 29, 2019

kindsun added the Feature:Telemetry label Jan 29, 2019

Reorg

6d0f932

kindsun added 2 commits January 30, 2019 09:33

Merge remote-tracking branch 'upstream/master' into enh/gisTelemetry

7579bcd

Add usage collector tests. Add test utils

b0e2152

thomasneirynck reviewed Jan 31, 2019

View reviewed changes

kindsun added 3 commits January 31, 2019 10:27

Merge remote-tracking branch 'upstream/master' into enh/gisTelemetry

9c01a21

Add maps telemetry tests to verify values. Multiple structure changes…

929907e

… to support testing

Merge remote-tracking branch 'upstream/master' into enh/gisTelemetry

f6c63d1

# Conflicts: # x-pack/plugins/maps/index.js

Review feedback

e15e00e

kindsun requested a review from tsullivan February 1, 2019 18:53

tsullivan reviewed Feb 1, 2019

View reviewed changes

thomasneirynck reviewed Feb 1, 2019

View reviewed changes

kindsun added 3 commits February 4, 2019 09:11

Merge remote-tracking branch 'upstream/master' into enh/gisTelemetry

cf97d4a

# Conflicts: # x-pack/plugins/maps/index.js

More review feedback

0859ca4

Add capture date/time to telemetry

fdd66bf

Change date to 'type: date'

d9ab073

tsullivan approved these changes Feb 4, 2019

View reviewed changes

tsullivan reviewed Feb 4, 2019

View reviewed changes

kindsun added 2 commits February 4, 2019 18:10

Review feedback

9a8fc37

Merge remote-tracking branch 'upstream/master' into enh/gisTelemetry

20742b8

# Conflicts: # x-pack/plugins/maps/index.js

Fix merge issue

7a1aab4

kindsun merged commit 55e7b18 into elastic:master Feb 5, 2019

kindsun mentioned this pull request Feb 5, 2019

[6.x] [Maps] Enh/gis telemetry (#29346) #30099

Merged

alexfrancoeur mentioned this pull request Mar 1, 2019

[Maps] Telemetry on custom layers #32316

Closed

[Maps] Enh/gis telemetry #29346

[Maps] Enh/gis telemetry #29346

Conversation

kindsun commented Jan 25, 2019

elasticmachine commented Jan 25, 2019

💔 Build Failed

elasticmachine commented Jan 29, 2019

💔 Build Failed

elasticmachine commented Jan 29, 2019

💔 Build Failed

elasticmachine commented Jan 29, 2019

kindsun commented Jan 29, 2019 • edited Loading

alexfrancoeur commented Jan 29, 2019 • edited Loading

elasticmachine commented Jan 31, 2019

💚 Build Succeeded

thomasneirynck 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Feb 1, 2019

💔 Build Failed

elasticmachine commented Feb 1, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsullivan commented Feb 1, 2019

tsullivan Feb 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasneirynck 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

alexfrancoeur commented Feb 1, 2019

kindsun commented Feb 4, 2019

elasticmachine commented Feb 4, 2019

💚 Build Succeeded

elasticmachine commented Feb 4, 2019

💔 Build Failed

kindsun commented Feb 4, 2019

elasticmachine commented Feb 4, 2019

💚 Build Succeeded

alexfrancoeur commented Feb 4, 2019

tsullivan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Feb 5, 2019

💔 Build Failed

elasticmachine commented Feb 5, 2019

💚 Build Succeeded

kindsun commented Jan 29, 2019 •

edited

Loading

alexfrancoeur commented Jan 29, 2019 •

edited

Loading

tsullivan Feb 1, 2019 •

edited

Loading