Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move src/apm.js to @kbn/apm-config-loader #94315

Merged
merged 6 commits into from
Mar 14, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions packages/kbn-apm-config-loader/src/config_loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[],
Expand All @@ -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;
};
3 changes: 2 additions & 1 deletion packages/kbn-apm-config-loader/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
12 changes: 12 additions & 0 deletions packages/kbn-apm-config-loader/src/init_apm.test.mocks.ts
Original file line number Diff line number Diff line change
@@ -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,
}));
66 changes: 66 additions & 0 deletions packages/kbn-apm-config-loader/src/init_apm.test.ts
Original file line number Diff line number Diff line change
@@ -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);
});
});
39 changes: 39 additions & 0 deletions packages/kbn-apm-config-loader/src/init_apm.ts
Original file line number Diff line number Diff line change
@@ -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');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understood correctly, we use this same agent for the UI. However, this is the elastic-apm-node package, which is different from the @elastic/apm-rum used on the UI.

Are we sure that we can reuse the same agent initialization for both: server and public?

Copy link
Contributor Author

@pgayvallet pgayvallet Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is only executed on the server-side.

We're not using the same agent on server and browser (apm agent vs rum agent), we're just sharing the same configuration and factorizing the configuration loading logic (the RUM config is then sent to the client in the injected metadatas).

Note that nothing changed in this PR in term of design/architecture, we just moved the script to a package to resolve the cyclic dependency between core and cli.


// Filter out all user PII
apm.addFilter((payload: Record<string, any>) => {
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);
};
52 changes: 0 additions & 52 deletions src/apm.js

This file was deleted.

18 changes: 18 additions & 0 deletions src/cli/apm.js
Original file line number Diff line number Diff line change
@@ -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, '../..');
Copy link
Contributor

@mshustov mshustov Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional: we have a package for everything:

import { REPO_ROOT, kibanaPackageJson } from '@kbn/utils';

const isKibanaDistributable = Boolean(build && build.distributable === true);

module.exports = function (serviceName = name) {
initApm(process.argv, rootDir, isKibanaDistributable, serviceName);
};
2 changes: 1 addition & 1 deletion src/cli/dev.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
2 changes: 1 addition & 1 deletion src/cli/dist.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
6 changes: 3 additions & 3 deletions src/core/server/http_resources/get_apm_config.test.mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, any>;
Expand Down
12 changes: 6 additions & 6 deletions src/core/server/http_resources/get_apm_config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand All @@ -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', () => {
Expand Down
5 changes: 2 additions & 3 deletions src/core/server/http_resources/get_apm_config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down