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

[APM] Replace custom map with link to Client Side Monitoring #77458

Merged

Conversation

ogupte
Copy link
Contributor

@ogupte ogupte commented Sep 15, 2020

Closes #48538 by replacing the RUM-specific charts (including the custom map for average duration by country) with a callout & link to the client side monitoring app which already displays this data.

Screen Shot 2020-09-17 at 5 25 44 PM

@ogupte ogupte added release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Sep 15, 2020
@ogupte ogupte requested a review from a team as a code owner September 15, 2020 10:03
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Sep 15, 2020
@elasticmachine
Copy link
Contributor

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

const { basePath } = core.http;
const href = basePath.prepend(`/app/csm`);

return <EuiLink {...props} href={href} />;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this use onClick and navigateToUrl to avoid page reload? https://www.elastic.co/guide/en/kibana/master/kibana-navigation.html#navigating-between-kibana-apps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made this open the CSM app in new tab, which is always a full page load. I'm not totally sure if we should open a new tab, so we could end up going with your suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

We're using the RedirectAppLinks component so if it doesn't open in a new tab it will not cause a page reload.

Copy link
Member

Choose a reason for hiding this comment

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

I'm leaning toward not opening in new tab - but would be good to know if there is a consensus around this in Kibana.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should open in a new tab.

@sorenlouv
Copy link
Member

LGTM. @formgeist any thoughts on the design?

@formgeist
Copy link
Contributor

I think we should remove the browser breakdown because this is also available in the new CSM experience. I would add an EuiCallOut instead of a single link that contains a little more information around the new app and what to expect. This is just mock descriptions, but something like this;

screencapture-localhost-5601-dbp-app-apm-services-opbeans-rum-transactions-2020-09-16-11_31_03

The link should not open in a new tab/window.

@sorenlouv
Copy link
Member

sorenlouv commented Sep 16, 2020

Thanks @formgeist. Agree that browser breakdown should also be taken out as well. Callout looks better 👍

@ogupte ogupte force-pushed the apm-48538-remove-duration-map-from-rum-view branch from 7bee08e to ee9b5b5 Compare September 18, 2020 00:28
Copy link
Contributor

@formgeist formgeist left a comment

Choose a reason for hiding this comment

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

Just some suggested new copy for the callout. We probably should also think about not displaying this message if the CSM app is disabled. Adding to that, we might consider have another callout message for that case, so the user is aware they need to enable it to use it. I'll reach out to the RUM team to clarify.

<EuiButton href={clientSideMonitoringHref}>
{i18n.translate(
'xpack.apm.transactionOverview.clientSideMonitoring.linkLabel',
{ defaultMessage: 'Learn more' }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{ defaultMessage: 'Learn more' }
{ defaultMessage: 'Take me there' }

'xpack.apm.transactionOverview.clientSideMonitoring.calloutText',
{
defaultMessage:
'We are introducing a new app which contains breakdown information around browser and location by page load.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'We are introducing a new app which contains breakdown information around browser and location by page load.',
'We are beyond excited to introduce a new experience for analyzing the user experience metrics specifically for your RUM services. It provides insights into the core vitals and visitor breakdown by browser and location. The app is always available in the left sidebar among the other Observability views. ',

iconType="cheer"
title={i18n.translate(
'xpack.apm.transactionOverview.clientSideMonitoring.calloutTitle',
{ defaultMessage: 'New app: Client Side Monitoring' }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{ defaultMessage: 'New app: Client Side Monitoring' }
{ defaultMessage: 'Introducing: Client Side Monitoring' }

@ogupte
Copy link
Contributor Author

ogupte commented Sep 21, 2020

@formgeist

We probably should also think about not displaying this message if the CSM app is disabled.

This is not currently possible since APM and CSM are both enabled/disabled with the same config xpack.apm.enabled. If that changes in the future, we can update this callout to display conditionally.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
apm 1248 -9 1257

async chunks size

id value diff baseline
apm 4.2MB -801.5KB 5.0MB

distributable file count

id value diff baseline
default 45922 -5 45927

History

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

@ogupte ogupte merged commit f015bea into elastic:master Sep 21, 2020
ogupte added a commit to ogupte/kibana that referenced this pull request Sep 21, 2020
…#77458)

* Closes elastic#48538 by removing the average duration by country custom geo map.

* adds link to client side monitoring below the rum charts

* removes unused translations

* Removes RUM-specific chart for Avg Duration By Browser

* Replace link under charts with CallOut above charts with link to Client Side Monitoring

* removes api integration tests for  avg_duration_by_browser and usused i18n translations

* updates to copy in the CSM callout
ogupte added a commit that referenced this pull request Sep 21, 2020
…#78093)

* Closes #48538 by removing the average duration by country custom geo map.

* adds link to client side monitoring below the rum charts

* removes unused translations

* Removes RUM-specific chart for Avg Duration By Browser

* Replace link under charts with CallOut above charts with link to Client Side Monitoring

* removes api integration tests for  avg_duration_by_browser and usused i18n translations

* updates to copy in the CSM callout
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support v7.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Maps] [APM] Use Maps in APM instead of custom solution
7 participants