-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[ES|QL] Bypass no data views screen #174316
Conversation
@clintandrewhall: Regarding design support, I recall @andreadelrio sharing some design concepts for this a week or so ago, so I have a feeling she has something in mind for how to present these ES|QL links. I believe she's on PTO today, but will be back next week. @andreadelrio: Hopefully I'm remembering correctly, but let me know if that's not the case, and I can happily lend some design assistance here as needed. |
6d48c06
to
b75c9ee
Compare
@rshen91 @stratoula @andreadelrio I think we should ship the locator and related logic in this PR and hold off on the UX, rather than continue to rebase this until the UX is decided. We can iterate in code on the different proposals using Storybook, and ship the changes with only Shared UX and design really being involved, (from a PR approval perspective, that is). Thoughts? |
@clintandrewhall makes sense and I am fine with that. This means that is going to look like this right? |
@clintandrewhall fixed the logic here 2a9edec You can remove the dataviewSpec, it works without it 🎸 |
Actually, I'm suggesting we don't ship and UI change at all in this PR, just provide the locator and related logic. We'll leave any UX implementation to the next PR. |
@clintandrewhall got it! I am fine with this approach too 👍 |
I like that @nreese ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one minor point below. Also, would be nice if someone else on the team can approve the state changes (resetStateContainer
), but I personally don't see an issue.
const { discoverAppLocator, getIndices } = this.deps; | ||
|
||
const indexName = await getIndexForESQLQuery({ dataViews: { getIndices } }); | ||
const esql = `from ${indexName ?? '*'} | limit 10`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this should be fixed in this PR, but the indexName
can contain special characters (like |
). Is there some sanitization that we can add here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really? I get this error when I try to create one:
Invalid index name [my|index|1], must not contain the following characters ['\',' ','"','<','*','?','>','|',',','/']
/ci |
@Dosant amazing job! You are super fast! This is a very important improvement imho. Being able to query your data or even create visualizations with a dataview is huge! I am ready to approve, just waiting for the presentation team to chime in.
This would be a very good idea. We will need ui actions here I think, it is not possible from a locator (or is it and I am missing something?) I was hoping that the @elastic/kibana-presentation team can take the dashboard behavior as a follow up. I was thinking that we can navigate to a dashboard as Nathan says (and having the ES|QL panel open would be also fantastic) and then have only active the panels that do not require a dataview. These must be ES|QL, text, image, vega and maps. Now every time you click to Lens you get this no data view that I dont necessarily like. Lens should be inactive, works only with dataviews for now etc. But I think this is a bigger project and out of the scope of this PR. We can discuss further as an analyst XP team! I think for now as a first iteration and as this is something that multiple people have complained about it makes sense. It navigates to Discover which is the best application we have to learn ES|QL. So it is ok, can this be better for dashboards? Absolutely!! With that being said, I am fine with that if the rest of the gang is. @Dosant I think we need to prioritize the redesign of this component. There are 2 things I don't like:
Also can we remove the ES|QL button when you specifically are in the dataviews management app? |
@nreese, @ninoslavmiskovic, agree with @stratoula, I am afraid to extend the scope of this slipped pr further with the ff coming up. But the suggestion is totally how I'd like it to work. I'd like to create an issue and will see if this is something shared-ux can also do
I'll create an issue for the @andreadelrio's proposal #174316 (comment). Not sure if I will follow up with it straight away, but will see. As of the docs, |
…t_esql # Conflicts: # packages/kbn-esql-utils/index.ts
…bana into shared_ux/prompt_esql
agree. I have created #176159 to track |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kibana-presentation changes LGTM
code review and tested in chrome
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Visualizations team changes LGTM! Thanx for this PR Anton. I did a brief testing again and everything works as expected. :)
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
History
To update your PR or re-run it, just comment with: |
> Derived from elastic#173068 > Further addresses elastic#169873 ## Summary - We're adding a link to the No Data Prompt to send a person to explore their data using ES|QL if there exists any ingested data but no data view. - This PR populates the query bar with the first available index (some special handling for logs index. elastic@2a9edec) - All consumers of the prompt/no data view are updated in this PR. - [x] ~There's an issue where, if you're in Discover, clicking the link won't refresh the page. .~ This is fixed on Discover side by reinitializing the state container when user clicks the "try es|ql" link and URL state updates. - [x] ~There is an issue that you can save the es|ql chart from Discover, but Dashboard's empty screen blocks the navigation because data views don't exist elastic#174316 (comment). This is fixed by allowing the dashboard to work without the default data view. Hopefully, this won't lead to major issues - [x] ~ES|QL panels can't be created without the default data views~ this is fixed by trying to fallback to an ad-hoc dataview, plan to move that code to the utils introduced here elastic#174736 - [x] fix circular deps - [x] Add functional tests ## Visuals https://github.com/elastic/kibana/assets/7784120/af3592c1-f4c8-43bb-a128-3268b7761367 ### Storybook Stories #### Can access ES|QL ![Screenshot 2024-01-31 at 17 05 47](https://github.com/elastic/kibana/assets/7784120/370d0351-198e-4dc3-b22e-86f497ad4df5) #### Cannot access (e.g. preview is unavailable - _not implemented_) ![Screenshot 2024-01-31 at 17 05 59](https://github.com/elastic/kibana/assets/7784120/c2bf52ab-9fa8-4f25-9e5d-512d4f4342fa) --------- Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co> Co-authored-by: Rachel Shen <rshen@elastic.co> Co-authored-by: Anton Dosov <anton.dosov@elastic.co>
> Derived from elastic#173068 > Further addresses elastic#169873 ## Summary - We're adding a link to the No Data Prompt to send a person to explore their data using ES|QL if there exists any ingested data but no data view. - This PR populates the query bar with the first available index (some special handling for logs index. elastic@2a9edec) - All consumers of the prompt/no data view are updated in this PR. - [x] ~There's an issue where, if you're in Discover, clicking the link won't refresh the page. .~ This is fixed on Discover side by reinitializing the state container when user clicks the "try es|ql" link and URL state updates. - [x] ~There is an issue that you can save the es|ql chart from Discover, but Dashboard's empty screen blocks the navigation because data views don't exist elastic#174316 (comment). This is fixed by allowing the dashboard to work without the default data view. Hopefully, this won't lead to major issues - [x] ~ES|QL panels can't be created without the default data views~ this is fixed by trying to fallback to an ad-hoc dataview, plan to move that code to the utils introduced here elastic#174736 - [x] fix circular deps - [x] Add functional tests ## Visuals https://github.com/elastic/kibana/assets/7784120/af3592c1-f4c8-43bb-a128-3268b7761367 ### Storybook Stories #### Can access ES|QL ![Screenshot 2024-01-31 at 17 05 47](https://github.com/elastic/kibana/assets/7784120/370d0351-198e-4dc3-b22e-86f497ad4df5) #### Cannot access (e.g. preview is unavailable - _not implemented_) ![Screenshot 2024-01-31 at 17 05 59](https://github.com/elastic/kibana/assets/7784120/c2bf52ab-9fa8-4f25-9e5d-512d4f4342fa) --------- Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co> Co-authored-by: Rachel Shen <rshen@elastic.co> Co-authored-by: Anton Dosov <anton.dosov@elastic.co>
> Derived from elastic#173068 > Further addresses elastic#169873 ## Summary - We're adding a link to the No Data Prompt to send a person to explore their data using ES|QL if there exists any ingested data but no data view. - This PR populates the query bar with the first available index (some special handling for logs index. elastic@2a9edec) - All consumers of the prompt/no data view are updated in this PR. - [x] ~There's an issue where, if you're in Discover, clicking the link won't refresh the page. .~ This is fixed on Discover side by reinitializing the state container when user clicks the "try es|ql" link and URL state updates. - [x] ~There is an issue that you can save the es|ql chart from Discover, but Dashboard's empty screen blocks the navigation because data views don't exist elastic#174316 (comment). This is fixed by allowing the dashboard to work without the default data view. Hopefully, this won't lead to major issues - [x] ~ES|QL panels can't be created without the default data views~ this is fixed by trying to fallback to an ad-hoc dataview, plan to move that code to the utils introduced here elastic#174736 - [x] fix circular deps - [x] Add functional tests ## Visuals https://github.com/elastic/kibana/assets/7784120/af3592c1-f4c8-43bb-a128-3268b7761367 ### Storybook Stories #### Can access ES|QL ![Screenshot 2024-01-31 at 17 05 47](https://github.com/elastic/kibana/assets/7784120/370d0351-198e-4dc3-b22e-86f497ad4df5) #### Cannot access (e.g. preview is unavailable - _not implemented_) ![Screenshot 2024-01-31 at 17 05 59](https://github.com/elastic/kibana/assets/7784120/c2bf52ab-9fa8-4f25-9e5d-512d4f4342fa) --------- Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co> Co-authored-by: Rachel Shen <rshen@elastic.co> Co-authored-by: Anton Dosov <anton.dosov@elastic.co>
Summary
There's an issue where, if you're in Discover, clicking the link won't refresh the page. .This is fixed on Discover side by reinitializing the state container when user clicks the "try es|ql" link and URL state updates.There is an issue that you can save the es|ql chart from Discover, but Dashboard's empty screen blocks the navigation because data views don't exist [ES|QL] Bypass no data views screen #174316 (comment). This is fixed by allowing the dashboard to work without the default data view. Hopefully, this won't lead to major issuesES|QL panels can't be created without the default data viewsthis is fixed by trying to fallback to an ad-hoc dataview, plan to move that code to the utils introduced here [ES|QL] Use same adhoc dataviews for queries with the same index pattern #174736Visuals
esql.empty.prompt.demo.mov
Storybook Stories
Can access ES|QL
Cannot access (e.g. preview is unavailable - not implemented)
cc: @stratoula @mattkime @thomasneirynck