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

[Lens] Enables text-based languages (SQL) #140469

Merged
merged 62 commits into from
Sep 27, 2022
Merged

Conversation

stratoula
Copy link
Contributor

@stratoula stratoula commented Sep 12, 2022

Summary

Closes #137811

Enables text based query languages in Lens.

lens

This is the initial PR so many features are not included. It's primary goal is to set up the architecture of the text based languages in Lens. Things to have in mind:

  • You can create only one layer. There are some UI limitations on supporting multiple layers that we are discussing. This is going to work on the future
  • Moving from one datasource to another cleanups the unsaved state
  • All the data manipulation is now happening from the text based language. For this reason we allow only chart-specific configuration
  • You can't create filters by clicking on a chart
  • As the data manipulation is done by the query there are some visualizations that might seem "weird" depending on the query. We want to improve the UX here by adding some warnings in the future.
  • Discover to Lens and the opposite direction is not supported for now.
  • The suggestions can be improved but this is not on the scope of this PR.

How to test

The feature is hidden under an advanced setting. To enable it go to advanced settings and switch on the "Enable SQL" setting. If you navigate to Lens and open the dataview picker you will see a new entry

image

Flaky runners

https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1303 25 times
https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1304 100 times

Checklist

@stratoula stratoula changed the title Sql in lens [Lens] Enables text-based languages (SQL) Sep 12, 2022
@@ -315,7 +315,7 @@ export const getUiSettings: (docLinks: DocLinksServiceSetup) => Record<string, U
value: false,
description: i18n.translate('discover.advancedSettings.enableSQLDescription', {
defaultMessage:
'{technicalPreviewLabel} This tech preview feature is highly experimental--do not rely on this for production saved searches or dashboards. This setting enables SQL as a text-based query language in Discover. If you have feedback on this experience please reach out to us on {link}',
'{technicalPreviewLabel} This tech preview feature is highly experimental--do not rely on this for production saved searches, visualizations or dashboards. This setting enables SQL as a text-based query language in Discover and Lens. If you have feedback on this experience please reach out to us on {link}',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ We are using the same switch with discover. I have slightly changed the text to include Lens. The setting is still on the Discover section.

@@ -262,7 +271,7 @@ export function ChangeDataView({
setIsTextBasedLangSelected(false);
// clean up the Text based language query
onTextLangQuerySubmit?.({
language: 'kql',
language: 'kuery',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ This is called on cleanup (i.e. when navigating from the text based languages mode to the dataviewMode. I was wrongly setting the language to kql while it is kuery

return (
<>
<Easteregg query={externalContext?.query} />
{Object.keys(props.datasourceMap).length > 1 && (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ This was adding a popover for changing datasources (in case of more than one). In this PR we are introducing a new datasource so this begun to be displayed but we don't want it. The datasource change is done by the dataview picker.

@stratoula stratoula marked this pull request as ready for review September 23, 2022 13:19
@stratoula stratoula requested review from a team as code owners September 23, 2022 13:19
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

Bundle bump LGTM

@@ -158,7 +159,7 @@ const toExpression = (
showLabels: [!attributes?.mode || attributes?.mode === 'full'],
colorMode: !canColor ? [ColorMode.None] : [state?.colorMode || ColorMode.None],
autoScale: [true],
colorFullBackground: [true],
colorFullBackground: [!isTextBasedLanguage],
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is because of the possibility of multiple metrics being rendered, then I think it's fine to just ignore it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@flash1293 I did this, because when this is set to true I get this error
image

Any suggestions here?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO that's alright for now (especially as it's only the legacy vis)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 61b1af1

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

On dashboards it doesn't reload when changing the time range (it does reload when changing filters though)

@stratoula
Copy link
Contributor Author

@flash1293 timeRange error fixed! Thanx for the great help!

I have also removed the cleaning up of the layers on easy query update. You can see how it works now here:

lens

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

app services code LGTM

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.

DataDiscovery.team owned code LGTM, just a function and a Advanced setting was renamed. SQL in Discover works as expected

const layer = props.state.layers[props.layerId];
const selectedField = layer?.allColumns?.find(
(column) => column.columnId === props.columnId
)!;
Copy link
Contributor

Choose a reason for hiding this comment

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

the selected field might be not available, we should handle this case by rendering a "FIELD MISSING" label or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 309b68c

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

This works pretty well now - just found one issue

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Tested again and everything works fine. Thanks for the change with not dropping the config on query change, it works like a charm!

@stratoula
Copy link
Contributor Author

Thanx for your comments Joe!

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Rules, Alerts and Exceptions ResponseOps Cypress Tests on Security Solution / Alerts detection rules table auto-refresh should disable auto refresh when any rule selected and enable it after rules unselected

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
data 518 519 +1
lens 908 917 +9
total +10

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
data 2508 2509 +1
lens 558 560 +2
total +3

Async chunks

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

id before after diff
discover 477.4KB 477.4KB +13.0B
lens 1.2MB 1.3MB +16.5KB
unifiedSearch 260.8KB 260.9KB +80.0B
total +16.6KB

Page load bundle

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

id before after diff
data 432.2KB 432.5KB +306.0B
lens 35.4KB 35.6KB +275.0B
unifiedSearch 47.0KB 47.1KB +66.0B
total +647.0B
Unknown metric groups

API count

id before after diff
data 3211 3213 +2
lens 646 649 +3
total +5

History

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

@stratoula stratoula merged commit 4c3cd03 into elastic:main Sep 27, 2022
jennypavlova pushed a commit to jennypavlova/kibana that referenced this pull request Sep 27, 2022
* [Lens] Enable text-based languages-sql

* Display data

* Chart switcher and further fixes

* Drag and drop fixes

* Small fix

* Multiple improvements

* Errors implementation and save and exit

* Some cleanup

* Fix types failures

* Revert change

* Fix underlying data error

* Fix jest test

* Fixes app test

* Rename datasource to textBased

* display the dataview picker component but disabled

* Fix functional test

* Refactoring

* Populate the dataview to theembeddable

* Load sync

* sync load of the new dtsource

* Fix

* Fix bug when the dtaview is not found

* Refactoring

* Add some unit tests

* Add some unit tests

* Add more unit tests

* Add a functional test

* Add all files

* Update lens limit

* Fixes bug

* Bump lens size

* Fix flakiness

* Further fixes

* Fix check

* More fixes

* Fix test

* Wait for query to run

* More changes

* Fix

* Fix the function input to fetch from variable

* Remove the colorFullBackground check

* Fix dashboard bug when timeRange changes

* Remove the clear on query update, show column error instead

* Render a field missing label in case the label is not defined

* Fix

Co-authored-by: Joe Reuter <johannes.reuter@elastic.co>
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 Feature:Lens Feature:Unified search Unified search related tasks release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Text based query languages integration
7 participants