From 62dda4fb27f89081a7408d0c5d2bd7e022d34cc1 Mon Sep 17 00:00:00 2001 From: restrry Date: Fri, 5 Nov 2021 13:22:28 +0100 Subject: [PATCH] TO DISCUSS. what if we enforce contextPropagationOnly to be configured when active is defined --- .../getting-started/debugging.asciidoc | 2 +- .../kbn-apm-config-loader/src/config.test.ts | 60 +++++-------------- packages/kbn-apm-config-loader/src/config.ts | 10 ++-- 3 files changed, 19 insertions(+), 53 deletions(-) diff --git a/docs/developer/getting-started/debugging.asciidoc b/docs/developer/getting-started/debugging.asciidoc index 1254462d2e4eac..9842f3b5358f4e 100644 --- a/docs/developer/getting-started/debugging.asciidoc +++ b/docs/developer/getting-started/debugging.asciidoc @@ -53,7 +53,7 @@ APM https://www.elastic.co/guide/en/apm/agent/rum-js/current/index.html[Real User Monitoring agent] is not available in the {kib} distributables, however the agent can be enabled by setting `ELASTIC_APM_ACTIVE` to -`true`. flags +`true` and `ELASTIC_APM_CONTEXT_PROPAGATION_ONLY` to `false`. flags .... ELASTIC_APM_ACTIVE=true yarn start diff --git a/packages/kbn-apm-config-loader/src/config.test.ts b/packages/kbn-apm-config-loader/src/config.test.ts index b0beebfefd6bde..09c808689b3874 100644 --- a/packages/kbn-apm-config-loader/src/config.test.ts +++ b/packages/kbn-apm-config-loader/src/config.test.ts @@ -133,6 +133,7 @@ describe('ApmConfiguration', () => { elastic: { apm: { active: true, + contextPropagationOnly: false, serverUrl: 'https://url', secretToken: 'secret', }, @@ -151,6 +152,7 @@ describe('ApmConfiguration', () => { it('loads the configuration from the dev config is present', () => { devConfigMock.raw = { active: true, + contextPropagationOnly: false, serverUrl: 'https://dev-url.co', }; const config = new ApmConfiguration(mockedRootDir, {}, false); @@ -179,6 +181,7 @@ describe('ApmConfiguration', () => { elastic: { apm: { active: true, + contextPropagationOnly: false, serverUrl: 'https://url', secretToken: 'secret', }, @@ -272,35 +275,7 @@ describe('ApmConfiguration', () => { ); }); - it('is "false" if "active: true" configured and "contextPropagationOnly" is not specified', () => { - const kibanaConfig = { - elastic: { - apm: { - active: true, - }, - }, - }; - - expect( - new ApmConfiguration(mockedRootDir, kibanaConfig, false).getConfig('serviceName') - ).toEqual( - expect.objectContaining({ - active: true, - contextPropagationOnly: false, - }) - ); - - expect( - new ApmConfiguration(mockedRootDir, kibanaConfig, true).getConfig('serviceName') - ).toEqual( - expect.objectContaining({ - active: true, - contextPropagationOnly: false, - }) - ); - }); - - it('throws if "active: false" set without configuring "contextPropagationOnly: false"', () => { + it('throws if "active: false" set without configuring "contextPropagationOnly"', () => { const kibanaConfig = { elastic: { apm: { @@ -312,42 +287,35 @@ describe('ApmConfiguration', () => { expect(() => new ApmConfiguration(mockedRootDir, kibanaConfig, false).getConfig('serviceName') ).toThrowErrorMatchingInlineSnapshot( - `"APM is disabled, but context propagation is enabled. Please disable context propagation with contextPropagationOnly:false"` + `"[apm.active] is configured but contextPropagationOnly is not. Please, set [apm.contextPropagationOnly] value explicitly."` ); expect(() => new ApmConfiguration(mockedRootDir, kibanaConfig, true).getConfig('serviceName') ).toThrowErrorMatchingInlineSnapshot( - `"APM is disabled, but context propagation is enabled. Please disable context propagation with contextPropagationOnly:false"` + `"[apm.active] is configured but contextPropagationOnly is not. Please, set [apm.contextPropagationOnly] value explicitly."` ); }); - it('does not throw if "active: false" and "contextPropagationOnly: false" configured', () => { + it('throws if "active: true" set without configuring "contextPropagationOnly"', () => { const kibanaConfig = { elastic: { apm: { - active: false, - contextPropagationOnly: false, + active: true, }, }, }; - expect( + expect(() => new ApmConfiguration(mockedRootDir, kibanaConfig, false).getConfig('serviceName') - ).toEqual( - expect.objectContaining({ - active: false, - contextPropagationOnly: false, - }) + ).toThrowErrorMatchingInlineSnapshot( + `"[apm.active] is configured but contextPropagationOnly is not. Please, set [apm.contextPropagationOnly] value explicitly."` ); - expect( + expect(() => new ApmConfiguration(mockedRootDir, kibanaConfig, true).getConfig('serviceName') - ).toEqual( - expect.objectContaining({ - active: false, - contextPropagationOnly: false, - }) + ).toThrowErrorMatchingInlineSnapshot( + `"[apm.active] is configured but contextPropagationOnly is not. Please, set [apm.contextPropagationOnly] value explicitly."` ); }); }); diff --git a/packages/kbn-apm-config-loader/src/config.ts b/packages/kbn-apm-config-loader/src/config.ts index ecafcbd7e32612..e74d60096717e0 100644 --- a/packages/kbn-apm-config-loader/src/config.ts +++ b/packages/kbn-apm-config-loader/src/config.ts @@ -113,6 +113,8 @@ export class ApmConfiguration { if (process.env.ELASTIC_APM_ACTIVE === 'true') { config.active = true; + } else if (process.env.ELASTIC_APM_ACTIVE === 'false') { + config.active = false; } if (process.env.ELASTIC_APM_CONTEXT_PROPAGATION_ONLY === 'true') { @@ -268,16 +270,12 @@ export class ApmConfiguration { this.getConfigFromEnv() ); - if (config.active === false && config.contextPropagationOnly !== false) { + if (config.active !== undefined && config.contextPropagationOnly === undefined) { throw new Error( - 'APM is disabled, but context propagation is enabled. Please disable context propagation with contextPropagationOnly:false' + '[apm.active] is configured but contextPropagationOnly is not. Please, set [apm.contextPropagationOnly] value explicitly.' ); } - if (config.active === true) { - config.contextPropagationOnly = config.contextPropagationOnly ?? false; - } - return config; } }