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

[APM] Add Obs side nav and refactor APM templates #101044

Merged
merged 12 commits into from
Jun 4, 2021
Merged

[APM] Add Obs side nav and refactor APM templates #101044

merged 12 commits into from
Jun 4, 2021

Conversation

sorenlouv
Copy link
Member

@sorenlouv sorenlouv commented Jun 1, 2021 •
edited
Loading

Closes #99883

This PR adds the new Observability Page Template including left side nav

image

I had to refactor quite a few of the existing components, and started templatizing views. This is still WIP and I want to get people's take on the current direction of this PR.

sorenlouv
sorenlouv commented Jun 1, 2021
View reviewed changes
x-pack/plugins/apm/common/environment_filter_values.ts
text: string;
}) {
return { value, text };
}
Copy link
Member Author

@sorenlouv sorenlouv Jun 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is needed due to a console error introduced in #95903 (comment)

I don't think the console error in itself is an issue but it made it a bit harder to make the changes needed for this PR when the console contained unrelated errors.

sorenlouv
sorenlouv commented Jun 1, 2021
View reviewed changes
x-pack/plugins/apm/public/application/index.tsx
</RedirectAppLinks>
);
}
import { ApmAppRoot } from '../components/routing/app_root';
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved all of this to the routing section (which contains routing and templating)

sorenlouv
sorenlouv commented Jun 1, 2021
View reviewed changes
x-pack/plugins/apm/public/components/app/service_node_metrics/index.tsx
@@ -180,27 +168,26 @@ export function ServiceNodeMetrics({ match }: ServiceNodeMetricsProps) {
</MetadataFlexGroup>
)}
<SearchBar />
<EuiPage>
Copy link
Member Author

@sorenlouv sorenlouv Jun 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I've removed EuiPage everywhere. Everything is now wrapped in EuiPage automatically when using ApmMainTemplate

sorenlouv
sorenlouv commented Jun 1, 2021
View reviewed changes
x-pack/plugins/apm/public/components/app/service_node_overview/index.tsx
@@ -145,25 +145,19 @@ function ServiceNodeOverview({ serviceName }: ServiceNodeOverviewProps) {
return (
<>
<SearchBar />
<EuiPage>
<EuiFlexGroup direction="column" gutterSize="s">
Copy link
Member Author

Choose a reason for hiding this comment

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

We had a lot of unnecessary EuiFlexGroup wrappers. Removed them

sorenlouv
sorenlouv commented Jun 1, 2021
View reviewed changes
x-pack/plugins/apm/public/components/app/service_node_overview/index.tsx Outdated
</EuiFlexGroup>
</EuiPage>

<EuiPanel hasShadow={false}>
Copy link
Member Author

@sorenlouv sorenlouv Jun 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If there's only a single EuiPanel in a view we use hasShadow={false}. When there are multiple panels we still want the drop shadow.

sorenlouv
sorenlouv commented Jun 1, 2021
View reviewed changes
x-pack/plugins/apm/public/components/routing/render_as_redirect.tsx Outdated
@@ -0,0 +1,38 @@
/*
Copy link
Member Author

@sorenlouv sorenlouv Jun 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

renderAsRedirectTo is not new. It was just extracted from another file

sorenlouv
sorenlouv commented Jun 1, 2021
View reviewed changes
x-pack/plugins/apm/public/components/routing/templates/apm_main_template.tsx
* Optionally:
* - EnvironmentFilter
*/
export function ApmMainTemplate({
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the core of this PR: an apm-wide template that should contain all elements that most views need (left-side nav, top-bar action menu, page title, environment filter)

@sorenlouv sorenlouv marked this pull request as ready for review June 2, 2021 15:20
@sorenlouv sorenlouv requested review from a team as code owners June 2, 2021 15:20
@botelastic botelastic bot added Team:APM All issues that need APM UI Team support Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Jun 2, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@elasticmachine
Copy link
Contributor

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

@cauemarcondes
Copy link
Contributor

@sqren do you mind running the e2e test, I think it might break.

node  x-pack/plugins/apm/scripts/ftr_e2e/cypress_run.js

(It's not running on CI yet, I still need to make a few improvements.)

@sorenlouv
Copy link
Member Author

retest

1 similar comment
@sorenlouv
Copy link
Member Author

retest

cauemarcondes
cauemarcondes approved these changes Jun 3, 2021
View reviewed changes
Copy link
Contributor

@cauemarcondes cauemarcondes left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the clean up 👍🏻.

@sorenlouv
Copy link
Member Author

@elasticmachine merge upstream

@sorenlouv
Copy link
Member Author

jenkins run the e2e

smith
smith approved these changes Jun 3, 2021
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.

Thank you! This looks great and tons of good cleanup.

x-pack/plugins/apm/public/components/routing/templates/apm_main_template.tsx Outdated
children,
}: {
pageTitle: React.ReactNode;
environmentSelector?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Should this be hasEnvironmentSelector or showEnvironmentSelector to indicate it's a boolean. As it's named it makes it look like I'm supposed to supply some sort of environmentSelector here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. Will rename

@sorenlouv sorenlouv mentioned this pull request Jun 3, 2021
Open
@sorenlouv sorenlouv enabled auto-merge (squash) June 3, 2021 23:12
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
apm 1604 1600 -4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
apm 4.3MB 4.3MB -8.7KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
apm 31.9KB 32.5KB +579.0B

History

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

@sorenlouv sorenlouv merged commit caa4bd1 into elastic:master Jun 4, 2021
@formgeist formgeist mentioned this pull request Jun 4, 2021
Closed
@sorenlouv sorenlouv mentioned this pull request Jun 4, 2021
Merged
9 tasks
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 8, 2021
@kibanamachine
Copy link
Contributor

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

@sorenlouv sorenlouv added the auto-backport Deprecated - use backport:version if exact versions are needed label Jun 8, 2021
@kibanamachine kibanamachine mentioned this pull request Jun 8, 2021
Merged
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Jun 8, 2021
@sorenlouv @kibanamachine
[APM] Add Obs side nav and refactor APM templates (elastic#101044)
fd38b87 
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
✅ 7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Jun 8, 2021
@kibanamachine @sorenlouv
[APM] Add Obs side nav and refactor APM templates (#101044) (#101559)
d117ba7 
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Søren Louv-Jansen <sorenlouv@gmail.com>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 8, 2021
@smith smith mentioned this pull request Jun 9, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cauemarcondes cauemarcondes cauemarcondes approved these changes

@shahzad31 shahzad31 shahzad31 approved these changes

@smith smith smith approved these changes

Assignees
No one assigned
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:enhancement Team:APM All issues that need APM UI Team support Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.14.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[APM] Convert to the new Observability page template and remove tab navigation from the global views
6 participants
@sorenlouv @elasticmachine @cauemarcondes @kibanamachine @smith @shahzad31