[Discover] Rely on `dataSource` for ES|QL mode #183108

davismcphee · 2024-05-10T03:37:36Z

Summary

This PR updates Discover to rely on dataSource for determining if ES|QL mode is enabled instead of relying on query directly. This creates a clearer separation between the modes and moves Discover toward supporting multiple data source types. It also includes a number of cleanups around ES|QL mode that make the intent of the code clearer and removes tech debt / code duplication.

Changes included in the PR:

Add a new useIsEsqlMode hook that relies on dataSource for checking if Discover is in ES|QL mode.
Remove "raw record type" concept in Discover and replace it with ES|QL dataSource checks.
Remove references to isPlainRecord and replace them with ES|QL dataSource checks.
Remove isTextBasedQuery utility function and replace it with ES|QL dataSource checks or isOfAggregateQueryType where casting is necessary.
Replace references to isOfAggregateQueryType with ES|QL dataSource checks except where casting is necessary.
Replace other references to "text based" with "ES|QL" for clarity and consistency.

Closes #181963.

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
Flaky Test Runner was used on any tests changed
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

davismcphee · 2024-05-10T03:37:49Z

/ci

davismcphee · 2024-05-10T20:14:30Z

/ci

davismcphee · 2024-05-10T20:53:50Z

/ci

davismcphee · 2024-05-10T22:07:33Z

/ci

davismcphee · 2024-05-13T01:56:17Z

/ci

davismcphee · 2024-05-15T23:36:03Z

/ci

elasticmachine · 2024-05-16T01:25:10Z

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

jughosta

Awesome! Thanks for the clean up!

I did not see any issues which would be reproducible. At some point I could not switch to Field Statistics tab, another time the histogram was missing - but I can't get to these issues again, maybe something to keep an eye on.

jughosta · 2024-05-16T12:29:01Z

src/plugins/discover/public/application/main/components/layout/discover_main_content.test.tsx

-    it('should include DocumentViewModeToggle when isPlainRecord is true', async () => {
-      const component = await mountComponent({ isPlainRecord: true });
+    it('should include DocumentViewModeToggle when in ES|QL mode', async () => {
+      const component = await mountComponent();


These tests look the same now 🤔

Yeah, @davismcphee I think we don't need both

Good catch! I updated the test to correctly set this one to ES|QL mode: 42afbd3.

jughosta · 2024-05-16T12:48:00Z

src/plugins/discover/public/components/discover_tour/discover_tour_provider.tsx

@@ -215,13 +211,13 @@ export const DiscoverTourProvider = ({
  );
  const tourSteps = useMemo(
    () =>
-      isPlainRecord


Oh, I think we don't need this condition anymore. Looks like ES|QL now also supports all the mentioned functionality.

@davismcphee it so nice to see this go away ... the naming wasn't good anyway, I wonder who ... ah .. it was me :)
I wonder why I didn't I name this isBoringRecord

What is it about? What is the mentioned functionality?

@stratoula It's a tour explaining how to use the data grid. For data view mode it has more steps configured.

I can address that separately to show all steps for ES|QL mode too, since we've added more features to it.

Sounds fantastic, thanx Julia!

Makes sense, but yes let's include them in a separate PR to avoid any functionality changes in this one. I opened an issue for it here: #183791.

stratoula

This is a very brave PR 👏 I added some comments / questions but nothing critical. I also did some testing and everything seems to work as expected.

It will cause some conflicts on #180849 and #182557 but I don't think it will be difficult to resolve them.

stratoula · 2024-05-17T06:33:44Z

src/plugins/discover/public/application/main/components/layout/discover_documents.tsx

@@ -416,12 +408,12 @@ function DiscoverDocumentsComponent({
                  rowHeightState={rowHeight}
                  onUpdateRowHeight={onUpdateRowHeight}
                  isSortEnabled={true}
-                  isPlainRecord={isTextBasedQuery}
+                  isPlainRecord={isEsqlMode}


Why here we are keeping the isPlainRecord and we dont change the prop to isEsqlMode ? I see that we are doing this in other places

Because this component is a trick 😄 It's actually inheriting the props from Unified Data Table, and I wanted to keep this PR limited to the Discover codebase. But it would be a good idea to make similar changes within our shared components too separately.

stratoula · 2024-05-17T06:36:33Z

src/plugins/discover/public/application/main/components/layout/discover_main_content.test.tsx

-    it('should include DocumentViewModeToggle when isPlainRecord is true', async () => {
-      const component = await mountComponent({ isPlainRecord: true });
+    it('should include DocumentViewModeToggle when in ES|QL mode', async () => {
+      const component = await mountComponent();


Yeah, @davismcphee I think we don't need both

stratoula · 2024-05-17T06:47:42Z

src/plugins/discover/public/components/discover_tour/discover_tour_provider.tsx

@@ -215,13 +211,13 @@ export const DiscoverTourProvider = ({
  );
  const tourSteps = useMemo(
    () =>
-      isPlainRecord


What is it about? What is the mentioned functionality?

kibana-ci · 2024-05-18T02:07:28Z

💚 Build Succeeded

Buildkite Build
Commit: 42afbd3

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`discover`	911	910	-1

Async chunks

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

id	before	after	diff
`discover`	808.8KB	809.4KB	+567.0B
`unifiedDocViewer`	860.0KB	859.9KB	-10.0B
total			+557.0B

Page load bundle

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

id	before	after	diff
`discover`	35.6KB	35.6KB	-44.0B
`unifiedDocViewer`	11.5KB	11.4KB	-20.0B
total			-64.0B

Unknown metric groups

async chunk count

id	before	after	diff
`discover`	24	23	-1

History

💚 Build #210339 succeeded eb3556d
💚 Build #209375 succeeded a936f16
💔 Build #209327 failed 034d0a48b5586f007e623359e0322ade3c5edd59
💔 Build #209318 failed b81d48111fc1c392e3e0f48e44d4eee9e7db6977
💔 Build #209311 failed 75734afb2d078ae19b5627d1f6aa63f9875194f9
💔 Build #209106 failed 79ee67a010a4450ff1f6dfa45a58d610f0634d22

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

cc @davismcphee

davismcphee

Thanks @jughosta and @stratoula for the feedback! I fixed the Jest test and created a followup issue for the Discover tour. I'm going to merge this PR now since it's a merge conflict magnet 🧲 with all these file changes.

I did not see any issues which would be reproducible. At some point I could not switch to Field Statistics tab, another time the histogram was missing - but I can't get to these issues again, maybe something to keep an eye on.

This is interesting, although I also tried quite a bit to reproduce and was unable to. I agree, let's proceed with this PR since none of us can reproduce it, and keep an eye out for those issues.

davismcphee · 2024-05-18T00:50:50Z

src/plugins/discover/public/application/main/components/layout/discover_main_content.test.tsx

-    it('should include DocumentViewModeToggle when isPlainRecord is true', async () => {
-      const component = await mountComponent({ isPlainRecord: true });
+    it('should include DocumentViewModeToggle when in ES|QL mode', async () => {
+      const component = await mountComponent();


Good catch! I updated the test to correctly set this one to ES|QL mode: 42afbd3.

davismcphee · 2024-05-18T00:59:49Z

src/plugins/discover/public/components/discover_tour/discover_tour_provider.tsx

@@ -215,13 +211,13 @@ export const DiscoverTourProvider = ({
  );
  const tourSteps = useMemo(
    () =>
-      isPlainRecord


Makes sense, but yes let's include them in a separate PR to avoid any functionality changes in this one. I opened an issue for it here: #183791.

davismcphee · 2024-05-18T01:04:05Z

src/plugins/discover/public/application/main/components/layout/discover_documents.tsx

@@ -416,12 +408,12 @@ function DiscoverDocumentsComponent({
                  rowHeightState={rowHeight}
                  onUpdateRowHeight={onUpdateRowHeight}
                  isSortEnabled={true}
-                  isPlainRecord={isTextBasedQuery}
+                  isPlainRecord={isEsqlMode}


Because this component is a trick 😄 It's actually inheriting the props from Unified Data Table, and I wanted to keep this PR limited to the Discover codebase. But it would be a good idea to make similar changes within our shared components too separately.

davismcphee added release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. Project:OneDiscover Enrich Discover with contextual awareness labels May 10, 2024

davismcphee self-assigned this May 10, 2024

davismcphee force-pushed the one-discover-esql-cleanup branch from 034d0a4 to a936f16 Compare May 13, 2024 01:56

davismcphee added 12 commits May 15, 2024 20:14

Begin updating ES|QL checks

8fbb5d4

Remove references to isPlainRecord

68c5610

Removing textbased references

9a75f9f

Update translation keys

06b01a6

Replace occurrences of plain record and text based

f293da6

Rename files

67379fb

Cleanup

e832785

Fix translation value name

08fabc9

Clean up isOfAggregateQueryType references

5f9649b

Fix use of useIsEsqlMode in embeddable code

7db393e

Fix Jest tests

fa44891

Fix type in Jest test from merge

eb3556d

davismcphee force-pushed the one-discover-esql-cleanup branch from a936f16 to eb3556d Compare May 15, 2024 23:31

davismcphee changed the title ~~[Discover] ES|QL cleanup~~ [Discover] Rely on dataSource for ES|QL mode May 15, 2024

davismcphee marked this pull request as ready for review May 16, 2024 01:25

davismcphee requested a review from a team as a code owner May 16, 2024 01:25

kertal requested a review from a team May 16, 2024 07:23

jughosta approved these changes May 16, 2024

View reviewed changes

stratoula reviewed May 17, 2024

View reviewed changes

stratoula mentioned this pull request May 17, 2024

[ML] Adds ES|QL support for field statistics table in Discover #180849

Merged

10 tasks

davismcphee added 2 commits May 17, 2024 21:30

Merge branch 'main' into one-discover-esql-cleanup

c6d0285

Fix discover_main_content test

42afbd3

davismcphee commented May 18, 2024

View reviewed changes

davismcphee merged commit 808a89c into elastic:main May 18, 2024
17 checks passed

kibanamachine added v8.15.0 backport:skip This commit does not require backporting labels May 18, 2024

davismcphee deleted the one-discover-esql-cleanup branch May 21, 2024 00:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Rely on `dataSource` for ES|QL mode #183108

[Discover] Rely on `dataSource` for ES|QL mode #183108

davismcphee commented May 10, 2024 •

edited

Loading

davismcphee commented May 10, 2024

davismcphee commented May 10, 2024

davismcphee commented May 10, 2024

davismcphee commented May 10, 2024

davismcphee commented May 13, 2024

davismcphee commented May 15, 2024

elasticmachine commented May 16, 2024

jughosta left a comment

jughosta May 16, 2024

stratoula May 17, 2024

davismcphee May 18, 2024

jughosta May 16, 2024

kertal May 16, 2024

stratoula May 17, 2024

jughosta May 17, 2024

stratoula May 17, 2024

davismcphee May 18, 2024

stratoula left a comment

stratoula May 17, 2024

davismcphee May 18, 2024

stratoula May 17, 2024

stratoula May 17, 2024

kibana-ci commented May 18, 2024

async chunk count

davismcphee left a comment

davismcphee May 18, 2024

davismcphee May 18, 2024

davismcphee May 18, 2024

[Discover] Rely on dataSource for ES|QL mode #183108

[Discover] Rely on dataSource for ES|QL mode #183108

Conversation

davismcphee commented May 10, 2024 • edited Loading

Summary

Checklist

For maintainers

davismcphee commented May 10, 2024

davismcphee commented May 10, 2024

davismcphee commented May 10, 2024

davismcphee commented May 10, 2024

davismcphee commented May 13, 2024

davismcphee commented May 15, 2024

elasticmachine commented May 16, 2024

jughosta 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stratoula 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

kibana-ci commented May 18, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

async chunk count

History

davismcphee 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

[Discover] Rely on `dataSource` for ES|QL mode #183108

[Discover] Rely on `dataSource` for ES|QL mode #183108

davismcphee commented May 10, 2024 •

edited

Loading