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.

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

[RUM Dashboard] Create new path for client side monitoring #74740

Merged
merged 25 commits into from
Aug 19, 2020

Conversation

shahzad31
Copy link
Contributor

@shahzad31 shahzad31 commented Aug 11, 2020

Summary

Fixes: elastic/uptime#240
Created a new entry point for client side monitoring solutions, code as of now will remain part of APM, so simply just registered a new feature.

For permission perspective, it will continue to use apm_user role.

Also used lazy loading in plugin setup to reduce initial page load bundle size.

image

@andrewvc andrewvc self-requested a review August 11, 2020 15:48
@shahzad31 shahzad31 marked this pull request as ready for review August 11, 2020 16:05
@shahzad31 shahzad31 requested a review from a team as a code owner August 11, 2020 16:05
@shahzad31 shahzad31 self-assigned this Aug 11, 2020
@shahzad31 shahzad31 added v7.10.0 release_note:enhancement v8.0.0 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Aug 11, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@shahzad31 shahzad31 requested a review from sorenlouv August 11, 2020 16:07
@shahzad31 shahzad31 requested a review from a team as a code owner August 12, 2020 08:53
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Aug 12, 2020
@elasticmachine
Copy link
Contributor

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

@legrego legrego self-requested a review August 12, 2020 10:54
Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

Sorry for my lack of understanding, but I need some more context. I'm also sorry for the wall of text that follows.

From what I can tell, this appears to be existing functionality that we are extracting from APM into its own top-level application/feature. If that is the case, then I have some thoughts around the privilege/access model.

  1. APM users in 7.9 have access to this feature within APM itself. After upgrading, they won't have access to this anymore, based on the way this is currently configured. I don't think this is what you want, as it's considered a breaking change to take features away from users who were previously authorized.
  2. Does APM continue to function if the user doesn't have access to this new Client Side Monitoring app?
  3. Does Client Side Monitoring function if the user doesn't have access to the APM feature? What type of access is required for me to test and verify this?
    3a) Does Client Side Monitoring rely on any API routes defined by the APM application? APM secures access to its routes using the access:apm tag, and others. Client Side Monitoring is not configured to grant access to these APIs, so if you're relying on them, then we'll need to adjust the feature definition accordingly.
    3b) Does Client Side Monitoring rely on any UI Capabilities defined by the APM feature? If so, how will this work for users who don't have access to APM itself?
  4. Should users with the existing apm_user role have access to this new Client Side Monitoring feature?

x-pack/plugins/apm/server/feature.ts Outdated Show resolved Hide resolved
x-pack/plugins/apm/public/plugin.ts Outdated Show resolved Hide resolved
x-pack/plugins/apm/server/feature.ts Outdated Show resolved Hide resolved
x-pack/plugins/apm/server/feature.ts Outdated Show resolved Hide resolved
x-pack/plugins/apm/server/feature.ts Outdated Show resolved Hide resolved
x-pack/plugins/apm/server/feature.ts Outdated Show resolved Hide resolved
x-pack/plugins/apm/server/feature.ts Outdated Show resolved Hide resolved
x-pack/plugins/apm/server/feature.ts Outdated Show resolved Hide resolved
x-pack/plugins/apm/server/feature.ts Outdated Show resolved Hide resolved
@shahzad31
Copy link
Contributor Author

Sorry for my lack of understanding, but I need some more context. I'm also sorry for the wall of text that follows.

From what I can tell, this appears to be existing functionality that we are extracting from APM into its own top-level application/feature. If that is the case, then I have some thoughts around the privilege/access model.

  1. APM users in 7.9 have access to this feature within APM itself. After upgrading, they won't have access to this anymore, based on the way this is currently configured. I don't think this is what you want, as it's considered a breaking change to take features away from users who were previously authorized.
  2. Does APM continue to function if the user doesn't have access to this new Client Side Monitoring app?
  3. Does Client Side Monitoring function if the user doesn't have access to the APM feature? What type of access is required for me to test and verify this?
    3a) Does Client Side Monitoring rely on any API routes defined by the APM application? APM secures access to its routes using the access:apm tag, and others. Client Side Monitoring is not configured to grant access to these APIs, so if you're relying on them, then we'll need to adjust the feature definition accordingly.
    3b) Does Client Side Monitoring rely on any UI Capabilities defined by the APM feature? If so, how will this work for users who don't have access to APM itself?
  4. Should users with the existing apm_user role have access to this new Client Side Monitoring feature?

@legrego thanks for the feedback, So this was kind of hidden feature within apm in 7.9, which was only accessible through a URL, acting kind of as a preview for users.

So now it will be totally dependant on apm_user role. I have removed the new server feature file.

Only apm users will be able to access this new feature.

@legrego legrego self-requested a review August 12, 2020 19:43
@shahzad31 shahzad31 removed the request for review from sorenlouv August 13, 2020 09:48
@legrego legrego removed their request for review August 13, 2020 11:43
@legrego
Copy link
Member

legrego commented Aug 13, 2020

Thanks for the updates @shahzad31 - your new changes mean you don't technically need my review anymore, so LGTM from a security perspective

Copy link
Member

@dgieselaar dgieselaar left a comment

Choose a reason for hiding this comment

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

Mostly some remarks around parallelisation and a bug in the observability dashboard. Also I think we should use csm consistently rather than rum or clientSideMonitoring where appropriate, but will leave that up to you.

x-pack/plugins/apm/public/plugin.ts Outdated Show resolved Hide resolved
x-pack/plugins/apm/public/plugin.ts Outdated Show resolved Hide resolved
x-pack/plugins/apm/public/application/index.tsx Outdated Show resolved Hide resolved
x-pack/plugins/apm/public/application/rumApp.tsx Outdated Show resolved Hide resolved
x-pack/plugins/apm/public/plugin.ts Outdated Show resolved Hide resolved
x-pack/plugins/apm/public/plugin.ts Show resolved Hide resolved
x-pack/plugins/apm/public/plugin.ts Show resolved Hide resolved
x-pack/plugins/apm/public/plugin.ts Outdated Show resolved Hide resolved
@shahzad31
Copy link
Contributor Author

@dgieselaar Thanks for the feedback, i have addressed all of it.

@shahzad31 shahzad31 requested a review from dgieselaar August 17, 2020 09:48
x-pack/plugins/apm/public/plugin.ts Show resolved Hide resolved
x-pack/plugins/apm/public/plugin.ts Outdated Show resolved Hide resolved
x-pack/plugins/apm/public/plugin.ts Outdated Show resolved Hide resolved
x-pack/plugins/apm/public/plugin.ts Outdated Show resolved Hide resolved
x-pack/plugins/apm/public/application/csmApp.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@andrewvc andrewvc left a comment

Choose a reason for hiding this comment

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

I think @dgieselaar has done a better job reviewing here than I can, so I have nothing to add other than confirming his findings.

@shahzad31 shahzad31 requested a review from dgieselaar August 18, 2020 12:14
Copy link
Member

@dgieselaar dgieselaar left a comment

Choose a reason for hiding this comment

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

I think there's one or two lines that could be removed and a translation that should be added, but other than that, LGTM!

x-pack/plugins/apm/public/application/csmApp.tsx Outdated Show resolved Hide resolved
@kibanamachine
Copy link
Contributor

💛 Build succeeded, but was flaky


Test Failures

Plugin Functional Tests.test/plugin_functional/test_suites/core/route·ts.core route timeouts idle socket should timeout if servers response is too slow

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has not failed recently on tracked branches

[00:00:00]       │
[00:00:00]         └-: core
[00:00:00]           └-> "before all" hook
[00:00:00]           └-: route
[00:00:00]             └-> "before all" hook
[00:00:00]             └-: timeouts
[00:00:00]               └-> "before all" hook
[00:00:00]               └-: idle socket
[00:00:00]                 └-> "before all" hook
[00:00:00]                 └-> should timeout if payload sending has too long of an idle period
[00:00:00]                   └-> "before each" hook: global before each
[00:00:00]                   └- ✓ pass  (55ms) "core route timeouts idle socket should timeout if payload sending has too long of an idle period"
[00:00:00]                 └-> should not timeout if payload sending does not have too long of an idle period
[00:00:00]                   └-> "before each" hook: global before each
[00:00:00]                   └- ✓ pass  (230ms) "core route timeouts idle socket should not timeout if payload sending does not have too long of an idle period"
[00:00:00]                 └-> should timeout if servers response is too slow
[00:00:00]                   └-> "before each" hook: global before each
[00:00:00]                   │ info Taking screenshot "/dev/shm/workspace/parallel/2/kibana/test/functional/screenshots/failure/core route timeouts idle socket should timeout if servers response is too slow.png"
[00:00:01]                   │ info Current URL is: data:/,
[00:00:01]                   │ info Saving page source to: /dev/shm/workspace/parallel/2/kibana/test/plugin_functional/failure_debug/html/core route timeouts idle socket should timeout if servers response is too slow.html
[00:00:01]                   └- ✖ fail: core route timeouts idle socket should timeout if servers response is too slow
[00:00:01]                   │      Error: expected 'read ECONNRESET' to equal 'socket hang up'
[00:00:01]                   │       at Assertion.assert (packages/kbn-expect/expect.js:100:11)
[00:00:01]                   │       at Assertion.be.Assertion.equal (packages/kbn-expect/expect.js:227:8)
[00:00:01]                   │       at Assertion.be (packages/kbn-expect/expect.js:69:22)
[00:00:01]                   │       at result.then.err (test/plugin_functional/test_suites/core/route.ts:147:38)
[00:00:01]                   │       at process._tickCallback (internal/process/next_tick.js:68:7)
[00:00:01]                   │ 
[00:00:01]                   │ 

Stack Trace

Error: expected 'read ECONNRESET' to equal 'socket hang up'
    at Assertion.assert (packages/kbn-expect/expect.js:100:11)
    at Assertion.be.Assertion.equal (packages/kbn-expect/expect.js:227:8)
    at Assertion.be (packages/kbn-expect/expect.js:69:22)
    at result.then.err (test/plugin_functional/test_suites/core/route.ts:147:38)
    at process._tickCallback (internal/process/next_tick.js:68:7)

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
apm 942 +1 941

async chunks size

id value diff baseline
apm 4.7MB ⚠️ +1.0MB 3.7MB

page load bundle size

id value diff baseline
apm 40.6KB -217.4KB 258.0KB

History

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

@shahzad31 shahzad31 merged commit 2c865f5 into elastic:master Aug 19, 2020
@shahzad31 shahzad31 deleted the rum-sidebar-link branch August 19, 2020 17:05
shahzad31 added a commit to shahzad31/kibana that referenced this pull request Aug 19, 2020
@sorenlouv sorenlouv removed the Team:APM All issues that need APM UI Team support label Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:enhancement Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move RUM tab to sidebar
7 participants