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

Migrate observability to new Kibana page template #133021

Merged
merged 15 commits into from
Jun 8, 2022
Merged

Migrate observability to new Kibana page template #133021

merged 15 commits into from
Jun 8, 2022

Conversation

alisonelizabeth
Copy link
Contributor

@alisonelizabeth alisonelizabeth commented May 26, 2022 •
edited
Loading

Summary

In #129323, the KibanaPageTemplate component was migrated to the shared_ux plugin, and the component in kibana_react was deprecated. This PR migrates Observability apps to the new component.

This work was completed in order to simplify work for the guided onboarding project, in which a product tour will be added within Observability. Related to: #132699, #133159.

How to review

The only noticeable visual changes should be to the empty state view, as a custom description is no longer supported. This issue was resolved via #133594.

I have tested these changes to the best of my ability. However, since I am not as familiar with the Observability apps, please make sure each app is verified with the different states.

Screenshots

/overview
Screen Shot 2022-06-08 at 9 58 39 AM

/alerts
Screen Shot 2022-06-08 at 9 58 47 AM

/logs
Screen Shot 2022-06-08 at 9 58 55 AM

/metrics
Screen Shot 2022-06-08 at 9 59 01 AM

/apm
Screen Shot 2022-06-08 at 9 59 08 AM

/uptime
Screen Shot 2022-06-08 at 9 59 14 AM

/ux
Screen Shot 2022-06-08 at 9 59 20 AM

@alisonelizabeth alisonelizabeth added release_note:skip Skip the PR/issue when compiling release notes v8.4.0 chore labels Jun 1, 2022
@alisonelizabeth alisonelizabeth marked this pull request as ready for review June 1, 2022 15:45
@alisonelizabeth alisonelizabeth requested a review from a team June 1, 2022 15:45
@alisonelizabeth alisonelizabeth requested review from a team as code owners June 1, 2022 15:45
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Jun 1, 2022
@elasticmachine
Copy link
Contributor

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

mgiota
mgiota requested changes Jun 3, 2022
View reviewed changes
Copy link
Contributor

@mgiota mgiota left a comment

Choose a reason for hiding this comment

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

@alisonelizabeth There is one more page that is not listed in your screenshots and this is the Observability > Alerts page. The before and after empty state views are different. Is this change intended or was Alerts page missed somehow?

Before

before

After
after

If the change was intended, please re-request a review and I will approve the PR

@weltenwort weltenwort self-requested a review June 3, 2022 14:39
weltenwort
weltenwort reviewed Jun 3, 2022
View reviewed changes
Copy link
Member

@weltenwort weltenwort left a comment •
edited
Loading

Choose a reason for hiding this comment

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

infra plugin changes look mostly good to me, I just left one question below.

x-pack/test/functional/apps/infra/feature_controls/infrastructure_security.ts Outdated
@@ -72,7 +72,7 @@ export default function ({ getPageObjects, getService }: FtrProviderContext) {
ensureCurrentUrl: true,
shouldLoginIfPrompted: false,
});
await testSubjects.existOrFail('~noDataPage');
await testSubjects.existOrFail('kbnNoDataPage');
Copy link
Member

Choose a reason for hiding this comment

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

With this change we'd be testing for a test subject that is an implementation detail of the black box (from the infra perspective) KibanaPageTemplate. If the noDataPage test subject defined by the specific page template is still there, would it make sense to keep using it?

(This applies to the other similar changes to the infra tests as well.)

Copy link
Contributor Author

@alisonelizabeth alisonelizabeth Jun 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Good point! Fixed. @weltenwort Can you take another look?

@clintandrewhall
Copy link
Contributor

@mgiota The "after" screenshot is consistent with the new "No Data" page, which means the "before" was using a bespoke layout that should no longer appear. So it should be correct!

@clintandrewhall clintandrewhall mentioned this pull request Jun 6, 2022
Merged
mgiota
mgiota approved these changes Jun 6, 2022
View reviewed changes
Copy link
Contributor

@mgiota mgiota left a comment

Choose a reason for hiding this comment

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

Cool! I wanted to be sure that Alerts page is consistent with the new change. It would be nice, if you updated your description with a screenshot from Alerts page as well.

@alisonelizabeth
Copy link
Contributor Author

Thanks @clintandrewhall for your help!

To all reviewers - the PR should be updated now with descriptions for the empty prompt. Screenshots in the PR description have also been updated.

@alisonelizabeth
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
observability 419 519 +100

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
observability 379 380 +1

Async chunks

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

id before after diff
apm 2.9MB 2.9MB -1.0B
dashboard 426.3KB 426.3KB +33.0B
discover 515.7KB 515.8KB +33.0B
infra 1001.1KB 1001.1KB -2.0B
lens 1.2MB 1.2MB +33.0B
observability 495.8KB 617.4KB +121.7KB
securitySolution 5.1MB 5.1MB +33.0B
synthetics 858.1KB 858.1KB -1.0B
ux 157.6KB 157.6KB -1.0B
visualizations 287.8KB 287.8KB +33.0B
total +121.8KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
kibana 358 359 +1

Page load bundle

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

id before after diff
observability 63.1KB 63.2KB +106.0B
Unknown metric groups

API count

id before after diff
observability 382 383 +1

async chunk count

id before after diff
observability 16 18 +2

miscellaneous assets size

id before after diff
observability 0.0B 161.8KB +161.8KB

References to deprecated APIs

id before after diff
apm 38 32 -6
infra 38 34 -4
observability 10 3 -7
synthetics 11 9 -2
ux 5 3 -2
total -21

History

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

weltenwort
weltenwort approved these changes Jun 7, 2022
View reviewed changes
Copy link
Member

@weltenwort weltenwort left a comment •
edited
Loading

Choose a reason for hiding this comment

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

infra plugin changes LGTM, thank you!

shahzad31
shahzad31 approved these changes Jun 7, 2022
View reviewed changes
Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

Uptime changes LGTM !!

sorenlouv
sorenlouv approved these changes Jun 7, 2022
View reviewed changes
Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

APM changes lgtm

estermv
estermv approved these changes Jun 8, 2022
View reviewed changes
Copy link
Contributor

@estermv estermv left a comment

Choose a reason for hiding this comment

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

Unified Observability changes LGTM!

@alisonelizabeth alisonelizabeth removed the request for review from yuliacech June 8, 2022 12:17
@alisonelizabeth
Copy link
Contributor Author

@clintandrewhall would you mind taking a final look at the shared-ux changes?

@mgiota
Copy link
Contributor

mgiota commented Jun 8, 2022 •
edited
Loading

@alisonelizabeth Is the screenshot for Alerts in the description the correct one? It looks to me that it is still the old one. Could you update it?

clintandrewhall
clintandrewhall approved these changes Jun 8, 2022
View reviewed changes
Copy link
Contributor

@clintandrewhall clintandrewhall left a comment

Choose a reason for hiding this comment

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

Great work!

@alisonelizabeth alisonelizabeth merged commit 346dea9 into elastic:main Jun 8, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jun 8, 2022
@alisonelizabeth alisonelizabeth deleted the observability/page_template_migration branch June 8, 2022 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cchaos cchaos cchaos left review comments

@sorenlouv sorenlouv sorenlouv approved these changes

@clintandrewhall clintandrewhall clintandrewhall approved these changes

@weltenwort weltenwort weltenwort approved these changes

@estermv estermv estermv approved these changes

@mgiota mgiota mgiota approved these changes

@shahzad31 shahzad31 shahzad31 approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting chore release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support v8.4.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.