-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Instrument Kibana with APM RUM agent #44281
Conversation
retest |
💚 Build Succeeded |
💚 Build Succeeded |
💔 Build Failed |
Looks like the xpack functional tests are failing because |
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.
Please request review from @elastic/kibana-operations
once the tests are passing
@spalger Sure, would have to wait for Node agent PR to get merged before we merge this one. |
💚 Build Succeeded |
💔 Build Failed |
💔 Build Failed
|
💔 Build Failed
|
@elastic/kibana-operations Can i get review on this PR? Thanks in advance. |
@vigneshshanmugam What's the idea behind the |
@watson It's an extra guard and considered it only for RUM at the moment, since we don't want to ship RUM agent JavaScript bundle to the client even if the flag is set to true by accident. Wont be applicable for the Node.js agent since its we require it anyway in the server. |
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.
Now that you no longer use the configFile
config option, but manually load the config file. You could make the code a little bit simpler by refactoring the way src/apm.js
loads config/apm.js
which in turn loads config/apm.dev.js
. You could just move the logic of config/apm.js
into the getConfig
function of src/apm.js
and so there's only a need for one config file: config/apm.dev.js
. What do you think of that?
I agree with you that it would be simpler and we won't have two config files, But would it be easier for the devs to look at APM specific config inside |
I don't think there's a big need for people to know the default config current set inside the |
@elasticmachine merge upstream |
1 similar comment
@elasticmachine merge upstream |
@elasticmachine merge upstream |
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 run this locally, but looking at the code, it all seems perfect 👍
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 for platform changes
Could you please open an issue for the integration in new platform and ping the team on it?
|
||
apm: getApmConfig(legacyMetadata.app), |
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 @eliperelman Will need to reintegrate this in renderer PR
apm: { | ||
[key: string]: unknown; | ||
}; |
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.
Quick note on this: injectedMetadata are more or less already deprecated and might be removed at some point in NP. You have to keep in mind that you need to migrate this to an asynchronous way to access and use this data at some point.
* In Kibana app, this logic would run after the boostrap js files gets | ||
* downloaded and get associated with the page-load transaction | ||
*/ | ||
$rootScope.$on('$routeChangeStart', (_, nextRoute) => { |
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.
in NP this will probably need to be hooked into ApplicationService.navigateToApp
, However NP does not yet exposes that kind of hooks
@pgayvallet Thanks for the review,I have opened an issue and added the label for the same #53466 |
* Instrument Kibana with APM RUM agent * make route-change transaction work with properl url * extract page-load transaction url from app link * check if app is hidden and set active:false * make distributed tracing work and merge config * remove config/apm.js and address review * address review comments * add apm.js to build tassks * move apm from dev to src * add @types/hoist-non-react-statics which is required by react rum * apply changes correctly from master
* Instrument Kibana with APM RUM agent * make route-change transaction work with properl url * extract page-load transaction url from app link * check if app is hidden and set active:false * make distributed tracing work and merge config * remove config/apm.js and address review * address review comments * add apm.js to build tassks * move apm from dev to src * add @types/hoist-non-react-statics which is required by react rum * apply changes correctly from master
* Instrument Kibana with APM RUM agent (#44281) * Instrument Kibana with APM RUM agent * make route-change transaction work with properl url * extract page-load transaction url from app link * check if app is hidden and set active:false * make distributed tracing work and merge config * remove config/apm.js and address review * address review comments * add apm.js to build tassks * move apm from dev to src * add @types/hoist-non-react-statics which is required by react rum * apply changes correctly from master * Remove unneded changes * fix apminit Co-authored-by: Vignesh Shanmugam <vignesh.shanmugam22@gmail.com>
* Instrument Kibana with APM RUM agent (#44281) * Instrument Kibana with APM RUM agent * make route-change transaction work with properl url * extract page-load transaction url from app link * check if app is hidden and set active:false * make distributed tracing work and merge config * remove config/apm.js and address review * address review comments * add apm.js to build tassks * move apm from dev to src * add @types/hoist-non-react-statics which is required by react rum * apply changes correctly from master * Suggested fixes Co-authored-by: Vignesh Shanmugam <vignesh.shanmugam22@gmail.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
💔 Build FailedHistory
To update your PR or re-run it, just comment with: |
Summary
Instruments Kibana and other X-Pack Plugins(APM UI) with Elastic APM RUM JS agent. The agent is code is bundled on the client side and initialised based on ELASTIC_APM_ACTIVE env flag.
For now, this is meant as a useful internal tool for Kibana developers and will not be distributed with
the Kibana distrubution.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.config/apm.js
Unknown
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n supportUnit or functional tests were updated or added to match the most common scenariosThis was checked for keyboard-only and screenreader accessibilityFor maintainers