[ES|QL] Adds the ability to breakdown the histogram in Discover #193820

stratoula · 2024-09-24T07:06:13Z

Summary

Part of #186369

It enables the users to breakdown the histogram visualization in Discover.

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
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)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

kibanamachine · 2024-09-25T08:34:19Z

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#7010

[✅] test/functional/apps/discover/esql/config.ts: 25/25 tests passed.

see run history

elasticmachine · 2024-09-25T11:02:09Z

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

jughosta · 2024-09-25T13:17:52Z

We should make sure to unskip functional tests before merging the PR #184600

stratoula · 2024-09-25T13:35:06Z

@jughosta who is on it?

jughosta · 2024-09-26T15:40:18Z

The test should be enabled now. Hopefully, it won't get skipped again before we merge.

stratoula · 2024-09-26T16:15:49Z

Thanx Jul 🙌 I will update tomorrow and hopefully it will pass to my PR

src/plugins/unified_histogram/public/container/hooks/use_state_props.ts

src/plugins/unified_histogram/public/layout/layout.tsx

test/functional/apps/discover/group3/_lens_vis.ts

kertal

Awesome work @stratoula!
Some initial testing feedback, it seems filter out is creating the same filter like "Filter for"

While testing with a totally nuts field to breakdown by, edge case testing, I was wondering, shouldn't we limit the values displayed in the histogram? Or maybe don't render it in this case, since it doesn't make much sense?

You can test it here: https://kertal-pr-193820-breakdown-esql-discover.kbndev.co/app/r/s/YngOs

stratoula · 2024-09-30T06:22:02Z

@kertal nice catch about the filter out but unfortunately it happens in main too and is irrelevant with this PR. I will create an issue but you can check in main this scenario

Updated: Here is the bug issue #194373

About the limit this is not very easy (at least from the ES|QL query) due to the double breakdkown (time and field) and I can't set a limit size in the legend either (if I am not mistaken). So I am not sure how we could tackle it tbh. 🤔

The maximum number of values will be 1000 (taken under consideration that the query will return 1000 rows with different values so I think we are ok. (Also it feels like an edge case?)

jughosta

Looking great! Left a couple of suggestions.

jughosta · 2024-09-30T13:59:49Z

src/plugins/unified_histogram/public/chart/breakdown_field_selector.tsx

+    return esqlColumns.map((column) => ({
+      name: column.name,
+      displayName: column.name,
+      type: column.meta?.type,


We could use convertDatatableColumnToDataViewFieldSpec here too for consistency in how types are treated and map as new DataViewField(convertDatatableColumnToDataViewFieldSpec(column))

We might also want to filter out unsupported field types with fieldSupportsBreakdown for ES|QL case too.

Very nice suggestion, implemented here 9e731df

Note: I don't filter out as we do with the dataview mode as they are not the same operations. Here you can group by with all the filed types. But I added a filter out for unsupported fields as these are not supported by the BY operator e757a54

jughosta · 2024-09-30T14:03:02Z

src/plugins/unified_histogram/public/chart/breakdown_field_selector.tsx

+      if (esqlColumns) {
+        const breakdownColumn = esqlColumns?.find((column) => column.name === chosenOption?.value);
+        breakdownField = breakdownColumn
+          ? new DataViewField(convertDatatableColumnToDataViewFieldSpec(breakdownColumn))
+          : undefined;
+      } else {
+        breakdownField = chosenOption?.value
+          ? dataView.fields.find((currentField) => currentField.name === chosenOption.value)
+          : undefined;
+      }


We could simplify this: remove if case and go through fields array instead (considering it consists of data view fields or derived fields with new DataViewField(convertDatatableColumnToDataViewFieldSpec(column)))

Sure! Done here 9e731df

jughosta · 2024-09-30T14:12:37Z

src/plugins/unified_histogram/public/services/lens_vis_service.ts

  }): Suggestion | undefined => {
-    const { dataView, query, timeRange } = queryParams;
+    const { dataView, query, timeRange, columns } = queryParams;
+    const breakdownColumn = columns?.find((column) => column.name === breakdownField?.name);


Suggested change

const breakdownColumn = columns?.find((column) => column.name === breakdownField?.name);

const breakdownColumn = breakdownField?.name ? columns?.find((column) => column.name === breakdownField.name) : undefined;

^^ so we don't iterate through all the columns unless there is a breakdown field.

Done 9e731df

jughosta · 2024-09-30T14:16:23Z

src/plugins/unified_histogram/public/services/lens_vis_service.ts

  }): string => {
    const queryInterval = interval ?? computeInterval(timeRange, this.services.data);
    const language = getAggregateQueryMode(query);
    const safeQuery = removeDropCommandsFromESQLQuery(query[language]);
+    const breakdown = breakdownColumn
+      ? `, ${breakdownColumn.name} | sort ${breakdownColumn.name} asc`


Shall we escape column name?

We should, nice catch. Done here 9e731df

jughosta · 2024-09-30T14:18:58Z

src/plugins/unified_histogram/public/services/lens_vis_service.ts

    const currentQuery =
      suggestionType === UnifiedHistogramSuggestionType.histogramForESQL && isTextBased && timeRange
        ? {
-            esql: this.getESQLHistogramQuery({ dataView, query, timeRange }),
+            esql: this.getESQLHistogramQuery({ dataView, query, timeRange, breakdownColumn }),


Suggested change

esql: this.getESQLHistogramQuery({ dataView, query, timeRange, breakdownColumn }),

esql: this.getESQLHistogramQuery({ dataView, query, timeRange, breakdownColumn: breakdownField?.name ? columns?.find((column) => column.name === breakdownField.name) : undefined}),

Same, iterate only if necessary.

Done 9e731df

stratoula · 2024-10-01T06:52:43Z

/ci

kibana-ci · 2024-10-01T08:18:58Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: b66c58d

Failed CI Steps

Metrics [docs]

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
`unifiedHistogram`	36	37	+1

Async chunks

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

id	before	after	diff
`lens`	1.5MB	1.5MB	+88.0B
`unifiedHistogram`	67.9KB	69.4KB	+1.5KB
total			+1.6KB

Unknown metric groups

API count

id	before	after	diff
`unifiedHistogram`	71	72	+1

History

💛 Build #237866 was flaky cae1002
💚 Build #237726 succeeded 1b95c94
💛 Build #237588 was flaky 3fb5319
💚 Build #237219 succeeded cc03575
💛 Build #236946 was flaky 97d8cbd
💚 Build #236871 succeeded baf8933

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

cc @stratoula

jughosta

LGTM, thanks! 👍

kibanamachine · 2024-10-01T09:29:53Z

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11122563398

…tic#193820) ## Summary Part of elastic#186369 It enables the users to breakdown the histogram visualization in Discover. ![meow](https://github.com/user-attachments/assets/d5fdaa41-0a69-4caf-9da2-1221dcfd5ce2) ### Checklist - [x] 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) - [x] [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 - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [x] 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)) - [x] 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)) - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) --------- Co-authored-by: kibanamachine <[email protected]> (cherry picked from commit dfe00f2)

kibanamachine · 2024-10-01T09:34:14Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…#193820) (#194534) # Backport This will backport the following commits from `main` to `8.x`: - [[ES|QL] Adds the ability to breakdown the histogram in Discover (#193820)](#193820)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Stratoula Kalafateli <[email protected]>

stratoula added 5 commits September 23, 2024 10:10

[ES|QL][Discover] Enables the breakdown in the histogram

7c79925

Get fields from the query

ef0e4e6

Merge branch 'main' into breakdown-esql-discover

a4c9d0b

Merge branch 'main' into breakdown-esql-discover

b05bcbf

Adjust for numeric fields

6ec8eb8

stratoula changed the title ~~Breakdown esql discover~~ [ES|QL] Adds the ability to breakdown the histogram in Discover Sep 24, 2024

stratoula and others added 6 commits September 24, 2024 11:29

Add some unit tests

906cc89

More unit tests

7609ce9

[CI] Auto-commit changed files from 'node scripts/notice'

128a1a2

Adds a FT

59a3d48

Fix filtering problem

e1698d7

Fix functional test

baf8933

Cleanup

97d8cbd

stratoula marked this pull request as ready for review September 25, 2024 11:02

stratoula requested review from a team as code owners September 25, 2024 11:02

Merge branch 'main' into breakdown-esql-discover

cc03575

stratoula added 2 commits September 27, 2024 08:43

Merge branch 'main' into breakdown-esql-discover

527bf1b

Update FT

3fb5319

markov00 reviewed Sep 27, 2024

View reviewed changes

src/plugins/unified_histogram/public/container/hooks/use_state_props.ts Show resolved Hide resolved

src/plugins/unified_histogram/public/layout/layout.tsx Outdated Show resolved Hide resolved

test/functional/apps/discover/group3/_lens_vis.ts Outdated Show resolved Hide resolved

Address PR comments

1b95c94

kertal assigned stratoula Sep 27, 2024

markov00 self-requested a review September 27, 2024 15:53

markov00 approved these changes Sep 27, 2024

View reviewed changes

kertal reviewed Sep 27, 2024

View reviewed changes

kertal requested a review from a team September 27, 2024 15:57

Merge branch 'main' into breakdown-esql-discover

cae1002

kertal mentioned this pull request Sep 30, 2024

[ES|QL][Lens] Fixes the filter out legend action #194374

Merged

1 task

jughosta reviewed Sep 30, 2024

View reviewed changes

stratoula added 3 commits October 1, 2024 08:42

Address PR comments

9e731df

Filter unsupported fields

e757a54

Merge branch 'main' into breakdown-esql-discover

b66c58d

jughosta approved these changes Oct 1, 2024

View reviewed changes

stratoula merged commit dfe00f2 into elastic:main Oct 1, 2024
22 checks passed

kibanamachine mentioned this pull request Oct 1, 2024

[8.x] [ES|QL] Adds the ability to breakdown the histogram in Discover (#193820) #194534

Merged

stratoula mentioned this pull request Oct 1, 2024

[ES|QL] [Unified histogram] Add a breakdown dropdown to the ES|QL histogram #186369

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ES|QL] Adds the ability to breakdown the histogram in Discover #193820

[ES|QL] Adds the ability to breakdown the histogram in Discover #193820

stratoula commented Sep 24, 2024 •

edited

Loading

kibanamachine commented Sep 25, 2024

elasticmachine commented Sep 25, 2024

jughosta commented Sep 25, 2024

stratoula commented Sep 25, 2024

jughosta commented Sep 26, 2024

stratoula commented Sep 26, 2024

kertal left a comment •

edited

Loading

stratoula commented Sep 30, 2024 •

edited

Loading

jughosta left a comment

jughosta Sep 30, 2024

jughosta Sep 30, 2024

stratoula Oct 1, 2024 •

edited

Loading

jughosta Sep 30, 2024

stratoula Oct 1, 2024

jughosta Sep 30, 2024

stratoula Oct 1, 2024

jughosta Sep 30, 2024

stratoula Oct 1, 2024

jughosta Sep 30, 2024

stratoula Oct 1, 2024

stratoula commented Oct 1, 2024

kibana-ci commented Oct 1, 2024

API count

jughosta left a comment

kibanamachine commented Oct 1, 2024

kibanamachine commented Oct 1, 2024

	const breakdownColumn = columns?.find((column) => column.name === breakdownField?.name);
	const breakdownColumn = breakdownField?.name ? columns?.find((column) => column.name === breakdownField.name) : undefined;

	esql: this.getESQLHistogramQuery({ dataView, query, timeRange, breakdownColumn }),
	esql: this.getESQLHistogramQuery({ dataView, query, timeRange, breakdownColumn: breakdownField?.name ? columns?.find((column) => column.name === breakdownField.name) : undefined}),

[ES|QL] Adds the ability to breakdown the histogram in Discover #193820

[ES|QL] Adds the ability to breakdown the histogram in Discover #193820

Conversation

stratoula commented Sep 24, 2024 • edited Loading

Summary

Checklist

kibanamachine commented Sep 25, 2024

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#7010

elasticmachine commented Sep 25, 2024

jughosta commented Sep 25, 2024

stratoula commented Sep 25, 2024

jughosta commented Sep 26, 2024

stratoula commented Sep 26, 2024

kertal left a comment • edited Loading

Choose a reason for hiding this comment

stratoula commented Sep 30, 2024 • edited Loading

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

stratoula Oct 1, 2024 • edited Loading

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 commented Oct 1, 2024

kibana-ci commented Oct 1, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Public APIs missing comments

Async chunks

API count

History

jughosta left a comment

Choose a reason for hiding this comment

kibanamachine commented Oct 1, 2024

kibanamachine commented Oct 1, 2024

💚 All backports created successfully

Questions ?

stratoula commented Sep 24, 2024 •

edited

Loading

kertal left a comment •

edited

Loading

stratoula commented Sep 30, 2024 •

edited

Loading

stratoula Oct 1, 2024 •

edited

Loading