Move src/apm.js to @kbn/apm-config-loader #94315
Conversation
pgayvallet
added
v7.13.0
Team:Core
|
Pinging @elastic/kibana-core (Team:Core) |
| // Filter out all user PII | ||
| apm.addFilter((payload: Record<string, any>) => { | ||
| try { | ||
| if (payload.context && payload.context.user && typeof payload.context.user === 'object') { |
There was a problem hiding this comment.
Super-NIT:
| if (payload.context && payload.context.user && typeof payload.context.user === 'object') { | |
| if (payload.context?.user && typeof payload.context.user === 'object') { |
There was a problem hiding this comment.
Can't use ? operator in packages code. Not sure why (well, we're not using babel for packages, only tsc, so it's probably that), but we can't.
There was a problem hiding this comment.
Hm. What an error did you see?
Optional chaining is supported natively in node.js v14.
We also use it in other packages
There was a problem hiding this comment.
Hum, may be a while back then. Did the change
|
|
||
| // 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'); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| const { name, build } = require('../../package.json'); | ||
| const { initApm } = require('@kbn/apm-config-loader'); | ||
|
|
||
| const rootDir = join(__dirname, '../..'); |
There was a problem hiding this comment.
optional: we have a package for everything:
import { REPO_ROOT, kibanaPackageJson } from '@kbn/utils';
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
* Move src/apm.js to @kbn/apm-config-loader * add unit tests for `initApm` * return undefined instead of empty config * use ?.
Summary
Fix #93823
Part of #94141
PR removes the dependency from core to
/src/apm.jsby moving the apm script to the@kbn/apm-config-loaderpackage.Checklist