[Canvas] Use data views service #139610

cqliu1 · 2022-08-29T05:42:21Z

Summary

Related to #91715.
Merges #130320 and #138922.

This uses the data views service instead of the saved object client to retrieve data views. These changes should only affect the esdocs and escount datasources.

Checklist

Delete any items that are not applicable to this PR.

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

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2022-08-29T15:22:56Z

Pinging @elastic/kibana-presentation (Team:Presentation)

mattkime · 2022-08-29T15:26:19Z

x-pack/plugins/canvas/public/services/kibana/data_views.ts

+  getDataViews: async () => {
+    try {
+      const dataViews = await startPlugins.data.dataViews.getIdsWithTitle();
+      return dataViews.map((view) => view.title);


How is the list of data views being used? Title is no longer unique, and generally speaking data views should be referenced by name in UIs.

We use it to populate this dropdown menu, but I can certainly switch to using the IDs as the value and the name as the label. Let me update the PR.

For BWC, I won't be able to use the data view id instead of the title because this value is used directly in the SQL statement generated in this esdocs datasource function and updates the SQL statement to something like SELECT * FROM kibana_sample_data_flights. Setting the value to id instead results in an error because ES doesn't recognize this ID and ES SQL expects an index name here. I can use name for the label for each of the options, but I'll still need to use title for the value selected and inserted into the SQL statement.

I'm not sure that uniqueness is a requirement here because if you have two data views with the same title, ES will just return results from both data views right?

Ouch, thats an awkward mixing of metaphors. This should definitely be resolved but I'm not sure how. Data views should be referenced by name even if their title is used for a query.

Any idea how we might address this? Who owns SQL?

Perhaps a short term fix would be adding more info to each item in the list - display the data view name but beneath it show the index pattern.

Any idea how we might address this? Who owns SQL?

@elastic/es-ql owns ES SQL

Perhaps a short term fix would be adding more info to each item in the list - display the data view name but beneath it show the index pattern.

You mean display both the title and name for each data view in the dropdown list? Is there a problem with seeing the name only in the option list and seeing the title in the expression/SQL statement?

Is there a problem with seeing the name only in the option list and seeing the title in the expression/SQL statement?

Thats your call but I worry that it might confuse some.

x-pack/plugins/canvas/i18n/ui.ts

cqliu1 · 2022-10-18T15:34:48Z

Here's how it looks when I'm using a data view:

Here's an expression where I use the exact name of an index instead of a dataview:

This test index doesn't appear in the options list as an available option to select, but it is still a valid custom input because the function supports querying by index name directly without using dataviews.

mattkime

Data view usage looks good to me

ThomThomson

Nice service addition, and I looked through the code and things look pretty good. Left some small nits!

Also ran this locally, and was able to select data views as expected. Verified that the expression looks as expected too.

Ran into the router issue, which was super annoying because it happened when I saved the change after changing from demo data to es docs, it kicked me back to the listing page and did that weird loading URL thing.

Anyway, LGTM!

ThomThomson · 2022-10-19T21:57:42Z

x-pack/plugins/canvas/public/components/datasource/datasource_component.js

+      .getServices()
+      .dataViews.getDefaultDataView()
+      .then((defaultDataView) => {
+        console.log('componnentDidMount', defaultDataView);


Should we remove the console.log here?

ThomThomson · 2022-10-19T22:02:52Z

x-pack/plugins/canvas/public/components/es_data_view_select/es_data_view_select.tsx

+
+type ESDataViewSelectProps = Omit<Props, 'indices' | 'loading'>;
+
+export const ESDataViewSelect: React.FunctionComponent<ESDataViewSelectProps> = (props) => {


Nit: Not necessary for this PR, but I'm wondering if we should standardize the way we define functional components as part of that style guide we wrote. I usually do:

export const ESDataViewSelect = ({ value, onChange }: ESDataViewSelectProps) => { ... }

Instead of how it's done here. I find that that is more concise and easier to understand, but it isn't explicity labeled as a function component, the dev has to infer that based on the fact that it returns JSX. Wondering if we should come together to use a standard definition, for all of our code, or if we should just define it in the same way it's defined elsewhere in the plugin.

I actually prefer using the FC type like this:

import { FC } from 'react'; export const ESDataViewSelect: FC<ESDataViewSelectProps> = ({ value, onChange }) => { ... }

which only adds two characters while also being easy to read/concise.

Sounds like a good pattern!

ThomThomson · 2022-10-19T22:05:00Z

x-pack/plugins/canvas/public/components/es_data_view_select/es_data_view_select.tsx

+
+  useEffectOnce(() => {
+    getDataViews().then((newDataViews) => {
+      console.log({ newDataViews });


Nit: remove console logs - I thought we had some sort of linting rule that highlighted these - but maybe that's not the case in Canvas?

I'm not really sure why the precommit check didn't catch this before letting me commit. I don't think it's a canvas specific issue. Maybe something's wrong with my dev environment?

I'm not actually sure if the pre-commit checks get run anymore, I think you might have to run them manually now.

cqliu1 · 2022-10-24T18:23:33Z

@elasticmachine merge upstream

kibana-ci · 2022-10-24T19:33:39Z

💚 Build Succeeded

Metrics [docs]

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	-1.0KB

Page load bundle

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

id	before	after	diff
`canvas`	13.3KB	13.2KB	-8.0B

Unknown metric groups

References to deprecated APIs

id	before	after	diff
`canvas`	62	72	+10

History

💚 Build #81875 succeeded 5a44ae3
💔 Build #81667 failed 899c298
💔 Build #81105 failed 4ed24f6
💔 Build #81072 failed 52635e7
💚 Build #69786 succeeded e5093bd36dc888cd96784af31b1e12f85bcf9303
💚 Build #68033 succeeded 56949c051288a2646b1e797ba61d97a22b961a99

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

## Summary Closes #157101 and #157083. This refactors all Canvas routes to use the versioned router for BWC and makes access for all routes internal only. I also removed the `es_fields` routes because the last usage of that route was removed in #139610. All routes have been changed from `/api/canvas/*` to `/internal/canvas/*`. ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) 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](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### Risk Matrix Delete this section if it is not applicable to this PR. Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release. When forming the risk matrix, consider some of the following examples and how they may potentially impact the change: | Risk | Probability | Severity | Mitigation/Notes | |---------------------------|-------------|----------|-------------------------| | Multiple Spaces—unexpected behavior in non-default Kibana Space. | Low | High | Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces. | | Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. | High | Low | Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure. | | Code should gracefully handle cases when feature X or plugin Y are disabled. | Medium | High | Unit tests will verify that any feature flag or plugin combination still results in our service operational. | | [See more potential risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) | ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

This was referenced Aug 29, 2022

[Canvas] Use dataviews api in canvas #130320

Closed

[Canvas] Use index pattern service to load index patterns #138922

Closed

cqliu1 requested a review from mattkime August 29, 2022 14:36

cqliu1 marked this pull request as ready for review August 29, 2022 15:22

cqliu1 requested a review from a team as a code owner August 29, 2022 15:22

mattkime requested changes Aug 29, 2022

View reviewed changes

gchaps reviewed Aug 29, 2022

View reviewed changes

x-pack/plugins/canvas/i18n/ui.ts Outdated Show resolved Hide resolved

Use data views service in Canvas

52635e7

cqliu1 force-pushed the canvas/use-data-views-service branch from e5093bd to 52635e7 Compare October 18, 2022 15:28

Use getIndexPattern instead of title property

4ed24f6

mattkime approved these changes Oct 18, 2022

View reviewed changes

ThomThomson approved these changes Oct 19, 2022

View reviewed changes

cqliu1 added 2 commits October 19, 2022 23:53

Fix ts errors

899c298

Remove console log statement

5a44ae3

Merge branch 'main' into canvas/use-data-views-service

08227a6

cqliu1 enabled auto-merge (squash) October 24, 2022 18:23

cqliu1 merged commit 670fe25 into elastic:main Oct 24, 2022

kibanamachine added the v8.6.0 label Oct 24, 2022

cqliu1 deleted the canvas/use-data-views-service branch October 25, 2022 15:27

cqliu1 mentioned this pull request Jun 13, 2023

[Canvas] Versioned and internal routes #159426

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Canvas] Use data views service #139610

[Canvas] Use data views service #139610

cqliu1 commented Aug 29, 2022 •

edited

Loading

elasticmachine commented Aug 29, 2022

mattkime Aug 29, 2022

cqliu1 Aug 29, 2022

cqliu1 Sep 13, 2022

mattkime Sep 13, 2022

mattkime Sep 13, 2022

cqliu1 Sep 14, 2022

mattkime Sep 14, 2022

cqliu1 commented Oct 18, 2022

mattkime left a comment

ThomThomson left a comment

ThomThomson Oct 19, 2022

ThomThomson Oct 19, 2022

cqliu1 Oct 20, 2022

ThomThomson Oct 20, 2022

ThomThomson Oct 19, 2022

cqliu1 Oct 20, 2022

ThomThomson Oct 20, 2022

cqliu1 commented Oct 24, 2022

kibana-ci commented Oct 24, 2022

References to deprecated APIs


		type ESDataViewSelectProps = Omit<Props, 'indices' \| 'loading'>;

		export const ESDataViewSelect: React.FunctionComponent<ESDataViewSelectProps> = (props) => {

[Canvas] Use data views service #139610

[Canvas] Use data views service #139610

Conversation

cqliu1 commented Aug 29, 2022 • edited Loading

Summary

Checklist

Risk Matrix

For maintainers

elasticmachine commented Aug 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cqliu1 commented Oct 18, 2022

mattkime left a comment

Choose a reason for hiding this comment

ThomThomson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cqliu1 commented Oct 24, 2022

kibana-ci commented Oct 24, 2022

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

References to deprecated APIs

History

cqliu1 commented Aug 29, 2022 •

edited

Loading