From ed5bbdaa95c6249e5e42d0992e4c1b5987698c98 Mon Sep 17 00:00:00 2001 From: Pierre Gayvallet Date: Sun, 14 Mar 2021 22:53:52 +0100 Subject: [PATCH] Move src/apm.js to @kbn/apm-config-loader (#94315) * Move src/apm.js to @kbn/apm-config-loader * add unit tests for `initApm` * return undefined instead of empty config * use ?. --- .../src/config_loader.ts | 21 +++++- packages/kbn-apm-config-loader/src/index.ts | 3 +- .../src/init_apm.test.mocks.ts | 12 ++++ .../src/init_apm.test.ts | 66 +++++++++++++++++++ .../kbn-apm-config-loader/src/init_apm.ts | 39 +++++++++++ src/apm.js | 52 --------------- src/cli/apm.js | 18 +++++ src/cli/dev.js | 2 +- src/cli/dist.js | 2 +- .../get_apm_config.test.mocks.ts | 6 +- .../http_resources/get_apm_config.test.ts | 12 ++-- .../server/http_resources/get_apm_config.ts | 5 +- 12 files changed, 169 insertions(+), 69 deletions(-) create mode 100644 packages/kbn-apm-config-loader/src/init_apm.test.mocks.ts create mode 100644 packages/kbn-apm-config-loader/src/init_apm.test.ts create mode 100644 packages/kbn-apm-config-loader/src/init_apm.ts delete mode 100644 src/apm.js create mode 100644 src/cli/apm.js diff --git a/packages/kbn-apm-config-loader/src/config_loader.ts b/packages/kbn-apm-config-loader/src/config_loader.ts index d9b8fcaa8f1c..75f69481da76 100644 --- a/packages/kbn-apm-config-loader/src/config_loader.ts +++ b/packages/kbn-apm-config-loader/src/config_loader.ts @@ -8,13 +8,16 @@ import { getConfigurationFilePaths, getConfigFromFiles, applyConfigOverrides } from './utils'; import { ApmConfiguration } from './config'; +import { ApmAgentConfig } from './types'; + +let apmConfig: ApmConfiguration | undefined; /** * Load the APM configuration. * * @param argv the `process.argv` arguments * @param rootDir The root directory of kibana (where the sources and the `package.json` file are) - * @param production true for production builds, false otherwise + * @param isDistributable true for production builds, false otherwise */ export const loadConfiguration = ( argv: string[], @@ -24,5 +27,19 @@ export const loadConfiguration = ( const configPaths = getConfigurationFilePaths(argv); const rawConfiguration = getConfigFromFiles(configPaths); applyConfigOverrides(rawConfiguration, argv); - return new ApmConfiguration(rootDir, rawConfiguration, isDistributable); + + apmConfig = new ApmConfiguration(rootDir, rawConfiguration, isDistributable); + return apmConfig; +}; + +export const getConfiguration = (serviceName: string): ApmAgentConfig | undefined => { + // integration test runner starts a kibana server that import the module without initializing APM. + // so we need to check initialization of the config. + // note that we can't just load the configuration during this module's import + // because jest IT are ran with `--config path-to-jest-config.js` which conflicts with the CLI's `config` arg + // causing the config loader to try to load the jest js config as yaml and throws. + if (apmConfig) { + return apmConfig.getConfig(serviceName); + } + return undefined; }; diff --git a/packages/kbn-apm-config-loader/src/index.ts b/packages/kbn-apm-config-loader/src/index.ts index 737bd1bc8e49..da42bfad1841 100644 --- a/packages/kbn-apm-config-loader/src/index.ts +++ b/packages/kbn-apm-config-loader/src/index.ts @@ -6,6 +6,7 @@ * Side Public License, v 1. */ -export { loadConfiguration } from './config_loader'; +export { getConfiguration } from './config_loader'; +export { initApm } from './init_apm'; export type { ApmConfiguration } from './config'; export type { ApmAgentConfig } from './types'; diff --git a/packages/kbn-apm-config-loader/src/init_apm.test.mocks.ts b/packages/kbn-apm-config-loader/src/init_apm.test.mocks.ts new file mode 100644 index 000000000000..c3a1f6b77158 --- /dev/null +++ b/packages/kbn-apm-config-loader/src/init_apm.test.mocks.ts @@ -0,0 +1,12 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +export const mockLoadConfiguration = jest.fn(); +jest.doMock('./config_loader', () => ({ + loadConfiguration: mockLoadConfiguration, +})); diff --git a/packages/kbn-apm-config-loader/src/init_apm.test.ts b/packages/kbn-apm-config-loader/src/init_apm.test.ts new file mode 100644 index 000000000000..95f0a15a448c --- /dev/null +++ b/packages/kbn-apm-config-loader/src/init_apm.test.ts @@ -0,0 +1,66 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +import { mockLoadConfiguration } from './init_apm.test.mocks'; + +import { initApm } from './init_apm'; +import apm from 'elastic-apm-node'; + +describe('initApm', () => { + let apmAddFilterSpy: jest.SpyInstance; + let apmStartSpy: jest.SpyInstance; + let getConfig: jest.Mock; + + beforeEach(() => { + apmAddFilterSpy = jest.spyOn(apm, 'addFilter').mockImplementation(() => undefined); + apmStartSpy = jest.spyOn(apm, 'start').mockImplementation(() => undefined as any); + getConfig = jest.fn(); + + mockLoadConfiguration.mockImplementation(() => ({ + getConfig, + })); + }); + + afterEach(() => { + jest.restoreAllMocks(); + mockLoadConfiguration.mockReset(); + }); + + it('calls `loadConfiguration` with the correct options', () => { + initApm(['foo', 'bar'], 'rootDir', true, 'service-name'); + + expect(mockLoadConfiguration).toHaveBeenCalledTimes(1); + expect(mockLoadConfiguration).toHaveBeenCalledWith(['foo', 'bar'], 'rootDir', true); + }); + + it('calls `apmConfigLoader.getConfig` with the correct options', () => { + initApm(['foo', 'bar'], 'rootDir', true, 'service-name'); + + expect(getConfig).toHaveBeenCalledTimes(1); + expect(getConfig).toHaveBeenCalledWith('service-name'); + }); + + it('registers a filter using `addFilter`', () => { + initApm(['foo', 'bar'], 'rootDir', true, 'service-name'); + + expect(apmAddFilterSpy).toHaveBeenCalledTimes(1); + expect(apmAddFilterSpy).toHaveBeenCalledWith(expect.any(Function)); + }); + + it('starts apm with the config returned from `getConfig`', () => { + const config = { + foo: 'bar', + }; + getConfig.mockReturnValue(config); + + initApm(['foo', 'bar'], 'rootDir', true, 'service-name'); + + expect(apmStartSpy).toHaveBeenCalledTimes(1); + expect(apmStartSpy).toHaveBeenCalledWith(config); + }); +}); diff --git a/packages/kbn-apm-config-loader/src/init_apm.ts b/packages/kbn-apm-config-loader/src/init_apm.ts new file mode 100644 index 000000000000..21c40c8b3941 --- /dev/null +++ b/packages/kbn-apm-config-loader/src/init_apm.ts @@ -0,0 +1,39 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +import { loadConfiguration } from './config_loader'; + +export const initApm = ( + argv: string[], + rootDir: string, + isDistributable: boolean, + serviceName: string +) => { + const apmConfigLoader = loadConfiguration(argv, rootDir, isDistributable); + const apmConfig = apmConfigLoader.getConfig(serviceName); + + // we want to only load the module when effectively used + // eslint-disable-next-line @typescript-eslint/no-var-requires + const apm = require('elastic-apm-node'); + + // Filter out all user PII + apm.addFilter((payload: Record) => { + try { + if (payload.context?.user && typeof payload.context.user === 'object') { + Object.keys(payload.context.user).forEach((key) => { + payload.context.user[key] = '[REDACTED]'; + }); + } + } catch (e) { + // just silently ignore the error + } + return payload; + }); + + apm.start(apmConfig); +}; diff --git a/src/apm.js b/src/apm.js deleted file mode 100644 index b5137b8f3624..000000000000 --- a/src/apm.js +++ /dev/null @@ -1,52 +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 - * 2.0 and the Server Side Public License, v 1; you may not use this file except - * in compliance with, at your election, the Elastic License 2.0 or the Server - * Side Public License, v 1. - */ - -const { join } = require('path'); -const { name, build } = require('../package.json'); -const { loadConfiguration } = require('@kbn/apm-config-loader'); - -const ROOT_DIR = join(__dirname, '..'); -let apmConfig; - -/** - * Flag to disable APM RUM support on all kibana builds by default - */ -const isKibanaDistributable = Boolean(build && build.distributable === true); - -module.exports = function (serviceName = name) { - apmConfig = loadConfiguration(process.argv, ROOT_DIR, isKibanaDistributable); - const conf = apmConfig.getConfig(serviceName); - const apm = require('elastic-apm-node'); - - // Filter out all user PII - apm.addFilter((payload) => { - try { - if (payload.context && payload.context.user && typeof payload.context.user === 'object') { - Object.keys(payload.context.user).forEach((key) => { - payload.context.user[key] = '[REDACTED]'; - }); - } - } finally { - return payload; - } - }); - - apm.start(conf); -}; - -module.exports.getConfig = (serviceName) => { - // integration test runner starts a kibana server that import the module without initializing APM. - // so we need to check initialization of the config. - // note that we can't just load the configuration during this module's import - // because jest IT are ran with `--config path-to-jest-config.js` which conflicts with the CLI's `config` arg - // causing the config loader to try to load the jest js config as yaml and throws. - if (apmConfig) { - return apmConfig.getConfig(serviceName); - } - return {}; -}; diff --git a/src/cli/apm.js b/src/cli/apm.js new file mode 100644 index 000000000000..0a9f38b7efca --- /dev/null +++ b/src/cli/apm.js @@ -0,0 +1,18 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +const { join } = require('path'); +const { name, build } = require('../../package.json'); +const { initApm } = require('@kbn/apm-config-loader'); + +const rootDir = join(__dirname, '../..'); +const isKibanaDistributable = Boolean(build && build.distributable === true); + +module.exports = function (serviceName = name) { + initApm(process.argv, rootDir, isKibanaDistributable, serviceName); +}; diff --git a/src/cli/dev.js b/src/cli/dev.js index 614529cc2658..42263986f98f 100644 --- a/src/cli/dev.js +++ b/src/cli/dev.js @@ -6,7 +6,7 @@ * Side Public License, v 1. */ -require('../apm')(process.env.ELASTIC_APM_SERVICE_NAME || 'kibana-proxy'); +require('./apm')(process.env.ELASTIC_APM_SERVICE_NAME || 'kibana-proxy'); require('../setup_node_env'); require('../setup_node_env/root'); require('./cli'); diff --git a/src/cli/dist.js b/src/cli/dist.js index b22bffa8cbd9..fccc93cd5363 100644 --- a/src/cli/dist.js +++ b/src/cli/dist.js @@ -6,7 +6,7 @@ * Side Public License, v 1. */ -require('../apm')(); +require('./apm')(); require('../setup_node_env/dist'); require('../setup_node_env/root'); require('./cli'); diff --git a/src/core/server/http_resources/get_apm_config.test.mocks.ts b/src/core/server/http_resources/get_apm_config.test.mocks.ts index 635e93913c22..8c3fa180a04f 100644 --- a/src/core/server/http_resources/get_apm_config.test.mocks.ts +++ b/src/core/server/http_resources/get_apm_config.test.mocks.ts @@ -6,9 +6,9 @@ * Side Public License, v 1. */ -export const getConfigMock = jest.fn(); -jest.doMock('../../../apm', () => ({ - getConfig: getConfigMock, +export const getConfigurationMock = jest.fn(); +jest.doMock('@kbn/apm-config-loader', () => ({ + getConfiguration: getConfigurationMock, })); export const agentMock = {} as Record; diff --git a/src/core/server/http_resources/get_apm_config.test.ts b/src/core/server/http_resources/get_apm_config.test.ts index 6ef68494ced0..bd867375f46d 100644 --- a/src/core/server/http_resources/get_apm_config.test.ts +++ b/src/core/server/http_resources/get_apm_config.test.ts @@ -6,7 +6,7 @@ * Side Public License, v 1. */ -import { getConfigMock, agentMock } from './get_apm_config.test.mocks'; +import { getConfigurationMock, agentMock } from './get_apm_config.test.mocks'; import { getApmConfig } from './get_apm_config'; const defaultApmConfig = { @@ -16,28 +16,28 @@ const defaultApmConfig = { describe('getApmConfig', () => { beforeEach(() => { - getConfigMock.mockReturnValue(defaultApmConfig); + getConfigurationMock.mockReturnValue(defaultApmConfig); }); afterEach(() => { - getConfigMock.mockReset(); + getConfigurationMock.mockReset(); agentMock.currentTransaction = null; }); it('returns null if apm is disabled', () => { - getConfigMock.mockReturnValue({ + getConfigurationMock.mockReturnValue({ active: false, }); expect(getApmConfig('/path')).toBeNull(); - getConfigMock.mockReturnValue(undefined); + getConfigurationMock.mockReturnValue(undefined); expect(getApmConfig('/path')).toBeNull(); }); it('calls `getConfig` with the correct parameters', () => { getApmConfig('/path'); - expect(getConfigMock).toHaveBeenCalledWith('kibana-frontend'); + expect(getConfigurationMock).toHaveBeenCalledWith('kibana-frontend'); }); it('returns the configuration from the `getConfig` call', () => { diff --git a/src/core/server/http_resources/get_apm_config.ts b/src/core/server/http_resources/get_apm_config.ts index 0b0eb7426f86..6ea172b162d2 100644 --- a/src/core/server/http_resources/get_apm_config.ts +++ b/src/core/server/http_resources/get_apm_config.ts @@ -7,11 +7,10 @@ */ import agent from 'elastic-apm-node'; -// @ts-expect-error apm module is a js file outside of core (need to split APM/rum configuration) -import { getConfig } from '../../../apm'; +import { getConfiguration } from '@kbn/apm-config-loader'; export const getApmConfig = (requestPath: string) => { - const baseConfig = getConfig('kibana-frontend'); + const baseConfig = getConfiguration('kibana-frontend'); if (!baseConfig?.active) { return null; }