From dcb67010eeb73f2fb7fce64cdf3384a3c88b535e Mon Sep 17 00:00:00 2001 From: Aaron Caldwell Date: Mon, 6 Jan 2020 13:05:07 -0700 Subject: [PATCH] [Maps] Fix regression preventing maps telemetry from populating & remove task manager logic (#52834) * Remove task logic. Remove server refs and revise for np. Migrate a few files to ts * Remove unused reference * Update mappings * Test usage collector register * Update api integration tests to include maps now that telemetry is 'normal' (not using task mgr state) * Update integration test to use stack stats * Update integration test to look for 'maps-telemetry' instead of 'maps' * Update jest test to reflect calls to register * Follow the same pattern as other int tests and test reliable nested attribute * Back out np-related changes for separate PR * timeCaptured hasn't changed but for some reason stopped working. Getting iso string fixes issue * Back out file shuffling for separate PR * Remove mappings updates (handled in separate PR) * Review feedback. Move telemetry type constant to constants file * Consolidate imports * Linting fix Co-authored-by: Elastic Machine --- .../legacy/plugins/maps/common/constants.js | 1 + .../server/maps_telemetry/maps_telemetry.js | 15 ++- .../maps_telemetry/maps_usage_collector.js | 88 ++---------- .../maps_usage_collector.test.js | 66 +++------ .../server/maps_telemetry/telemetry_task.js | 125 ------------------ .../maps_telemetry/telemetry_task.test.js | 68 ---------- .../apis/telemetry/telemetry_local.js | 4 + 7 files changed, 45 insertions(+), 322 deletions(-) delete mode 100644 x-pack/legacy/plugins/maps/server/maps_telemetry/telemetry_task.js delete mode 100644 x-pack/legacy/plugins/maps/server/maps_telemetry/telemetry_task.test.js diff --git a/x-pack/legacy/plugins/maps/common/constants.js b/x-pack/legacy/plugins/maps/common/constants.js index 4172bffb1cf24bd..44f87949889cbaf 100644 --- a/x-pack/legacy/plugins/maps/common/constants.js +++ b/x-pack/legacy/plugins/maps/common/constants.js @@ -22,6 +22,7 @@ export const EMS_TILES_VECTOR_TILE_PATH = 'ems/tiles/vector/tile'; export const MAP_SAVED_OBJECT_TYPE = 'map'; export const APP_ID = 'maps'; export const APP_ICON = 'gisApp'; +export const TELEMETRY_TYPE = 'maps-telemetry'; export const MAP_APP_PATH = `app/${APP_ID}`; export const GIS_API_PATH = `api/${APP_ID}`; diff --git a/x-pack/legacy/plugins/maps/server/maps_telemetry/maps_telemetry.js b/x-pack/legacy/plugins/maps/server/maps_telemetry/maps_telemetry.js index ae54005238dae52..06c9fa6d810afa8 100644 --- a/x-pack/legacy/plugins/maps/server/maps_telemetry/maps_telemetry.js +++ b/x-pack/legacy/plugins/maps/server/maps_telemetry/maps_telemetry.js @@ -5,10 +5,15 @@ */ import _ from 'lodash'; -import { EMS_FILE, MAP_SAVED_OBJECT_TYPE } from '../../common/constants'; +import { + EMS_FILE, + MAP_SAVED_OBJECT_TYPE, + TELEMETRY_TYPE, +} from '../../common/constants'; -function getSavedObjectsClient(server, callCluster) { +function getSavedObjectsClient(server) { const { SavedObjectsClient, getSavedObjectsRepository } = server.savedObjects; + const callCluster = server.plugins.elasticsearch.getCluster('admin').callWithInternalUser; const internalRepository = getSavedObjectsRepository(callCluster); return new SavedObjectsClient(internalRepository); } @@ -62,7 +67,7 @@ export function buildMapsTelemetry(savedObjects) { // Total count of maps mapsTotalCount: mapsCount, // Time of capture - timeCaptured: new Date(), + timeCaptured: new Date().toISOString(), attributesPerMap: { // Count of data sources per map dataSourcesCount: { @@ -101,8 +106,8 @@ export async function getMapsTelemetry(server, callCluster) { const savedObjects = await getSavedObjects(savedObjectsClient); const mapsTelemetry = buildMapsTelemetry(savedObjects); - return await savedObjectsClient.create('maps-telemetry', mapsTelemetry, { - id: 'maps-telemetry', + return await savedObjectsClient.create(TELEMETRY_TYPE, mapsTelemetry, { + id: TELEMETRY_TYPE, overwrite: true, }); } diff --git a/x-pack/legacy/plugins/maps/server/maps_telemetry/maps_usage_collector.js b/x-pack/legacy/plugins/maps/server/maps_telemetry/maps_usage_collector.js index 8e50a24d8027c1f..9c575e66f75567c 100644 --- a/x-pack/legacy/plugins/maps/server/maps_telemetry/maps_usage_collector.js +++ b/x-pack/legacy/plugins/maps/server/maps_telemetry/maps_usage_collector.js @@ -4,85 +4,19 @@ * you may not use this file except in compliance with the Elastic License. */ -import _ from 'lodash'; -import { TASK_ID, scheduleTask, registerMapsTelemetryTask } from './telemetry_task'; +import { getMapsTelemetry } from './maps_telemetry'; +import { TELEMETRY_TYPE } from '../../common/constants'; -export function initTelemetryCollection(server) { - registerMapsTelemetryTask(server); - scheduleTask(server); - registerMapsUsageCollector(server); -} - -async function isTaskManagerReady(server) { - const result = await fetch(server); - return result !== null; -} - -async function fetch(server) { - let docs; - const taskManager = server.plugins.task_manager; - - if (!taskManager) { - return null; - } - - try { - ({ docs } = await taskManager.fetch({ - query: { - bool: { - filter: { - term: { - _id: `task:${TASK_ID}`, - }, - }, - }, - }, - })); - } catch (err) { - const errMessage = err && err.message ? err.message : err.toString(); - /* - * The usage service WILL to try to fetch from this collector before the task manager has been initialized, because the task manager - * has to wait for all plugins to initialize first. - * It's fine to ignore it as next time around it will be initialized (or it will throw a different type of error) - */ - if (errMessage.indexOf('NotInitialized') >= 0) { - return null; - } else { - throw err; - } +export function initTelemetryCollection(usageCollection, server) { + if (!usageCollection) { + return; } - return docs; -} - -export function buildCollectorObj(server) { - let isCollectorReady = false; - async function determineIfTaskManagerIsReady() { - let isReady = false; - try { - isReady = await isTaskManagerReady(server); - } catch (err) {} // eslint-disable-line - - if (isReady) { - isCollectorReady = true; - } else { - setTimeout(determineIfTaskManagerIsReady, 500); - } - } - determineIfTaskManagerIsReady(); - - return { - type: 'maps', - isReady: () => isCollectorReady, - fetch: async () => { - const docs = await fetch(server); - return _.get(docs, '[0].state.stats'); - }, - }; -} + const mapsUsageCollector = usageCollection.makeUsageCollector({ + type: TELEMETRY_TYPE, + isReady: () => true, + fetch: async () => await getMapsTelemetry(server), + }); -export function registerMapsUsageCollector(server) { - const collectorObj = buildCollectorObj(server); - const mapsUsageCollector = server.usage.collectorSet.makeUsageCollector(collectorObj); - server.usage.collectorSet.register(mapsUsageCollector); + usageCollection.registerCollector(mapsUsageCollector); } diff --git a/x-pack/legacy/plugins/maps/server/maps_telemetry/maps_usage_collector.test.js b/x-pack/legacy/plugins/maps/server/maps_telemetry/maps_usage_collector.test.js index 727d60b5088aaf1..c5a3fca89b5608b 100644 --- a/x-pack/legacy/plugins/maps/server/maps_telemetry/maps_usage_collector.test.js +++ b/x-pack/legacy/plugins/maps/server/maps_telemetry/maps_usage_collector.test.js @@ -4,60 +4,32 @@ * you may not use this file except in compliance with the Elastic License. */ -import sinon from 'sinon'; -import { getMockCallWithInternal, getMockKbnServer, getMockTaskFetch } from '../test_utils'; -import { buildCollectorObj } from './maps_usage_collector'; +import { initTelemetryCollection } from './maps_usage_collector'; describe('buildCollectorObj#fetch', () => { - let mockKbnServer; + let makeUsageCollectorStub; + let registerStub; + let usageCollection; beforeEach(() => { - mockKbnServer = getMockKbnServer(); + makeUsageCollectorStub = jest.fn(); + registerStub = jest.fn(); + usageCollection = { + makeUsageCollector: makeUsageCollectorStub, + registerCollector: registerStub, + }; }); - test('can return empty stats', async () => { - const { type, fetch } = buildCollectorObj(mockKbnServer); - expect(type).toBe('maps'); - const fetchResult = await fetch(); - expect(fetchResult).toEqual({}); - }); - - test('provides known stats', async () => { - const mockTaskFetch = getMockTaskFetch([ - { - state: { - runs: 2, - stats: { wombat_sightings: { total: 712, max: 84, min: 7, avg: 63 } }, - }, - }, - ]); - mockKbnServer = getMockKbnServer(getMockCallWithInternal(), mockTaskFetch); - - const { type, fetch } = buildCollectorObj(mockKbnServer); - expect(type).toBe('maps'); - const fetchResult = await fetch(); - expect(fetchResult).toEqual({ wombat_sightings: { total: 712, max: 84, min: 7, avg: 63 } }); - }); - - describe('Error handling', () => { - test('Silently handles Task Manager NotInitialized', async () => { - const mockTaskFetch = sinon.stub(); - mockTaskFetch.rejects( - new Error('NotInitialized taskManager is still waiting for plugins to load') - ); - mockKbnServer = getMockKbnServer(getMockCallWithInternal(), mockTaskFetch); - - const { fetch } = buildCollectorObj(mockKbnServer); - await expect(fetch()).resolves.toBe(undefined); - }); - // In real life, the CollectorSet calls fetch and handles errors - test('defers the errors', async () => { - const mockTaskFetch = sinon.stub(); - mockTaskFetch.rejects(new Error('Sad violin')); - mockKbnServer = getMockKbnServer(getMockCallWithInternal(), mockTaskFetch); + test('makes and registers maps usage collector', async () => { + const serverPlaceholder = {}; + initTelemetryCollection(usageCollection, serverPlaceholder); - const { fetch } = buildCollectorObj(mockKbnServer); - await expect(fetch()).rejects.toMatchObject(new Error('Sad violin')); + expect(registerStub).toHaveBeenCalledTimes(1); + expect(makeUsageCollectorStub).toHaveBeenCalledTimes(1); + expect(makeUsageCollectorStub).toHaveBeenCalledWith({ + type: expect.any(String), + isReady: expect.any(Function), + fetch: expect.any(Function), }); }); }); diff --git a/x-pack/legacy/plugins/maps/server/maps_telemetry/telemetry_task.js b/x-pack/legacy/plugins/maps/server/maps_telemetry/telemetry_task.js deleted file mode 100644 index a0997cb2dbe8fe5..000000000000000 --- a/x-pack/legacy/plugins/maps/server/maps_telemetry/telemetry_task.js +++ /dev/null @@ -1,125 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ - -import { getMapsTelemetry } from './maps_telemetry'; - -const TELEMETRY_TASK_TYPE = 'maps_telemetry'; - -export const TASK_ID = `Maps-${TELEMETRY_TASK_TYPE}`; - -export function scheduleTask(server) { - const taskManager = server.plugins.task_manager; - - if (!taskManager) { - server.log(['debug', 'telemetry'], `Task manager is not available`); - return; - } - - const { kbnServer } = server.plugins.xpack_main.status.plugin; - - kbnServer.afterPluginsInit(() => { - // The code block below can't await directly within "afterPluginsInit" - // callback due to circular dependency. The server isn't "ready" until - // this code block finishes. Migrations wait for server to be ready before - // executing. Saved objects repository waits for migrations to finish before - // finishing the request. To avoid this, we'll await within a separate - // function block. - (async () => { - try { - await taskManager.schedule({ - id: TASK_ID, - taskType: TELEMETRY_TASK_TYPE, - state: { stats: {}, runs: 0 }, - }); - } catch (e) { - server.log(['warning', 'maps'], `Error scheduling telemetry task, received ${e.message}`); - } - })(); - }); -} - -export function registerMapsTelemetryTask(server) { - const taskManager = server.plugins.task_manager; - - if (!taskManager) { - server.log(['debug', 'telemetry'], `Task manager is not available`); - return; - } - - taskManager.registerTaskDefinitions({ - [TELEMETRY_TASK_TYPE]: { - title: 'Maps telemetry fetch task', - type: TELEMETRY_TASK_TYPE, - timeout: '1m', - createTaskRunner: telemetryTaskRunner(server), - }, - }); -} - -export function telemetryTaskRunner(server) { - return ({ taskInstance }) => { - const { state } = taskInstance; - const prevState = state; - - const callCluster = server.plugins.elasticsearch.getCluster('admin').callWithInternalUser; - - let mapsTelemetryTask; - - return { - async run({ taskCanceled = false } = {}) { - try { - mapsTelemetryTask = makeCancelable(getMapsTelemetry(server, callCluster), taskCanceled); - } catch (err) { - server.log(['warning'], `Error loading maps telemetry: ${err}`); - } finally { - return mapsTelemetryTask.promise - .then((mapsTelemetry = {}) => { - return { - state: { - runs: state.runs || 0 + 1, - stats: mapsTelemetry.attributes || prevState.stats || {}, - }, - runAt: getNextMidnight(), - }; - }) - .catch(errMsg => - server.log(['warning'], `Error executing maps telemetry task: ${errMsg}`) - ); - } - }, - async cancel() { - if (mapsTelemetryTask) { - mapsTelemetryTask.cancel(); - } else { - server.log(['warning'], `Can not cancel "mapsTelemetryTask", it has not been defined`); - } - }, - }; - }; -} - -function makeCancelable(promise, isCanceled) { - const logMsg = 'Maps telemetry task has been cancelled'; - const wrappedPromise = new Promise((resolve, reject) => { - promise - .then(val => (isCanceled ? reject(logMsg) : resolve(val))) - .catch(err => (isCanceled ? reject(logMsg) : reject(err.message))); - }); - - return { - promise: wrappedPromise, - cancel() { - isCanceled = true; - }, - }; -} - -function getNextMidnight() { - const nextMidnight = new Date(); - nextMidnight.setHours(0, 0, 0, 0); - nextMidnight.setDate(nextMidnight.getDate() + 1); - return nextMidnight; -} diff --git a/x-pack/legacy/plugins/maps/server/maps_telemetry/telemetry_task.test.js b/x-pack/legacy/plugins/maps/server/maps_telemetry/telemetry_task.test.js deleted file mode 100644 index ad23ed16342040c..000000000000000 --- a/x-pack/legacy/plugins/maps/server/maps_telemetry/telemetry_task.test.js +++ /dev/null @@ -1,68 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ - -import { getMockKbnServer, getMockTaskInstance } from '../test_utils'; -import { telemetryTaskRunner } from './telemetry_task'; -import * as mapsTelemetry from './maps_telemetry'; -jest.mock('./maps_telemetry'); - -const expectedAttributes = { - expect: 'values', - toBe: 'populated', -}; - -const generateTelemetry = ({ includeAttributes = true } = {}) => { - mapsTelemetry.getMapsTelemetry = async () => ({ // eslint-disable-line - attributes: includeAttributes ? expectedAttributes : {}, - }); -}; - -describe('telemetryTaskRunner', () => { - let mockTaskInstance; - let mockKbnServer; - let taskRunner; - - beforeEach(() => { - mockTaskInstance = getMockTaskInstance(); - mockKbnServer = getMockKbnServer(); - taskRunner = telemetryTaskRunner(mockKbnServer)({ taskInstance: mockTaskInstance }); - }); - - test('returns empty stats as default', async () => { - generateTelemetry({ includeAttributes: false }); - - const runResult = await taskRunner.run(); - - expect(runResult).toMatchObject({ - state: { - runs: 1, - stats: {}, - }, - }); - }); - - // Return stats when run normally - test('returns stats normally', async () => { - generateTelemetry(); - - const runResult = await taskRunner.run(); - - expect(runResult).toMatchObject({ - state: { - runs: 1, - stats: expectedAttributes, - }, - }); - }); - - test('cancels when cancel flag set to "true", returns undefined', async () => { - generateTelemetry(); - - const runResult = await taskRunner.run({ taskCanceled: true }); - - expect(runResult).toBe(undefined); - }); -}); diff --git a/x-pack/test/api_integration/apis/telemetry/telemetry_local.js b/x-pack/test/api_integration/apis/telemetry/telemetry_local.js index 0bf24be0fa0b05c..ef7124e2864f8c0 100644 --- a/x-pack/test/api_integration/apis/telemetry/telemetry_local.js +++ b/x-pack/test/api_integration/apis/telemetry/telemetry_local.js @@ -79,6 +79,10 @@ export default function({ getService }) { expect(stats.stack_stats.kibana.plugins.apm.services_per_agent).to.be.an('object'); expect(stats.stack_stats.kibana.plugins.infraops.last_24_hours).to.be.an('object'); expect(stats.stack_stats.kibana.plugins.kql.defaultQueryLanguage).to.be.a('string'); + expect(stats.stack_stats.kibana.plugins['maps-telemetry'].attributes.timeCaptured).to.be.a( + 'string' + ); + expect(stats.stack_stats.kibana.plugins.reporting.enabled).to.be(true); expect(stats.stack_stats.kibana.plugins.rollups.index_patterns).to.be.an('object'); expect(stats.stack_stats.kibana.plugins.spaces.available).to.be(true);