[Discover] [Log Explorer] Add tabs to Discover and Log Explorer

[davismcphee]

Summary

This PR adds tabs to navigate between Discover and Log Explorer in serverless O11y projects. The Discover top nav should remain unchanged in stateful deployments and all other serverless project types:

Important

While writing tests for this, I encountered a few issues we'll probably want to discuss and make decisions for before merging this PR.

1. When there are no data views in Kibana and a user navigates to Discover, they are prevented from accessing it and shown a no data screen. So if a user navigates to Discover in a serverless O11y project in order to get to Log Explorer but there are no existing data views, they will be unable to access the tabs since they will be blocked by the no data screen. Is this a realistic scenario in serverless O11y that we need to solve for, or will there always be at least one data view by default?
2. When navigating from the Log Explorer tab to the Discover tab, we convert the current dataset to an ad hoc data view to use in Discover. This doesn't work in reverse since Log Explorer can't load an arbitrary data view that might be selected in Discover, so instead I'm using the "all logs" locator. I'll leave it to the O11y team to decide if this is ok or if we need to take another approach here.
3. Since we are passing state between Discover and Log Explorer when a user switches between tabs, the default columns in Log Explorer will be overwritten by the current Discover grid columns (even if it's just the Document column). I imagine the O11y team doesn't want this, but how should we solve it? Should we avoid passing columns (and any other state that might overwrite defaults) when navigating from Discover to Log Explorer, or take another approach?

Notes

• The sidebar navigation link has been changed from "Log Explorer" to "Discover" and defaults to the Discover tab.
• "Log Explorer" has been renamed to "Logs Explorer" in the UI.
• The mockups used "Data Views" for the Discover tab title, but this was later decided against, so the implementation uses "Discover".
• All of the same state that used to be passed from Log Explorer to Discover through the top nav button is still being passed, but I also added support for passing sort as well (I can revert this if it's unwanted).
• In order to add tabs to the left side of the Discover top nav in serverless, we had to stop relying on Unified Search for rendering the top nav (in serverless only, stateful still uses it). Instead we are now rendering the top nav directly in Discover in serverless, similar to what Log Explorer does. This also required access to some of the internal navigation plugin components we rely on for the Discover top nav, so I exported them from the plugin since I figured it was better than duplicating them.

Part of #169964. In order to fully resolve this issue, there is still a remaining task to remove the "Data Views" tab from the Log Explorer dataset selector.

Part of #171386. In order to fully resolve this issue, it looks like there may be some documentation work to do (especially since "Log Explorer" has been named to "Logs Explorer" in the UI).

Checklist

• Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• Documentation was added for features that require explanation or tutorials
• Unit or functional tests were updated or added to match the most common scenarios
• Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
• Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
• If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
• This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
• This was checked for cross-browser compatibility

For maintainers

• This was checked for breaking API changes and was labeled appropriately

[elasticmachine]

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

[ruflin]

1. There are no data views by default. The problem is less that Logs Explorer can't be access, but the screen that is shown if there are no data views. If an o11y does not have data views, the most likely case is they have no data and we should navigate them to add data. Also in Logs Explorer, this empty screen is not great yet. There we can fix it on our own, with Discover we need support to get this changed. I would not make this a blocker for this PR.
2. This is ok for now. I expect we can iterate on this later on.
3. Logs explorer is opinionated about the columns. I rather have not passed on this part of the state as users navigate to the Logs Explorer to have a logs specific view. Please also don't pass on sort. @weltenwort

The other pending issues you can leave to us as this is all inside Logs Explorer.

[weltenwort]

@davismcphee thanks for calling out these issues

• re 1: Would it be very difficult to render the tabs even on the "no data" screens?
• re 2 and 3: I'd be in favor of not passing state between the two tabs at this point. I'm decoupling the log explorer state from the URL and any global singletons in [Logs+] Refactor state and URL persistence of Log Explorer #170200 at the moment. I'd rather introduce explicit state passing in the future as we do in the "Open in Discover" link right now.

[Dosant]

top nav changes lgmt!

[davismcphee]

There are no data views by default. The problem is less that Logs Explorer can't be access, but the screen that is shown if there are no data views. If an o11y does not have data views, the most likely case is they have no data and we should navigate them to add data. Also in Logs Explorer, this empty screen is not great yet. There we can fix it on our own, with Discover we need support to get this changed. I would not make this a blocker for this PR.

Ok sounds good, I will leave it as is for now then and we can have followup discussions about what to do with the no data screen.

3. Logs explorer is opinionated about the columns. I rather have not passed on this part of the state as users navigate to the Logs Explorer to have a logs specific view. Please also don't pass on sort.

Today when the "Open in Discover" link is clicked, we transfer state from Log Explorer -> Discover, so I haven't changed anything there apart from the Discover link being switched to a Discover tab (except adding sort, which should be passed too if we're passing columns, otherwise we shouldn't pass either). So my question is mainly about if we should be doing the same thing in the opposite direction, i.e. Discover -> Log Explorer.

Is your opinion that we should continue passing state from Log Explorer -> Discover as we do today, and that we should pass some state from Discover -> Log Explorer (excluding state like columns and sort that have defaults in Log Explorer)? Or do you think we should remove the state passing between tabs entirely and just completely clear a user's state on them when they switch tabs?

re 1: Would it be very difficult to render the tabs even on the "no data" screens?

I believe so. I can look into this a bit today, but currently all of Discover is blocked from rendering when there's no data, so all of its internal logic assumes there's data available. It could be tricky to refactor in a way that allows the top nav to render when the no data screen is shown, but I can at least have a look.

re 2 and 3: I'd be in favor of not passing state between the two tabs at this point. I'm decoupling the log explorer state from the URL and any global singletons in #170200 at the moment. I'd rather introduce explicit state passing in the future as we do in the "Open in Discover" link right now.

I'm not sure I understand this point. The logic I'm currently using is the explicit state passing from "Open in Discover" that exists today. Are you saying we should drop the existing functionality and just always reset the state to the defaults when navigating between tabs instead? We can do this, but I worry it would unfriendly and unexpected to users to lose all state just by switching tabs (but I guess at least a clever user who accidentally cleared their changes by switching tabs could use the browser back button to restore their old state).

[davismcphee]

@weltenwort So I looked into it today, and while it still required quite a bit of refactoring, it wasn't as bad as I thought it would be to get the tabs and no data screen showing in serverless. Rather than pulling the no data screen deeper into Discover like I thought we'd need to, I was instead able to pull the serverless top nav up to where the no data screen is rendered without too much difficulty. It probably still has some bugs currently, and I definitely broke some tests, but I pushed what I have so far in e1d6a3f and it seems to be working well for the most part. I'll follow up after the weekend to address any bugs it created and fix up the tests.

[weltenwort]

The logic I'm currently using is the explicit state passing from "Open in Discover" that exists today.

That's great! This is what I meant. Sorry for causing confusion here, apparently I didn't read the PR carefully enough.

I was instead able to pull the serverless top nav up to where the no data screen is rendered without too much difficulty.

🎉 neat, thank you

[davismcphee]

@weltenwort no worries, sounds like we're on the same page! Also I fixed the broken tests from the no data change and things should be back in a reviewable state now.

[andreadelrio]

I think after introducing tabs, passing the state between Logs Explorer tab and Discover tab might not be what user would expect.

Tabs as an UI element usually mean that it's possible to switch from one to another and get back to its previous content without losing the tab's state. With this PR, the state of tab 1 would be replaced with some reduced equivalent from tab 2 state.

@davismcphee Sorry for the delayed comment here, we were on the UX offsite. From a UX perspective, I want to +1 on Julia's comment above. So not passing state and perhaps keeping the open in Discover link to intentionally share state (like Nicolas mentioned above) sounds good to me, at least for this first version.

[ryankeairns]

+1 @andreadelrio (et al)

I suspect (hope?) we move toward tabs becoming a user-defined concept that aligns to a (singular) saved query/search... providing a 1:1 between a tab and query/search. If we go that route, then state transfer would not be expected.

In that world, we may also be able to split experiences along 'modes' and have each tab set to a mode (e.g. ES|QL, data views, Logs exp/data sets, etc.).

[davismcphee]

Thanks all for the feedback! It seems like everyone is aligned with removing the current state syncing, so I've updated the PR to not sync any state between tabs and always navigate to the application root URLs. I've also re-added the "Open in Discover" button, although I'm not sure what that should look like design wise, so I'm open to any suggestions:

[andreadelrio]

Thanks all for the feedback! It seems like everyone is aligned with removing the current state syncing, so I've updated the PR to not sync any state between tabs and always navigate to the application root URLs. I've also re-added the "Open in Discover" button, although I'm not sure what that should look like design wise, so I'm open to any suggestions:

I would align it to the right. Do we still use the Discover icon anywhere? maybe we are good to remove it.

[davismcphee]

I would align it to the right. Do we still use the Discover icon anywhere? maybe we are good to remove it.

@andreadelrio Works for me! And we do a few places (e.g. Add Sample Data screen, opening an index in Discover from Index Management, etc.), but not a whole lot that I've noticed. How does this look?

[jughosta]

Code changes LGTM 👍

[weltenwort]

The changes to the Logs Explorer LGTM, thanks for taking this on ❤️ I'll reserve any judgement on the Discover changes 😉

I noticed some state restoration weirdness when clicking the tabs, but I'm pretty sure that's on us and our locator. Since #170200 will change the internals of the locator anyway, I don't think this is anything we'll have to resolve here.

[andreadelrio]

Design changes LGTM, thanks!

[maryam-saeidi]

Obs ux management changes LGTM! (code review only)

[kibana-ci]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: dbb12eb
• Storybooks Preview
• Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-171467-dbb12eb9ea8a
• Observability Deployment

Failed CI Steps

• Explore - Security Solution Cypress Tests #4

Test Failures

• [job] [logs] Explore - Security Solution Cypress Tests #4 / url state sets and reads the url state for timeline by id sets and reads the url state for timeline by id

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
discover	707	717	+10
navigation	32	34	+2
total			+12

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
discover	91	96	+5
navigation	35	42	+7
total			+12

Async chunks

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

id	before	after	diff
discover	589.8KB	593.4KB	+3.6KB

Canvas Sharable Runtime

The Canvas "shareable runtime" is an bundle produced to enable running Canvas workpads outside of Kibana. This bundle is included in third-party webpages that embed canvas and therefor should be as slim as possible.

id	before	after	diff
module count	-	5713	+5713
total size	-	5.9MB	+5.9MB

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
discover	20	21	+1

Page load bundle

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

id	before	after	diff
discover	31.2KB	31.7KB	+552.0B
navigation	11.8KB	12.0KB	+194.0B
observabilityLogExplorer	32.5KB	32.9KB	+401.0B
serverlessObservability	65.0KB	65.0KB	-7.0B
total			+1.1KB

Unknown metric groups

API count

id	before	after	diff
discover	134	141	+7
navigation	35	42	+7
total			+14

async chunk count

id	before	after	diff
discover	19	20	+1

ESLint disabled line counts

id	before	after	diff
discover	22	23	+1

Total ESLint disabled count

id	before	after	diff
discover	22	23	+1

History

• 💔 Build #182601 failed 3843a4c
• 💔 Build #182290 failed 457641a
• 💚 Build #182276 succeeded 2e4b3cd
• 💚 Build #181794 succeeded 9e64703
• 💔 Build #181621 failed ae05ec7

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

cc @davismcphee

[sebelga]

Commenting late on this PR as I am working on part of the code that touches the changes here.
I see that the discover plugin is aware of the serverless world (it depends on the serverless plugin..). Ideally this would have been avoided and it is the serverless plugins that call extension points in Discover to extend its behaviour.

It is much harder for me now to bring the changes brought in this PR in a non serverless environment (stateful Kibana).

Not sure what can be done at this stage but wanted to leave the comment to limit future changes in that direction (and maybe even refactor at some point).

[davismcphee]

@sebelga Apologies for such a delay following up on this. Just wanted to give you a heads up that we're currently doing more work related to this in Discover, and I just merged a PR which gets rid of the serverless plugin dependency and instead replaces it with a showInlineTopNav method on the Discover start contract which gets called from serverless plugins: #178165. Thanks for the input!

[sebelga]

Awesome @davismcphee ! Thanks for making those changes 🎉