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

Implement new o11y nav hierarchy for serverless #193510

Merged
merged 13 commits into from
Nov 8, 2024

Conversation

mgiota
Copy link
Contributor

@mgiota mgiota commented Sep 20, 2024

Fixes #192804

This PR updates the nav hierarchy on serverless and changes the accordion to panelOpener. The menu items have been updated according to the Figma file. Here's a Video of how nav hierarchy looks like on serverless with the new changes:

Screen.Recording.2024-10-07.at.13.39.30.mov

What was changed

  • AI & ML menu is removed and split into
    • AI Assistant
    • Machine learning
  • Applications now opens a panelOpener instead of an accordion
    • Service Inventory was renamed to Service inventory to meet the use of sentence-case requirement
    • Synthetics was moved to a new section
  • Infrastructure
    • Infrastructure Inventory was renamed to Infrastructure inventory to match the sentence-case requirement
  • Machine learning: this menu was not present at all on serverless

Notes for the Reviewer

  • Stack Management on security and search don't use any panelOpener, they use a landing page instead. In order to be consistent with the rest solutions, I kept Stack Management as is.
  • Machine Learning menu item was not present at all on serverless. I need a confirmation, that it is fine to bring it in as it is from stateful cc @vinaychandrasekhar

@obltmachine
Copy link

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

@mgiota
Copy link
Contributor Author

mgiota commented Oct 7, 2024

@elasticmachine merge upstream

@mgiota mgiota marked this pull request as ready for review October 7, 2024 11:18
@mgiota mgiota requested a review from a team as a code owner October 7, 2024 11:18
@mgiota mgiota self-assigned this Oct 7, 2024
@mgiota mgiota added release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Team:obs-ux-management Observability Management User Experience Team labels Oct 7, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-management-team (Team:obs-ux-management)

@mgiota mgiota added the v9.0.0 label Oct 7, 2024
@botelastic botelastic bot added the ci:project-deploy-observability Create an Observability project label Oct 7, 2024
@kibana-ci
Copy link
Collaborator

kibana-ci commented Oct 7, 2024

💔 Build Failed

Failed CI Steps

History

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

cc @mgiota

@mgiota mgiota force-pushed the change_navigation_serverless branch from 5531ade to 1edc895 Compare October 7, 2024 19:21
@mgiota
Copy link
Contributor Author

mgiota commented Oct 30, 2024

@elasticmachine merge upstream

@mgiota mgiota requested a review from a team as a code owner November 2, 2024 00:15
@mgiota
Copy link
Contributor Author

mgiota commented Nov 2, 2024

@elasticmachine merge upstream

@mgiota mgiota requested a review from a team as a code owner November 4, 2024 13:06
@mgiota
Copy link
Contributor Author

mgiota commented Nov 4, 2024

@elasticmachine merge upstream

Copy link
Member

@wayneseymour wayneseymour left a comment

Choose a reason for hiding this comment

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

Just one question.
I do like the existOrFail call, but wondering if the last click needs an assertion call.

timeout: 60 * 1000,
});
await testSubjects.click(`~nav-item-observability_project_nav.aiops.ml:${id}`);
await testSubjects.click(`~panelNavItem-id-ml:${id}`);
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps similar to line #19, is this a good place for a missingOrFail() call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wayneseymour I see your point and makes sense what you say, I totally agree! FYI I didn't write these tests, I am adapting them to meet some new navigation changes (we replaced accordion with slideOpener).

I checked the file that is using the navigateToAnomalyDetection helper, and there are indeed quite a few assertion calls, so I think we should be fine.

I also checked security navigation tests and they use the same pattern. Navigation helpers find the nav items and click on them. Then the actual test files do all the assertions.

Copy link
Member

Choose a reason for hiding this comment

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

Ok cool beans. Have you tested this against MKI?

Copy link
Contributor Author

@mgiota mgiota Nov 6, 2024

Choose a reason for hiding this comment

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

No. Do you have any instructions on how I can test against MKI?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@mgiota mgiota Nov 7, 2024

Choose a reason for hiding this comment

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

@wayneseymour I need some help here, I am stuck and I don't want to block this PR more. I tried to test against MKI using the approach my team shared with me, but with that approach it looks like tests are not being run from my local Kibana. Here's what I used:

TEST_CLOUD_HOST_NAME=console.qa.cld.elstc.co TEST_CLOUD=1 NODE_TLS_REJECT_UNAUTHORIZED=0 TEST_ES_URL="https://testing-internal/:NEW-PASSWORD@es-host:443" TEST_KIBANA_URL="https://testing-internal/:NEW-PASSWORD@kbn-host" node  --no-warnings scripts/functional_test_runner --exclude-tag skipMKI --exclude-tag failsOnMKI --config x-pack/test_serverless/functional/test_suites/observability/config.ts

For es-host and kbn-host I used the ones from the project I created on console.qa cloud and for PASSWORD I used the one I got calling the reset credential api. So when I used above command from my local branch to run navigation functional tests, I could easily tell that it is not reading my local kibana source code, since I could see the old navigation.

I am trying now to follow the steps in the document you shared with me. I cloned the qaf repo locally. What do I do next? How do I use the qaf command?

I found the docker image location on the buildkite job page (docker.elastic.co/kibana-ci/kibana-serverless:pr-193510-11cd7adb5e79). What do I do next?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wayneseymour I managed to set up qaf locally and I was able to run the tests against MKI using the docker image from my PR. Can I have the final approval for this PR?

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 ran serverless navigation and ml tests, and they are green!
Screenshot 2024-11-08 at 14 02 09

Copy link
Contributor

@yngrdyn yngrdyn left a comment

Choose a reason for hiding this comment

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

Test changes LGTM

@mgiota mgiota requested a review from wayneseymour November 5, 2024 08:19
@maryam-saeidi maryam-saeidi self-requested a review November 5, 2024 09:51
Copy link
Member

@maryam-saeidi maryam-saeidi left a comment

Choose a reason for hiding this comment

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

LGTM! Added some clarification questions.

},
{ link: 'inventory', spaceBefore: 'm' },
{
id: 'apm',
Copy link
Member

Choose a reason for hiding this comment

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

I am not very familiar with nav menu, what is this id used for? When do we need to have an id for a navigation item?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you don't provide the id, the link will be the id. If there are no link like here as it is the parent group, the default id is auto generated based on the position of the node in the tree (e.g. group_0_child_1_group3), which is not very human friendly. You might want to override that by a clearer id that you can use in your tests.

},
{
link: 'ml:anomalyExplorer',
link: 'apm:services',
Copy link
Member

Choose a reason for hiding this comment

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

How does this link work?

@mgiota
Copy link
Contributor Author

mgiota commented Nov 8, 2024

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

merge conflict between base and head

@elasticmachine
Copy link
Contributor

elasticmachine commented Nov 8, 2024

💚 Build Succeeded

  • Buildkite Build
  • Commit: 292a8e7
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-193510-292a8e765dfe

Metrics [docs]

Page load bundle

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

id before after diff
serverlessObservability 27.2KB 28.9KB +1.7KB

History

cc @mgiota

Copy link
Member

@wayneseymour wayneseymour left a comment

Choose a reason for hiding this comment

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

x-pack/test_serverless/functional/services/ml/observability_navigation.ts LGTM

@mgiota mgiota merged commit 0e736e3 into elastic:main Nov 8, 2024
23 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11741968968

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 8, 2024
Fixes elastic#192804

This PR updates the nav hierarchy on serverless and changes the
`accordion` to `panelOpener`. The menu items have been updated according
to the [Figma
file](https://www.figma.com/design/IAR7FjBaSCDWypNYL83fzy/Observability-Navigation?node-id=1232-10087&node-type=frame&t=AMlUqaK2UhhiyqGi-0).
Here's a Video of how nav hierarchy looks like on serverless with the
new changes:

https://github.com/user-attachments/assets/55d04969-379e-4cd1-8e25-d50382cf51e0

## What was changed

- AI & ML menu is removed and split into
  - `AI Assistant`
  - `Machine learning`
- `Applications` now opens a `panelOpener` instead of an `accordion`
- `Service Inventory` was renamed to `Service inventory` to meet the use
of sentence-case requirement
  - Synthetics was moved to a new section
- `Infrastructure`
- `Infrastructure Inventory` was renamed to `Infrastructure inventory`
to match the sentence-case requirement
-  `Machine learning`: this menu was not present at all on serverless

## Notes for the Reviewer

- `Stack Management` on security and search don't use any `panelOpener`,
they use a landing page instead. In order to be consistent with the rest
solutions, I kept Stack Management as is.
- `Machine Learning` menu item was not present at all on serverless. I
need a confirmation, that it is fine to bring it in as it is from
stateful cc @vinaychandrasekhar

---------

Co-authored-by: Elastic Machine <[email protected]>
(cherry picked from commit 0e736e3)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

rbrtj pushed a commit to rbrtj/kibana that referenced this pull request Nov 8, 2024
Fixes elastic#192804

This PR updates the nav hierarchy on serverless and changes the
`accordion` to `panelOpener`. The menu items have been updated according
to the [Figma
file](https://www.figma.com/design/IAR7FjBaSCDWypNYL83fzy/Observability-Navigation?node-id=1232-10087&node-type=frame&t=AMlUqaK2UhhiyqGi-0).
Here's a Video of how nav hierarchy looks like on serverless with the
new changes:


https://github.com/user-attachments/assets/55d04969-379e-4cd1-8e25-d50382cf51e0

## What was changed

- AI & ML menu is removed and split into
  - `AI Assistant`
  - `Machine learning`
- `Applications` now opens a `panelOpener` instead of an `accordion`
- `Service Inventory` was renamed to `Service inventory` to meet the use
of sentence-case requirement
  - Synthetics was moved to a new section
- `Infrastructure`
- `Infrastructure Inventory` was renamed to `Infrastructure inventory`
to match the sentence-case requirement
-  `Machine learning`: this menu was not present at all on serverless 

## Notes for the Reviewer

- `Stack Management` on security and search don't use any `panelOpener`,
they use a landing page instead. In order to be consistent with the rest
solutions, I kept Stack Management as is.
- `Machine Learning` menu item was not present at all on serverless. I
need a confirmation, that it is fine to bring it in as it is from
stateful cc @vinaychandrasekhar

---------

Co-authored-by: Elastic Machine <[email protected]>
kibanamachine added a commit that referenced this pull request Nov 8, 2024
)

# Backport

This will backport the following commits from `main` to `8.x`:
- [Implement new o11y nav hierarchy for serverless
(#193510)](#193510)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Panagiota
Mitsopoulou","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-11-08T12:18:50Z","message":"Implement
new o11y nav hierarchy for serverless (#193510)\n\nFixes
https://github.com/elastic/kibana/issues/192804\r\n\r\nThis PR updates
the nav hierarchy on serverless and changes the\r\n`accordion` to
`panelOpener`. The menu items have been updated according\r\nto the
[Figma\r\nfile](https://www.figma.com/design/IAR7FjBaSCDWypNYL83fzy/Observability-Navigation?node-id=1232-10087&node-type=frame&t=AMlUqaK2UhhiyqGi-0).\r\nHere's
a Video of how nav hierarchy looks like on serverless with the\r\nnew
changes:\r\n\r\n\r\nhttps://github.com/user-attachments/assets/55d04969-379e-4cd1-8e25-d50382cf51e0\r\n\r\n##
What was changed\r\n\r\n- AI & ML menu is removed and split into\r\n -
`AI Assistant`\r\n - `Machine learning`\r\n- `Applications` now opens a
`panelOpener` instead of an `accordion`\r\n- `Service Inventory` was
renamed to `Service inventory` to meet the use\r\nof sentence-case
requirement\r\n - Synthetics was moved to a new section\r\n-
`Infrastructure`\r\n- `Infrastructure Inventory` was renamed to
`Infrastructure inventory`\r\nto match the sentence-case
requirement\r\n- `Machine learning`: this menu was not present at all on
serverless \r\n\r\n## Notes for the Reviewer\r\n\r\n- `Stack Management`
on security and search don't use any `panelOpener`,\r\nthey use a
landing page instead. In order to be consistent with the
rest\r\nsolutions, I kept Stack Management as is.\r\n- `Machine
Learning` menu item was not present at all on serverless. I\r\nneed a
confirmation, that it is fine to bring it in as it is from\r\nstateful
cc @vinaychandrasekhar\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic
Machine
<[email protected]>","sha":"0e736e31176fafa0059773d351782cb0707db426","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","backport:prev-minor","ci:build-serverless-image","ci:project-deploy-observability","Team:obs-ux-management"],"title":"Implement
new o11y nav hierarchy for
serverless","number":193510,"url":"https://github.com/elastic/kibana/pull/193510","mergeCommit":{"message":"Implement
new o11y nav hierarchy for serverless (#193510)\n\nFixes
https://github.com/elastic/kibana/issues/192804\r\n\r\nThis PR updates
the nav hierarchy on serverless and changes the\r\n`accordion` to
`panelOpener`. The menu items have been updated according\r\nto the
[Figma\r\nfile](https://www.figma.com/design/IAR7FjBaSCDWypNYL83fzy/Observability-Navigation?node-id=1232-10087&node-type=frame&t=AMlUqaK2UhhiyqGi-0).\r\nHere's
a Video of how nav hierarchy looks like on serverless with the\r\nnew
changes:\r\n\r\n\r\nhttps://github.com/user-attachments/assets/55d04969-379e-4cd1-8e25-d50382cf51e0\r\n\r\n##
What was changed\r\n\r\n- AI & ML menu is removed and split into\r\n -
`AI Assistant`\r\n - `Machine learning`\r\n- `Applications` now opens a
`panelOpener` instead of an `accordion`\r\n- `Service Inventory` was
renamed to `Service inventory` to meet the use\r\nof sentence-case
requirement\r\n - Synthetics was moved to a new section\r\n-
`Infrastructure`\r\n- `Infrastructure Inventory` was renamed to
`Infrastructure inventory`\r\nto match the sentence-case
requirement\r\n- `Machine learning`: this menu was not present at all on
serverless \r\n\r\n## Notes for the Reviewer\r\n\r\n- `Stack Management`
on security and search don't use any `panelOpener`,\r\nthey use a
landing page instead. In order to be consistent with the
rest\r\nsolutions, I kept Stack Management as is.\r\n- `Machine
Learning` menu item was not present at all on serverless. I\r\nneed a
confirmation, that it is fine to bring it in as it is from\r\nstateful
cc @vinaychandrasekhar\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic
Machine
<[email protected]>","sha":"0e736e31176fafa0059773d351782cb0707db426"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/193510","number":193510,"mergeCommit":{"message":"Implement
new o11y nav hierarchy for serverless (#193510)\n\nFixes
https://github.com/elastic/kibana/issues/192804\r\n\r\nThis PR updates
the nav hierarchy on serverless and changes the\r\n`accordion` to
`panelOpener`. The menu items have been updated according\r\nto the
[Figma\r\nfile](https://www.figma.com/design/IAR7FjBaSCDWypNYL83fzy/Observability-Navigation?node-id=1232-10087&node-type=frame&t=AMlUqaK2UhhiyqGi-0).\r\nHere's
a Video of how nav hierarchy looks like on serverless with the\r\nnew
changes:\r\n\r\n\r\nhttps://github.com/user-attachments/assets/55d04969-379e-4cd1-8e25-d50382cf51e0\r\n\r\n##
What was changed\r\n\r\n- AI & ML menu is removed and split into\r\n -
`AI Assistant`\r\n - `Machine learning`\r\n- `Applications` now opens a
`panelOpener` instead of an `accordion`\r\n- `Service Inventory` was
renamed to `Service inventory` to meet the use\r\nof sentence-case
requirement\r\n - Synthetics was moved to a new section\r\n-
`Infrastructure`\r\n- `Infrastructure Inventory` was renamed to
`Infrastructure inventory`\r\nto match the sentence-case
requirement\r\n- `Machine learning`: this menu was not present at all on
serverless \r\n\r\n## Notes for the Reviewer\r\n\r\n- `Stack Management`
on security and search don't use any `panelOpener`,\r\nthey use a
landing page instead. In order to be consistent with the
rest\r\nsolutions, I kept Stack Management as is.\r\n- `Machine
Learning` menu item was not present at all on serverless. I\r\nneed a
confirmation, that it is fine to bring it in as it is from\r\nstateful
cc @vinaychandrasekhar\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic
Machine
<[email protected]>","sha":"0e736e31176fafa0059773d351782cb0707db426"}}]}]
BACKPORT-->

Co-authored-by: Panagiota Mitsopoulou <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) ci:build-serverless-image ci:project-deploy-observability Create an Observability project release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-management Observability Management User Experience Team v8.17.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Observability serverless nav] Change nav hierarchy using panelOpener
9 participants