-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Add the @kbn/apm-config-loader package #77855
Add the @kbn/apm-config-loader package #77855
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshdover This is a POC of what we talked about, just to be sure we are on the same page. I created a new package, with minimal dependencies to allow the src/apm
script to access what it needed from the config file.
Note that:
-
As we still don't know what the apm agent configuration prefix / options will look like, I just migrated what was currently done in
src/apm.js
. Once we got those infos, adding it should be trivial -
As we are not using the cli's parsed argument, we are not currently applying config values overrides from the command line arguments
Line 68 in 9acf8d2
function applyConfigOverrides(rawConfig, opts, extraCliOptions) { |
The only config values we are currently accessing are server.uuid
and path.data
. Once we need to read the apm agent config from the kibana.yaml file, there will be other though. Not sure if it is critical or not.
Not sure what the best thing to do to address this is:
- Duplicating
applyConfigOverrides
fromserve.js
feels wrong - Extracting it feels a little better, but in both cases
- we have to also extract
src/cli/command.js
and move it somewhere - we will be importing
commander
before apm loads. Maybe fine, but I'm unsure of its import graph.
- we have to also extract
- Manually checking / parsing
process.argv
to try to read only the values we need feels very error prone and reinventing the wheel...
"@elastic/safer-lodash-set": "0.0.0", | ||
"@kbn/utils": "1.0.0", | ||
"js-yaml": "3.13.1", | ||
"lodash": "^4.17.20" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To determine the default path of the config file, and of the data folder, I need to import @kbn/utils
to use getDataPath()
and getConfigPath()
.
Even with deep imports from @kbn-utils/target/path
the module imports @kbn/config-schema
, which imports joi
kibana/packages/kbn-utils/src/path/index.ts
Lines 22 to 23 in 9acf8d2
import { TypeOf, schema } from '@kbn/config-schema'; | |
import { REPO_ROOT } from '../repo_root'; |
Not sure if this is blocker or not. I can split packages/kbn-utils/src/path/index.ts
into multiple files to be able to import the helper methods I need without import the schema that is currently in the same file. That would address this problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's ok if we end up importing Joi in this first pass. We already know it's a source of some slowness and I believe we'll still be able to see that at some level in our traces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Node.js agent currently doesn't instrument joi, but if we were to do that in the future this could present a problem. But I wouldn't worry about this currently - just thought I'd mention it.
export const getConfigFromFiles = (configFiles: readonly string[]) => { | ||
let mergedYaml = {}; | ||
|
||
for (const configFile of configFiles) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to duplicate read_config
and ensure_deep_object
files from packages/kbn-config/src/raw
to avoid importing the module here. It's probably okay-ish though I think.
/** | ||
* Return the configuration files that needs to be loaded. | ||
* | ||
* This mimics the behavior of the `src/cli/serve/serve.js` cli script by reading | ||
* `-c` and `--config` options from process.argv, and fallbacks to `@kbn/utils`'s `getConfigPath()` | ||
*/ | ||
export const getConfigurationFilePaths = (): string[] => { | ||
let configPaths: string[] = []; | ||
|
||
const args = process.argv; | ||
for (let i = 0; i < args.length; i++) { | ||
if ((args[i] === '-c' || args[i] === '--config') && args[i + 1]) { | ||
configPaths.push(resolve(process.cwd(), args[++i])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The command line argument are parsed AFTER the apm initialization
Lines 20 to 22 in 0e55b24
require('../src/apm')(process.env.ELASTIC_APM_PROXY_SERVICE_NAME || 'kibana-proxy'); | |
require('../src/setup_node_env'); | |
require('../src/cli/cli'); |
So I had to duplicate the parsing.
As we currently only need the --config
option, I did it manually to avoid importing commander
, as src/cli/cli.js
and/or src/cli/serve/serve.js
are doing
Extracting the parsing to a separate file isn't trivial either as src/cli/serve/serve.js
currently use some core utils (fromRoot
) in the argument defaults.
The jest integration test failures are caused by the fact that jest is ran with a kibana/src/legacy/ui/ui_render/ui_render_mixin.js Lines 202 to 204 in bcaffba
Causing
A simple |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like the right direction IMO. I'm ok accepting a bit of duplication to achieve what we need here for 7.10
"@elastic/safer-lodash-set": "0.0.0", | ||
"@kbn/utils": "1.0.0", | ||
"js-yaml": "3.13.1", | ||
"lodash": "^4.17.20" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's ok if we end up importing Joi in this first pass. We already know it's a source of some slowness and I believe we'll still be able to see that at some level in our traces.
private getDevConfig() { | ||
try { | ||
const apmDevConfigPath = join(this.rootDir, 'config', 'apm.dev.js'); | ||
return require(apmDevConfigPath); | ||
} catch (e) { | ||
return {}; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing this function would be replaced when we add config keys for APM to the regular yaml file to read from this.rawKibanaConfig
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how exactly the apm team was using this, but if it's really just for local development, I guess that once the properties are exposed / consumed from the kibana.yaml
config file, the dev config can just go away and be replaced by local kibana.yaml
config overrides? Need confirmation from apm though.
private kibanaVersion: string; | ||
|
||
constructor( | ||
private readonly rootDir: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can get this from ROOT_DIR
in @kbn/utils
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tylersmalley Just thought about something after our zoom meeting: If you were planning to move the src/apm
script to @kbn/utils
and import @kbn/apm-config-loader
from here, We are going to cause a cyclic dependency, as @kbn/apm-config-loader
actually also got a dependency to @kbn/utils
.
Maybe the src/apm
script itself should be moved to @kbn/apm-config-loader
instead to avoid this? In that case, I could rename it to something like @kbn/apm-agent
for example. Would not change much for future consumers of the agent or agent config. WDYT?
retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @joshdover @tylersmalley PTAL.
if (isDistributable) { | ||
return { | ||
active: false, | ||
globalLabels: {}, | ||
}; | ||
} | ||
return { | ||
active: false, | ||
serverUrl: 'https://f1542b814f674090afd914960583265f.apm.us-central1.gcp.cloud.es.io:443', | ||
// The secretToken below is intended to be hardcoded in this file even though | ||
// it makes it public. This is not a security/privacy issue. Normally we'd | ||
// instead disable the need for a secretToken in the APM Server config where | ||
// the data is transmitted to, but due to how it's being hosted, it's easier, | ||
// for now, to simply leave it in. | ||
secretToken: 'R0Gjg46pE9K9wGestd', | ||
globalLabels: {}, | ||
breakdownMetrics: true, | ||
centralConfig: false, | ||
logUncaughtExceptions: true, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I have a couple questions here.
- The prod/dev switch is currently based on the
build.distributable
property of thepackage.json
file. Is this still how we want to distinguish between prod and dev? For example in core, the 'dev mode' is based on
kibana/packages/kbn-config/src/env.ts
Line 136 in ab92bbb
const isDevMode = this.cliArgs.dev || this.cliArgs.envName === 'development'; |
I know 'dev mode' and 'distribution' are now the same thing, but still asking.
- currently even in non-distribution mode, the agent is disabled by default. Local development was using the 'development config' to enable it locally. Should the agent be enabled by default in non-distribution mode? That would enable it on local development instance though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these defaults are only here for the use by the APM team, I wonder if we could move them out of the repository and into some dev documentation instead. We don't have any other dev-specific configuration defaults like this. @vigneshshanmugam any objections?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the agent be enabled by default in non-distribution mode? That would enable it on local development instance though...
Ah yes if we do want to collect this stuff by default out of local dev then we will need this around. We probably shouldn't enable it by default yet in dev until we have the proper PII controls in place.
The prod/dev switch is currently based on the build.distributable property of the package.json file. Is this still how we want to distinguish between prod and dev?
I lean towards using the --dev
flag if practical, but I'm not sure it matters much. My main reason would be that we probably only want to enable the default dev APM collection in the default mode that developers use on a day-to-day which may have different performance characteristics than when --dev
is not specified.
// There is an (incomplete) `AgentConfigOptions` type declared in node_modules/elastic-apm-node/index.d.ts | ||
// but it's not exported, and using ts tricks to retrieve the type via Parameters<ApmAgent['start']>[0] | ||
// causes errors in the generated .d.ts file because of esModuleInterop and the fact that the apm module | ||
// is just exporting an instance of the `ApmAgent` type. | ||
export type ApmAgentConfig = Record<string, any>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the elastic-apm-node
package doesn't export its types, so I used a plain record here. I could duplicate the types, but some properties are not just primitives, so that's a lot of (probably unnecessary) duplication. For what we are doing with the config, I think this is alright though, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does export types - https://github.com/elastic/apm-agent-nodejs/blob/master/index.d.ts. Are we missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it declare
types, but does not export
them, so you can't import then via import { AgentConfigOptions } from 'elastic-apm-node'
. Only the agent
(default export) is actually declared as exported in the definition file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh i see. Will do a fix in the next release if it makes it easier.
export const applyConfigOverrides = (config: Record<string, any>, argv: string[]) => { | ||
const serverUuid = getArgValue(argv, '--server.uuid'); | ||
if (serverUuid) { | ||
set(config, 'server.uuid', serverUuid); | ||
} | ||
const dataPath = getArgValue(argv, '--path.data'); | ||
if (dataPath) { | ||
set(config, 'path.data', dataPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept it very simple as we only these two overrides from core configuration. Note that the elastic.apm.XXX
properties are therefor not overridable via cli arguments, but I assumed this wasn't really necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with that limitation
private getDevConfig(): ApmAgentConfig { | ||
try { | ||
const apmDevConfigPath = join(this.rootDir, 'config', 'apm.dev.js'); | ||
return require(apmDevConfigPath); | ||
} catch (e) { | ||
return {}; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't sure if this is still required. I kept it for now. Tell me if I should just remove it instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no problem with removing it. @vigneshshanmugam will this cause many issues? Instead of using 'apm.dev.js', developers will configure APM in dev with the regular kibana.yml file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, both should do the job. Probably some devs might already be using it, not sure if its worth having it for backward compatibility purposes. However we have to update the docs if we are removing it - https://github.com/elastic/kibana/blob/a20469f038e04e3c5fb821ed0b38360fb9698fe2/docs/developer/getting-started/debugging.asciidoc#instrumenting-with-elastic-apm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pointer to the doc. I will keep it for now, we'll see later if we want to make the kibana.yml the official way to enabled apm in dev.
private getConfigFromKibanaConfig(): ApmAgentConfig { | ||
return get(this.rawKibanaConfig, 'elastic.apm', {}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I'm currently not doing ANY validation for the elastic.apm
configuration from the kibana config file, and just using it as it is. Main reason for that was that the AgentConfigOptions
type is quite big, and incomplete (for example the breakdownMetrics
option we are using, and that is present/used in the agent code, is not listed on the type...). If we think this is necessary, I can still try to create a schema, but as the type is incomplete I would probably need to add unknows: 'allow'
which reduce a lot the 'security' of the validation...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
breakdownMetrics
is a flag on the RUM agent and its typed here - https://github.com/elastic/apm-agent-rum-js/blob/e0e04642b688274c7a1b9e1c2f8db048d92343c7/packages/rum/src/index.d.ts#L119
AFAIR, we were using the same config for both RUM and Node.js agent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh ok, thanks for the explanation. So I guess that if we were to add proper schema validation, it would require to defines all properties from both configurations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that would work or we can also define a intersection type from the exported configuarations.
Pinging @elastic/kibana-platform (Team:Platform) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a look at APM side of the configs, Code looks good to me 👍
private getConfigFromKibanaConfig(): ApmAgentConfig { | ||
return get(this.rawKibanaConfig, 'elastic.apm', {}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
breakdownMetrics
is a flag on the RUM agent and its typed here - https://github.com/elastic/apm-agent-rum-js/blob/e0e04642b688274c7a1b9e1c2f8db048d92343c7/packages/rum/src/index.d.ts#L119
AFAIR, we were using the same config for both RUM and Node.js agent.
// There is an (incomplete) `AgentConfigOptions` type declared in node_modules/elastic-apm-node/index.d.ts | ||
// but it's not exported, and using ts tricks to retrieve the type via Parameters<ApmAgent['start']>[0] | ||
// causes errors in the generated .d.ts file because of esModuleInterop and the fact that the apm module | ||
// is just exporting an instance of the `ApmAgent` type. | ||
export type ApmAgentConfig = Record<string, any>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does export types - https://github.com/elastic/apm-agent-nodejs/blob/master/index.d.ts. Are we missing something?
ack: will review first thing tomorrow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, mostly just some questions about APM configuration + some nits. If we'd like to, we can make the configuration changes in a separate PR to keep this one focused on config loading.
Should we go ahead and remove the distributable check on the frontend here? Could also be done as a follow up.
kibana/src/core/public/kbn_bootstrap.ts
Line 42 in 8ba60a4
const APM_ENABLED = process.env.IS_KIBANA_DISTRIBUTABLE !== 'true' && apmConfig != null; |
EDIT: I think we should merge this without any changes to the APM config right now. It'll be easier to iterate on that outside this PR and getting this in sooner than later allows to move forward quicker.
public getConfig(serviceName: string): ApmAgentConfig { | ||
return { | ||
...this.getBaseConfig(), | ||
serviceName: `${serviceName}-${this.kibanaVersion}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is how it works in the current implementation, but I think we should be using the serviceVersion
property rather than appending the Kibana version to the serviceName
. I also wonder if it would be useful to include the git revision, something like:
serviceName: `${serviceName}-${this.kibanaVersion}`, | |
serviceName, | |
serviceVersion: `${this.kibanaVersion}-${this.git_rev}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems better yea, but I'm not sure who's in charge of confirming that the change would be alright? Is that on us?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of you edit,
I think we should merge this without any changes to the APM config right now
I will keep this for a follow up to avoid changing anything from the apm config in the current PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the reason why I originally went with this approach was because the APM UI didn't have a way to filter on serviceVersion
- so it would bundle up all traces for Kibana v8.0.0, v7.9.2 etc etc. into the same view. I'm not sure if it's possible to apply such a filter easily today that will stick between clicks to different pages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems the service version filter does stick around in the APM UI when switching between different views now. It'd be nice to use the same serviceName for all versions so that we can do comparisons more easily
stdio: ['ignore', 'pipe', 'ignore'], | ||
}).trim(); | ||
} catch (e) { | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for distributables we should fallback to the build sha located on pkg.build.sha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in 8fc031d
packages/kbn-apm-config-loader/src/utils/get_config_file_paths.test.ts
Outdated
Show resolved
Hide resolved
@vigneshshanmugam @watson The PR should not be impacting the current APM config in any way (see #77855 (review)). Additional work to change the dev/production config will be done in a follow-up. Are you alright with us merging this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tried this out locally, but it seems fine 👍
I just left some comments to clarify a few things.
to load the required configuration options from the `kibana.yaml` configuration file with | ||
default values. | ||
|
||
### Why can't just use @kbn-config? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
### Why can't just use @kbn-config? | |
### Why not just use @kbn-config? |
"@elastic/safer-lodash-set": "0.0.0", | ||
"@kbn/utils": "1.0.0", | ||
"js-yaml": "3.13.1", | ||
"lodash": "^4.17.20" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Node.js agent currently doesn't instrument joi, but if we were to do that in the future this could present a problem. But I wouldn't worry about this currently - just thought I'd mention it.
public getConfig(serviceName: string): ApmAgentConfig { | ||
return { | ||
...this.getBaseConfig(), | ||
serviceName: `${serviceName}-${this.kibanaVersion}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the reason why I originally went with this approach was because the APM UI didn't have a way to filter on serviceVersion
- so it would bundle up all traces for Kibana v8.0.0, v7.9.2 etc etc. into the same view. I'm not sure if it's possible to apply such a filter easily today that will stick between clicks to different pages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍
@pgayvallet Sure, FYI I added the |
💚 Build SucceededMetrics [docs]async chunks size
page load bundle size
distributable file count
History
To update your PR or re-run it, just comment with: |
* first shot of the apm configuration loader * revert changes to kibana config * remove test files for now * remove `?.` usages * use lazy config init to avoid crashing integration test runner * loader improvements * add config value override via cli args * add tests for utils package * add prod/dev config handling + loader tests * add tests for config * address josh's comments * nit on doc
* master: (226 commits) [Enterprise Search] Added Logic for the Credentials View (elastic#77626) [CSM] Js errors (elastic#77919) Add the @kbn/apm-config-loader package (elastic#77855) [Security Solution] Refactor useSelector (elastic#75297) Implement tagcloud renderer (elastic#77910) [APM] Alerting: Add global option to create all alert types (elastic#78151) [Ingest pipelines] Upload indexed document to test a pipeline (elastic#77939) TypeScript cleanup in visualizations plugin (elastic#78428) Lazy load metric & mardown visualizations (elastic#78391) [Detections][EQL] EQL rule execution in detection engine (elastic#77419) Update tutorial-full-experience.asciidoc (elastic#75836) Update tutorial-define-index.asciidoc (elastic#75754) Add support for runtime field types to mappings editor. (elastic#77420) [Monitoring] Usage collection (elastic#75878) [Docs][Actions] Add docs for Jira and IBM Resilient (elastic#78316) [Security Solution][Resolver] Update @timestamp formatting (elastic#78166) [Security Solution] Fix app layout (elastic#76668) [Security Solution][Resolver] 2 new functions to DAL (elastic#78477) Adds new elasticsearch client to telemetry plugin (elastic#78046) skip flaky suite (elastic#78512) (elastic#78511) (elastic#78510) (elastic#78509) (elastic#78508) (elastic#78507) (elastic#78506) (elastic#78505) (elastic#78504) (elastic#78503) (elastic#78502) (elastic#78501) (elastic#78500) ...
* first shot of the apm configuration loader * revert changes to kibana config * remove test files for now * remove `?.` usages * use lazy config init to avoid crashing integration test runner * loader improvements * add config value override via cli args * add tests for utils package * add prod/dev config handling + loader tests * add tests for config * address josh's comments * nit on doc
@pgayvallet Should the documentation have been updated together with this change? |
Should have. Created #83045. |
Summary
Part of #76003
Add a new
@kbn/apm-config-loader
package, and adaptsrc/apm
to use it to load its configurationChecklist