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

Remember tab choice between logs explorer and discover #194930

Merged
merged 18 commits into from
Oct 14, 2024
Merged

Remember tab choice between logs explorer and discover #194930

merged 18 commits into from
Oct 14, 2024

Conversation

awahab07
Copy link
Contributor

@awahab07 awahab07 commented Oct 4, 2024 •
edited by kibanamachine
Loading

Closes #193321

Summary

The PR adds the redirection point when "Discover" menu item is clicked on the sidenav in serverless (or solution nav on stateful). Based on what tab between "Discover" or "Logs Explorer" the user clicked recently, "Discover" will point to that app/tab. Previously, "Discover" would always point to "Logs Explorer" on serverless and to "Discover" on stateful.

In order to implement this, a temporary app last-used-logs-viewer is registered in observability-logs-explorer plugin whose only job is to read the last stored value in local storage and perform the redirection.

Doing the redirection from a temporary app should help prevent triggering unnecessary telemetry and history entries. And it should be fairly easy to undo once context aware redirection is in place.

With this implementation, only the behavior of user clicking "Discover" on the sidenav and clicking the tabs is affected and any deeplinks from other apps or direct links should work as is. The tab choice will be updated even if the apps are visited via url.

193321-Remember-tab-choice-between-Logs-Explorer-and-Discover.mov

@awahab07 awahab07 requested review from a team as code owners October 4, 2024 11:19
@botelastic botelastic bot added the ci:project-deploy-observability Create an Observability project label Oct 4, 2024
@awahab07 awahab07 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-logs Observability Logs User Experience Team labels Oct 4, 2024
@elasticmachine
Copy link
Contributor

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

@awahab07 awahab07 requested a review from a team as a code owner October 4, 2024 14:47
@awahab07 awahab07 requested a review from a team as a code owner October 4, 2024 17:20
@kibana-ci
Copy link
Collaborator

kibana-ci commented Oct 7, 2024 •
edited
Loading

💛 Build succeeded, but was flaky

  • Buildkite Build
  • Commit: 8b6a774
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-194930-8b6a774b0fa6

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
observabilityLogsExplorer 203 207 +4

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
@kbn/deeplinks-observability 51 53 +2

Async chunks

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

id before after diff
discover 820.0KB 822.2KB +2.2KB
observabilityLogsExplorer 143.9KB 147.3KB +3.4KB
total +5.6KB

Page load bundle

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

id before after diff
discover 50.6KB 50.6KB +47.0B
observabilityLogsExplorer 14.7KB 15.0KB +324.0B
serverlessObservability 27.2KB 27.2KB -6.0B
total +365.0B
Unknown metric groups

API count

id before after diff
@kbn/deeplinks-observability 63 65 +2

async chunk count

id before after diff
observabilityLogsExplorer 5 6 +1

History

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

@tonyghiani tonyghiani self-requested a review October 7, 2024 08:26
@tonyghiani
Copy link
Contributor

@awahab07 It Looks like something is not working when storing the last visited page and trying to restore it, looks at this recording and let me know if you have any questions. This is a local stateful environment with project view enabled:

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

afharo
afharo approved these changes Oct 8, 2024
View reviewed changes
Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

Interesting that it triggered an application usage change... but telemetry changes LGTM

@botelastic botelastic bot added the Team:obs-ux-management Observability Management User Experience Team label Oct 8, 2024
@elasticmachine
Copy link
Contributor

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

@tonyghiani
Copy link
Contributor

I found a couple of scenarios where it seems to not work at the moment.

Restore not working when updating the page URL

When using any link from the history, the stored value in the localStorage is probably not updated, as when leaving the page and returning to it, we are no longer on the previously visited tab.

Screen.Recording.2024-10-09.at.12.06.34.mov

Active navigation item

The navigation item in the sidebar doesn't maintain the active status when the user switches to Discover.
We probably need to use somehow the getIsActive option in the navigation_tree.ts file definition for this entry.

Screen.Recording.2024-10-09.at.11.59.56.mov

@awahab07
Copy link
Contributor Author

awahab07 commented Oct 9, 2024

@tonyghiani thanks for catching:

Active navigation item
The navigation item in the sidebar doesn't maintain the active status when the user switches to Discover.

I've pushed a fix.

For

Restore not working when updating the page URL

It asks for a decision that how we want implement it. Currently the tab choice is only persisted when user explicitly clicks on the tab and not when the url is visited. Accounting the fact that direct links to Discover or Logs Explorer from other apps or pasted urls shouldn't affect this choice. For example, a user may want to always use Discover when simply exploring the logs, and links to Logs Explorer from other apps should not override this choice. WDYT? Open for suggestions if there are other considerations I am missing.

@tonyghiani
Copy link
Contributor

@awahab07 I see your point, on the other hand, it feels like an inconsistent experience that the user doesn't always get back to the last tab during a session if they perform back and forth from discover, Logs Explorer and other apps... We might be complicating this more than it's worth to, as it should soon be removed, although it still feels a bit confusing.

@awahab07
Persist Discover/Observability Logs Explorer tab selection based on l…
65671c9 
…ast visited status instead of last tab clicked status.
@awahab07
Merge remote-tracking branch 'origin/main' into 193321-Remember-tab-c…
ec39158 
…hoice-between-Logs-Explorer-and-Discover
tonyghiani
tonyghiani approved these changes Oct 10, 2024
View reviewed changes
Copy link
Contributor

@tonyghiani tonyghiani left a comment

Choose a reason for hiding this comment

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

It now works as expected, thanks for addressing all the edge cases.
Let's keep in mind this is a temporary solution and should be reverted/removed as soon as we remove the Logs Explorer tabs 👌

mgiota
mgiota approved these changes Oct 10, 2024
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.

I tested the navigation tree changes on stateful solution nav and works as expected. I didn't test on serverless, did code review only. LGTM!

Screen.Recording.2024-10-10.at.13.20.15.mov

@kertal kertal self-requested a review October 11, 2024 08:06
kertal
kertal approved these changes Oct 11, 2024
View reviewed changes
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

LGTM, codeowner review. tested it locally and works as expected

@elasticmachine
Copy link
Contributor

elasticmachine commented Oct 14, 2024 •
edited
Loading

💛 Build succeeded, but was flaky

  • Buildkite Build
  • Commit: f223dc6
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-194930-f223dc658ef5

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #3 / DataTableColumnHeader should render a correct token

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
observabilityLogsExplorer 204 208 +4

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
@kbn/deeplinks-observability 51 53 +2

Async chunks

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

id before after diff
discover 821.3KB 823.6KB +2.2KB
observability 467.5KB 467.6KB +159.0B
observabilityLogsExplorer 143.9KB 147.3KB +3.4KB
total +5.8KB

Page load bundle

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

id before after diff
discover 50.6KB 50.6KB +47.0B
observabilityLogsExplorer 14.7KB 15.0KB +324.0B
serverlessObservability 27.2KB 27.2KB -6.0B
total +365.0B
Unknown metric groups

API count

id before after diff
@kbn/deeplinks-observability 63 65 +2

async chunk count

id before after diff
observabilityLogsExplorer 5 6 +1

History

@awahab07 awahab07 merged commit fed9a19 into elastic:main Oct 14, 2024
25 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

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

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 14, 2024
@awahab07
Remember tab choice between logs explorer and discover (elastic#194930)
dc5eeb3 
Closes elastic#193321

## Summary

The PR adds the redirection point when "Discover" menu item is clicked
on the sidenav in serverless (or solution nav on stateful). Based on
what tab between "Discover" or "Logs Explorer" the user clicked
recently, "Discover" will point to that app/tab. Previously, "Discover"
would always point to "Logs Explorer" on serverless and to "Discover" on
stateful.

In order to implement this, a temporary app `last-used-logs-viewer` is
registered in `observability-logs-explorer` plugin whose only job is to
read the last stored value in local storage and perform the redirection.

Doing the redirection from a temporary app should help prevent
triggering unnecessary telemetry and history entries. And it should be
fairly easy to undo once context aware redirection is in place.

~With this implementation, only the behavior of user clicking "Discover"
on the sidenav and clicking the tabs is affected and any deeplinks from
other apps or direct links should work as is.~ The tab choice will be
updated even if the apps are visited via url.

https://github.com/user-attachments/assets/8a0308db-9ddb-47b6-b1a5-8ed70662040d

---------

Co-authored-by: kibanamachine <[email protected]>
(cherry picked from commit fed9a19)
@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

@kibanamachine kibanamachine mentioned this pull request Oct 14, 2024
Merged
kibanamachine added a commit that referenced this pull request Oct 14, 2024
@kibanamachine @awahab07
[8.x] Remember tab choice between logs explorer and discover (#194930) (
ad83fc7 
#196068)

# Backport

This will backport the following commits from `main` to `8.x`:
- [Remember tab choice between logs explorer and discover
(#194930)](#194930)

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

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

<!--BACKPORT [{"author":{"name":"Abdul Wahab
Zahid","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-14T09:26:24Z","message":"Remember
tab choice between logs explorer and discover (#194930)\n\nCloses
#193321 \r\n\r\n## Summary\r\n\r\nThe PR adds the redirection point when
\"Discover\" menu item is clicked\r\non the sidenav in serverless (or
solution nav on stateful). Based on\r\nwhat tab between \"Discover\" or
\"Logs Explorer\" the user clicked\r\nrecently, \"Discover\" will point
to that app/tab. Previously, \"Discover\"\r\nwould always point to
\"Logs Explorer\" on serverless and to \"Discover\"
on\r\nstateful.\r\n\r\nIn order to implement this, a temporary app
`last-used-logs-viewer` is\r\nregistered in
`observability-logs-explorer` plugin whose only job is to\r\nread the
last stored value in local storage and perform the
redirection.\r\n\r\nDoing the redirection from a temporary app should
help prevent\r\ntriggering unnecessary telemetry and history entries.
And it should be\r\nfairly easy to undo once context aware redirection
is in place.\r\n\r\n~With this implementation, only the behavior of user
clicking \"Discover\"\r\non the sidenav and clicking the tabs is
affected and any deeplinks from\r\nother apps or direct links should
work as is.~ The tab choice will be\r\nupdated even if the apps are
visited via
url.\r\n\r\n\r\n\r\nhttps://github.com/user-attachments/assets/8a0308db-9ddb-47b6-b1a5-8ed70662040d\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"fed9a193869739c2fae0bd7a49087fa1e216f5ac","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","backport:prev-minor","ci:project-deploy-observability","Team:obs-ux-logs","Team:obs-ux-management"],"title":"Remember
tab choice between logs explorer and
discover","number":194930,"url":"https://github.com/elastic/kibana/pull/194930","mergeCommit":{"message":"Remember
tab choice between logs explorer and discover (#194930)\n\nCloses
#193321 \r\n\r\n## Summary\r\n\r\nThe PR adds the redirection point when
\"Discover\" menu item is clicked\r\non the sidenav in serverless (or
solution nav on stateful). Based on\r\nwhat tab between \"Discover\" or
\"Logs Explorer\" the user clicked\r\nrecently, \"Discover\" will point
to that app/tab. Previously, \"Discover\"\r\nwould always point to
\"Logs Explorer\" on serverless and to \"Discover\"
on\r\nstateful.\r\n\r\nIn order to implement this, a temporary app
`last-used-logs-viewer` is\r\nregistered in
`observability-logs-explorer` plugin whose only job is to\r\nread the
last stored value in local storage and perform the
redirection.\r\n\r\nDoing the redirection from a temporary app should
help prevent\r\ntriggering unnecessary telemetry and history entries.
And it should be\r\nfairly easy to undo once context aware redirection
is in place.\r\n\r\n~With this implementation, only the behavior of user
clicking \"Discover\"\r\non the sidenav and clicking the tabs is
affected and any deeplinks from\r\nother apps or direct links should
work as is.~ The tab choice will be\r\nupdated even if the apps are
visited via
url.\r\n\r\n\r\n\r\nhttps://github.com/user-attachments/assets/8a0308db-9ddb-47b6-b1a5-8ed70662040d\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"fed9a193869739c2fae0bd7a49087fa1e216f5ac"}},"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/194930","number":194930,"mergeCommit":{"message":"Remember
tab choice between logs explorer and discover (#194930)\n\nCloses
#193321 \r\n\r\n## Summary\r\n\r\nThe PR adds the redirection point when
\"Discover\" menu item is clicked\r\non the sidenav in serverless (or
solution nav on stateful). Based on\r\nwhat tab between \"Discover\" or
\"Logs Explorer\" the user clicked\r\nrecently, \"Discover\" will point
to that app/tab. Previously, \"Discover\"\r\nwould always point to
\"Logs Explorer\" on serverless and to \"Discover\"
on\r\nstateful.\r\n\r\nIn order to implement this, a temporary app
`last-used-logs-viewer` is\r\nregistered in
`observability-logs-explorer` plugin whose only job is to\r\nread the
last stored value in local storage and perform the
redirection.\r\n\r\nDoing the redirection from a temporary app should
help prevent\r\ntriggering unnecessary telemetry and history entries.
And it should be\r\nfairly easy to undo once context aware redirection
is in place.\r\n\r\n~With this implementation, only the behavior of user
clicking \"Discover\"\r\non the sidenav and clicking the tabs is
affected and any deeplinks from\r\nother apps or direct links should
work as is.~ The tab choice will be\r\nupdated even if the apps are
visited via
url.\r\n\r\n\r\n\r\nhttps://github.com/user-attachments/assets/8a0308db-9ddb-47b6-b1a5-8ed70662040d\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"fed9a193869739c2fae0bd7a49087fa1e216f5ac"}}]}]
BACKPORT-->

Co-authored-by: Abdul Wahab Zahid <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davismcphee davismcphee davismcphee left review comments

@tonyghiani tonyghiani tonyghiani approved these changes

@afharo afharo afharo approved these changes

@mgiota mgiota mgiota approved these changes

@kertal kertal kertal approved these changes

Assignees
No one assigned
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) ci:project-deploy-observability Create an Observability project release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-logs Observability Logs User Experience Team Team:obs-ux-management Observability Management User Experience Team v8.16.0 v9.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Logs] Remember tab choice between Logs Explorer and Discover
9 participants
@awahab07 @elasticmachine @kibana-ci @tonyghiani @kibanamachine @kertal @mgiota @afharo @davismcphee