Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: rum agent should work correctly on new platform #67037

Merged
merged 5 commits into from
May 25, 2020
Merged

fix: rum agent should work correctly on new platform #67037

merged 5 commits into from
May 25, 2020

Conversation

vigneshshanmugam
Copy link
Member

@vigneshshanmugam vigneshshanmugam commented May 19, 2020 •
edited
Loading

Summary

  • fixes Error loading Kibana UI when started with ELASTIC_APM_ACTIVE=true #66583
  • Since we don't have a nice way to get the currently navigated app after the Kibana is bootstrapped, we will use the app.getId() which would return core for NP and consolidate all transactions under core. Issue to track this Integration of the APM RUM instrumentation on New Platform.  #53466
  • Updated Node.js agent to 3.6.0 which has the correct compatibility with RUM agent to make distributed tracing work as expected and also support Service Maps.
  • Currently the RUM agent is disabled even if we set ELASTIC_APM_ACTIVE flag to true since we dont have the environment variable set on webpack bundles. In order to fix this, We use the apmConfig as a source to activate the RUM agent on the NP as its better than setting ELASTIC_APM_ACTIVE on existing webpack bundles.
  • Added some documentation on enabling both RUM and Node.js. agent.

There is an existing issue in RUM agent which would be needed to associate APM UI correctly with RUM route-change transactions. elastic/apm-agent-rum-js#791

Screenshot 2020-05-19 at 18 00 03

Checklist

For maintainers

@vigneshshanmugam vigneshshanmugam requested a review from a team as a code owner May 19, 2020 16:37
@vigneshshanmugam
Copy link
Member Author

@elasticmachine merge upstream

@vigneshshanmugam vigneshshanmugam added the Team:APM All issues that need APM UI Team support label May 19, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

@vigneshshanmugam vigneshshanmugam added release_note:skip Skip the PR/issue when compiling release notes v8.0.0 labels May 19, 2020
joshdover
joshdover reviewed May 19, 2020
View reviewed changes
src/legacy/ui/apm/index.js
*/
const navLink = appMetadata.getNavLink();
const pageUrl = navLink ? navLink.toJSON().url : appMetadata._url;
const appId = app.getId();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the NP this will always be core, so I'm not sure that's very useful?

Copy link
Member Author

@vigneshshanmugam vigneshshanmugam May 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have left a comment in the description on why its like this for now.

Since we don't have a nice way to get the currently navigated app after the Kibana is bootstrapped, we will use the app.getId() which would return core for NP and consolidate all transactions under core. Issue to track this #53466

I found other way to solve this was to subscribing to currentAppId and updating the transaction. But that wont work as well since it happens after the kibana app is bootstrapped. I guess we need a better way to group them right from the time the agent is initialised on. Like from the server before the agent is itself initialised, need access to the current app. Do you have any other ideas?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps add a comment about this in the code as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I will add it on my upcoming PR which I am experimenting with using window.location.href since we are using it only for dev environment and can use it till we get a solution in NP.

smith
smith approved these changes May 19, 2020
View reviewed changes
Copy link
Contributor

@smith smith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works for me and I've got data showing up from my dev machine.

I noticed that on the front end any route-change transactions show up as "Unknown" but this was probably the case before and not something new.

@vigneshshanmugam
Copy link
Member Author

@smith Yeah that Unknown issue would be solved by this one elastic/apm-agent-rum-js#791. Thanks for the review 👍

@vigneshshanmugam
Copy link
Member Author

@elasticmachine merge upstream

@vigneshshanmugam
Copy link
Member Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💛 Build succeeded, but was flaky

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@vigneshshanmugam vigneshshanmugam merged commit faaafd6 into elastic:master May 25, 2020
@vigneshshanmugam vigneshshanmugam deleted the fix-apm-rum branch May 25, 2020 11:14
@vigneshshanmugam vigneshshanmugam mentioned this pull request May 25, 2020
Closed
jloleysens added a commit that referenced this pull request May 26, 2020
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into feature/pipel…
f06f9e4 
…ine-editor

* 'master' of github.com:elastic/kibana: (129 commits)
  [Canvas] Force embeddables to refresh when renderable reevaluated (#67133)
  [Canvas] Better handling navigating to/from canvas (#66407)
  [Ingest pipelines] Fix schema validation for simulate and update routes (#67199)
  do not use es from setup (#67277)
  Auto expand replicas for event log (#67286)
  Observability & APM do not use elasticsearch client provided via setup contract  (#67263)
  Fix privileges check when security is not enabled (#67308)
  add IIS home (#66918)
  [ML] Adding additional job service endpoint tests (#66892)
  [Ingest Manager] Update fleet internal doc with latest flags (#67193)
  [Discover] Deangularize the loading spinner (#67165)
  Add `application.navigateToUrl` core API (#67110)
  Improve indexpattern without timefield functional test (#67031)
  KibanaContext in index pattern managment ui (#66985)
  Fix Azure metrics tutorial inside the App Home/ Add data area (#66901)
  add azure logs home (#66910)
  fix: rum agent should work correctly on new platform (#67037)
  [test_utils/Testbed] Move to src/test_utils folder (OSS) (#66898)
  only block registration when appRoute contains the exact basePath (#67125)
  Changed actions API endpoints urls to follow Kibana STYLEGUIDE (#65936)
  ...

# Conflicts:
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_fields.tsx
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label May 27, 2020
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 67037 or prevent reminders by adding the backport:skip label.

@vigneshshanmugam vigneshshanmugam mentioned this pull request May 27, 2020
Merged
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label May 28, 2020
vigneshshanmugam added a commit that referenced this pull request May 28, 2020
@vigneshshanmugam @elasticmachine
fix: rum agent should work correctly on new platform (#67037) (#67477)
d49eb7f 
* fix: rum agent should work correctly on new platform

* chore: add IS_KIBANA_DISTRIBUTABLE flag back

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sorenlouv sorenlouv sorenlouv left review comments

@smith smith smith approved these changes

@joshdover joshdover joshdover approved these changes

Assignees
No one assigned
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error loading Kibana UI when started with ELASTIC_APM_ACTIVE=true
6 participants
@vigneshshanmugam @elasticmachine @kibanamachine @smith @sorenlouv @joshdover