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.

Sign up for GitHub

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

Jump to bottom

Prepare APM agent configuration for production use #78697

Merged
merged 6 commits into from
Oct 5, 2020
Merged

Prepare APM agent configuration for production use #78697

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
68b1c0d
Support APM agent in distributable builds
joshdover Sep 30, 2020
52d3557
Adjust APM configuration for production
joshdover Sep 30, 2020
08aeaf6
Filter username out of payload
joshdover Oct 2, 2020
6b430bb
Merge remote-tracking branch 'upstream/master' into prepare-apm-for-prod
joshdover Oct 5, 2020
d8dee60
PR comments
joshdover Oct 5, 2020
fa93453
Fix service name in development
joshdover Oct 5, 2020
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
5 changes: 3 additions & 2 deletions packages/kbn-apm-config-loader/src/config.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,13 @@ describe('ApmConfiguration', () => {
resetAllMocks();
});

it('sets the correct service name', () => {
it('sets the correct service name and version', () => {
packageMock.raw = {
version: '9.2.1',
};
const config = new ApmConfiguration(mockedRootDir, {}, false);
expect(config.getConfig('myservice').serviceName).toBe('myservice-9_2_1');
expect(config.getConfig('myservice').serviceName).toBe('myservice');
expect(config.getConfig('myservice').serviceVersion).toBe('9.2.1');
});

it('sets the git revision from `git rev-parse` command in non distribution mode', () => {
Expand Down
11 changes: 9 additions & 2 deletions packages/kbn-apm-config-loader/src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,13 @@ const getDefaultConfig = (isDistributable: boolean): ApmAgentConfig => {
return {
active: false,
globalLabels: {},
centralConfig: false,
joshdover marked this conversation as resolved.
Show resolved Hide resolved
captureHeaders: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a comment explaining why it's disabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am erring on the side of caution here and trying to avoid capturing any data that could be sensitive. We could be more selective about this and capture only headers we know to be safe (an allowlist of headers). WDYT?

captureBody: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

1.in elatic-apm-node/index.d.ts:

type CaptureBody = 'off' | 'errors' | 'transactions' | 'all';
  1. is it possible to improve type safety of

    kibana/packages/kbn-apm-config-loader/src/types.ts

    Line 24 in aa9774f

    export type ApmAgentConfig = Record<string, any>;
    ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pgayvallet I believe we were needing some type changes from APM agent for this correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

AgentConfigOptions is not exported from @elastic/apm-rum, but we could declare a compatible interface, maybe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will handle in follow up. Should not affect 7.10 since we still need to add config-schema validation for 'productionizing' this feature.

logUncaughtExceptions: true,
joshdover marked this conversation as resolved.
Show resolved Hide resolved
};
}

return {
active: false,
serverUrl: 'https://f1542b814f674090afd914960583265f.apm.us-central1.gcp.cloud.es.io:443',
Expand Down Expand Up @@ -60,14 +65,14 @@ export class ApmConfiguration {
) {
// eslint-disable-next-line @typescript-eslint/no-var-requires
const { version, build } = require(join(this.rootDir, 'package.json'));
this.kibanaVersion = version.replace(/\./g, '_');
this.kibanaVersion = version;
this.pkgBuild = build;
}

public getConfig(serviceName: string): ApmAgentConfig {
return {
...this.getBaseConfig(),
serviceName: `${serviceName}-${this.kibanaVersion}`,
serviceName,
};
}

Expand All @@ -88,6 +93,8 @@ export class ApmConfiguration {
if (uuid) {
apmConfig.globalLabels.kibana_uuid = uuid;
}

apmConfig.serviceVersion = this.kibanaVersion;
this.baseConfig = apmConfig;
}

Expand Down
14 changes: 12 additions & 2 deletions src/apm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,18 @@ module.exports = function (serviceName = name) {

apmConfig = loadConfiguration(process.argv, ROOT_DIR, isKibanaDistributable);
const conf = apmConfig.getConfig(serviceName);
require('elastic-apm-node').start(conf);
const apm = require('elastic-apm-node');

// Filter out username PII
apm.addFilter((payload) => {
joshdover marked this conversation as resolved.
Show resolved Hide resolved
if (payload.context && payload.context.user && payload.context.user.username) {
delete payload.context.user.username;
joshdover marked this conversation as resolved.
Show resolved Hide resolved
}

return payload;
});

apm.start(conf);
};

module.exports.getConfig = (serviceName) => {
Expand All @@ -50,4 +61,3 @@ module.exports.getConfig = (serviceName) => {
}
return {};
};
module.exports.isKibanaDistributable = isKibanaDistributable;
5 changes: 3 additions & 2 deletions src/core/public/apm_system.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import type { InternalApplicationStart } from './application';

interface ApmConfig {
// AgentConfigOptions is not exported from @elastic/apm-rum
active?: boolean;
globalLabels?: Record<string, string>;
}

Expand All @@ -39,10 +40,10 @@ export class ApmSystem {
private readonly enabled: boolean;
/**
* `apmConfig` would be populated with relevant APM RUM agent
* configuration if server is started with `ELASTIC_APM_ACTIVE=true`
* configuration if server is started with elastic.apm.* config.
*/
constructor(private readonly apmConfig?: ApmConfig) {
this.enabled = process.env.IS_KIBANA_DISTRIBUTABLE !== 'true' && apmConfig != null;
this.enabled = apmConfig != null && !!apmConfig.active;
Copy link
Contributor

Choose a reason for hiding this comment

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

I had a hard time debugging the config. It's turned out we don't read values from kibana.dev.yml. It might be another source of confusion for developers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's turned out we don't read values from kibana.dev.yml

I think there was some confusion on whether or not we still supported this. It appears we do. Can fix as a follow up so I can get this in for FF

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened issue: #79490
Should be a pretty quick follow up

}

async setup() {
Expand Down
4 changes: 4 additions & 0 deletions src/legacy/server/config/schema.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ const HANDLED_IN_NEW_PLATFORM = Joi.any().description(
);
export default () =>
Joi.object({
elastic: Joi.object({
apm: HANDLED_IN_NEW_PLATFORM,
}).default(),

pkg: Joi.object({
version: Joi.string().default(Joi.ref('$version')),
branch: Joi.string().default(Joi.ref('$branch')),
Expand Down
18 changes: 4 additions & 14 deletions src/legacy/ui/apm/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,30 +17,20 @@
* under the License.
*/

import { getConfig, isKibanaDistributable } from '../../../apm';
import { getConfig } from '../../../apm';
import agent from 'elastic-apm-node';

const apmEnabled = !isKibanaDistributable && process.env.ELASTIC_APM_ACTIVE === 'true';

export function apmImport() {
return apmEnabled ? 'import { init } from "@elastic/apm-rum"' : '';
}

export function apmInit(config) {
return apmEnabled ? `init(${config})` : '';
}
const apmEnabled = getConfig()?.active;

export function getApmConfig(requestPath) {
if (!apmEnabled) {
return null;
}
const config = {
...getConfig('kibana-frontend'),
...{
active: true,
pageLoadTransactionName: requestPath,
},
pageLoadTransactionName: requestPath,
};

/**
* Get current active backend transaction to make distrubuted tracing
* work for rendering the app
Expand Down