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

[Serverless Elasticsearch] Fix user is blocked from moving forward when opening Discover, Dashboard, or Visualize Library #164709

Merged
merged 17 commits into from
Aug 28, 2023

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Aug 24, 2023

Summary

fix #164432

AnalyticsNoDataPage could only point to "Integrations" (fleet), but serverless search disabled fleet plugin. The AnalyticsNoDataPage started showing "no access to fleet" state. This PR fixes it and shows empty state that makes sense for serverless search project:

Screenshot 2023-08-24 at 15 03 45

In this PR I am hacking together a way to override the content of the AnalyticsNoDataPage:

  • AnalyticsNoDataPage could only point to "Integrations". Some integrations-specific URLs and access checks are hardcoded deep inside the component. If the fleet plugin was not available, it just showed that the user didn't have access.
  • AnalyticsNoDataPage introduces "flavor" ('kibana' | 'serverless_search'). Depending on flavor it passes down different texts for the page and the card. There is also a canAccessFleet hack that forces it to "true" for the serverless_search flavor. This is needed to skip the fleet plugin access from inside the component and to avoid showing a "no permissions state". This needs to be refactored: probably, the inner card shouldn't be so tightly coupled to fleet
  • The no_data_page plugin is added to register the new config no_data_page.analyticsNoDataPageFlavor: 'serverless_search'. Analytics apps will pass the plugin down to the AnalyticsNoDataPage so it can pick the flavor. The introduced plugin is the downside of having such a complex page as a stateless component. An Alternative could be if discover, visualize, dashboard would register their configs to pass down to AnalyticsNoDataPage.

@@ -0,0 +1,3 @@
# No Data Page

Helps to globally configure the no data page components
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need this dummy plugin to create a config for configuring the analytics empty pages "flavor"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An alternative could be that all discover, visualize, dashboards expose this config on their own and pass it to the component.

@@ -33,13 +33,13 @@ export const NoDataPage = ({
values: { solution },
});

const link = (
const link = docsLink ? (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making docsLink here optional to skip the docs link for serverless search empty page for now

'data-test-subj': 'kbnOverviewElasticsearchGettingStarted',
href: prependBasePath('/app/elasticsearch/'),
/** force the no data card to be shown **/
canAccessFleet: true,
Copy link
Contributor Author

@Dosant Dosant Aug 24, 2023

Choose a reason for hiding this comment

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

Passing/faking this explicitly so that NoDataCard doesn't fallback to the "no access to fleet" page. This needs to be refactored.

Currently, it is ugly: even though the NoDataCard seems to be generic and text and links can be configured through props, some "fleet" logic parts are still hardcoded inside the component. For example, the component checks the access to fleet through context.

'Set up your programming language client, ingest some data, and start searching.',
}),
'data-test-subj': 'kbnOverviewElasticsearchGettingStarted',
href: prependBasePath('/app/elasticsearch/'),
Copy link
Contributor Author

@Dosant Dosant Aug 24, 2023

Choose a reason for hiding this comment

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

href on this level is added only for "serverless_search" flavor, but not for "kibana" flavor because the integrations page is already hardcoded inside the component. The NoDataCard must be refactored to be fully configurable and don't hardcode fleet stuff.

@Dosant Dosant changed the title wip [Serverless Elasticsearch] Fix user is blocked from moving forward when opening Discover, Dashboard, or Visualize Library [Serverless Elasticsearch] Fix user is blocked from moving forward when opening Discover, Dashboard, or Visualize Library Aug 25, 2023
@Dosant Dosant added release_note:skip Skip the PR/issue when compiling release notes Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) Project:Serverless Work as part of the Serverless project for its initial release labels Aug 25, 2023
@Dosant Dosant self-assigned this Aug 25, 2023
@@ -109,7 +105,7 @@ export function DiscoverMainRoute({
const hasUserDataViewValue = await data.dataViews.hasData
.hasUserDataView()
.catch(() => false);
const hasESDataValue = isDev || (await data.dataViews.hasData.hasESData().catch(() => false));
const hasESDataValue = await data.dataViews.hasData.hasESData().catch(() => false);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elastic/kibana-data-discovery, hope you don't mind. Because of isDev check I couldn't add a functional test for this state that would pass locally. This also made it harder to discover the bug

Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine, its was introduced to resolve #133273
Implemented in #133461

But I now tested what was mentioned not to work in #133273, so I think it has been resolved differently in the meantime?

It seems that isDev is now deprecated, which means we should cleanup in a follow up

@Dosant
Copy link
Contributor Author

Dosant commented Aug 25, 2023

@elasticmachine merge upstream

@Dosant Dosant marked this pull request as ready for review August 25, 2023 14:51
@Dosant Dosant requested review from a team as code owners August 25, 2023 14:51
@Dosant Dosant requested review from a team as code owners August 25, 2023 14:51
@elasticmachine
Copy link
Contributor

Pinging @elastic/appex-sharedux (Team:SharedUX)

@Dosant Dosant requested a review from sphilipse August 25, 2023 14:52
Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

New Dashboard service LGTM! Code only review. Great to see this addressed.

@kc13greiner kc13greiner self-requested a review August 25, 2023 15:26
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.

Changes to config/serverless.es.yml LGTM

Copy link
Contributor

@kc13greiner kc13greiner left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Visualize listing page LGTM!

Copy link
Contributor

@sebelga sebelga left a comment

Choose a reason for hiding this comment

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

LGTM! Tested locally and works as expected.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
dashboard 388 389 +1
noDataPage - 4 +4
total +5

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
noDataPage - 3 +3

Async chunks

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

id before after diff
canvas 1.0MB 1.0MB +188.0B
dashboard 368.6KB 370.0KB +1.4KB
discover 562.7KB 564.0KB +1.2KB
enterpriseSearch 2.6MB 2.6MB +188.0B
eventAnnotation 176.7KB 176.9KB +188.0B
filesManagement 89.7KB 89.9KB +188.0B
graph 407.8KB 408.0KB +188.0B
home 163.9KB 164.1KB +188.0B
infra 2.0MB 2.0MB +188.0B
kibanaOverview 43.9KB 45.1KB +1.2KB
lens 1.4MB 1.4MB +1.2KB
management 42.5KB 42.6KB +188.0B
maps 2.8MB 2.8MB +188.0B
ml 3.5MB 3.5MB +188.0B
observabilityShared 35.9KB 36.1KB +188.0B
security 567.8KB 568.0KB +188.0B
securitySolution 16.0MB 16.0MB +376.0B
securitySolutionEss 42.4KB 42.6KB +188.0B
securitySolutionServerless 266.2KB 266.4KB +188.0B
spaces 173.8KB 174.0KB +188.0B
visualizations 263.5KB 264.8KB +1.3KB
total +9.5KB

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.5KB 31.5KB +24.0B
esUiShared 156.3KB 156.5KB +188.0B
noDataPage - 1.4KB +1.4KB
visualizations 56.8KB 56.9KB +24.0B
total +1.7KB
Unknown metric groups

API count

id before after diff
noDataPage - 3 +3

History

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

cc @Dosant

@kertal kertal self-requested a review August 28, 2023 14:10
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.

Thx for fixing this 👍 Tested and works as expected now 👍

@sebelga
Copy link
Contributor

sebelga commented Aug 28, 2023

Thanks for the review @afharo @kc13greiner @stratoula @kertal @ThomThomson @jbudz !

@sphilipse if you have any feedback we can address it in a separate PR 👍

@sebelga sebelga merged commit 243142d into elastic:main Aug 28, 2023
@kibanamachine kibanamachine added v8.11.0 backport:skip This commit does not require backporting labels Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Project:Serverless Work as part of the Serverless project for its initial release release_note:skip Skip the PR/issue when compiling release notes Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.11.0
Projects
None yet