Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Move src/apm.js to @kbn/apm-config-loader #94315
Changes from 3 commits
0c89030
16bcc2b
331c6f5
a6ae126
850bb49
79cbb13
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 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.
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.
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.
Super-NIT:
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. What an error did you see?
Optional chaining is supported natively in node.js v14.
We also use it in other packages
kibana/packages/kbn-config/src/config_service.ts
Line 152 in 331c6f5
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.
Hum, may be a while back then. Did the change
This file was deleted.
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.
optional: we have a package for everything: