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

[Discover] Field List - refactor sections to align with Lens implementation #135678

Closed
8 of 9 tasks
kertal opened this issue Jul 5, 2022 · 4 comments · Fixed by #144412
Closed
8 of 9 tasks

[Discover] Field List - refactor sections to align with Lens implementation #135678

kertal opened this issue Jul 5, 2022 · 4 comments · Fixed by #144412
Assignees
Labels
8.7 candidate Feature:Discover Discover Application impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL.

Comments

@kertal
Copy link
Member

kertal commented Jul 5, 2022

Part 1 #142758

  • Extract Lens components to UnifiedFieldList plugin
  • Create a new hook for fetching data
  • Integrate these components back to Lens page
  • Refactor field list implementation for text based queries in Lens (separate implementation with classes)

Part 2:

More context

Currently in Discover, we calculate the available fields by the data returned by Elasticsearch.

Bildschirmfoto 2022-08-08 um 10 45 39

To show all fields users need to change the Hide empty fields filter

Bildschirmfoto 2022-08-08 um 10 49 52

Here's where the field statistics are calculated:

useEffect(() => {
const subscription = props.documents$.subscribe((next) => {
if (next.fetchStatus !== documentState.fetchStatus) {
if (next.result) {
fieldCounts.current = calcFieldCounts(next.result, selectedDataView!);
}
setDocumentState({ ...documentState, ...next });
}
});
return () => subscription.unsubscribe();
}, [props.documents$, selectedDataView, documentState, setDocumentState]);

Here's where available fields + selected fields are sent to the field statistics by observable (Note: sending selected fields should not be necessary, since it's in our state, to send the available fields by observable is something we could reconsider, POC PR: #138238)

useEffect(
() => {
// For an external embeddable like the Field stats
// it is useful to know what fields are populated in the docs fetched
// or what fields are selected by the user
const fieldCnts = fieldCounts.current ?? {};
const availableFields = props.columns.length > 0 ? props.columns : Object.keys(fieldCnts);
availableFields$.next({
fetchStatus: FetchStatus.COMPLETE,
fields: availableFields,
});
},

Here's where the grouping of the fields into (selected, popular, unpopular 😢 ) takes place:

const {
selected: selectedFields,
popular: popularFields,
unpopular: unpopularFields,
} = useMemo(
() => groupFields(fields, columns, popularLimit, fieldCounts, fieldFilter, useNewFieldsApi),
[fields, columns, popularLimit, fieldCounts, fieldFilter, useNewFieldsApi]
);

This should be rewritten using in the way Lens works, which requests those fields from Elasticsearch, with the addition, that it should also work with virtual fields for our text based query initiative. With the merge of #137031 the function for getting this available fields will be migrated from server -> public.

Here's how it currently works in Lens:

syncExistingFields({
dateRange,
setState,
isFirstExistenceFetch: state.isFirstExistenceFetch,
currentIndexPatternTitle: indexPatterns[currentIndexPatternId]?.title || '',
showNoDataPopover,
indexPatterns: indexPatternList,
fetchJson: core.http.post,
dslQuery,

@botelastic botelastic bot added the needs-team Issues missing a team label label Jul 5, 2022
@kertal kertal added the Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. label Jul 5, 2022
@elasticmachine
Copy link
Contributor

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

@botelastic botelastic bot removed the needs-team Issues missing a team label label Jul 5, 2022
@kertal kertal added needs-team Issues missing a team label 8.5 candidate labels Jul 5, 2022
@botelastic botelastic bot removed the needs-team Issues missing a team label label Jul 5, 2022
@kertal kertal added the Feature:Discover Discover Application label Jul 5, 2022
@kertal kertal changed the title Fieldlist - Refactor available list [Discover] Fieldlist - Refactor available list Jul 5, 2022
@jughosta
Copy link
Contributor

jughosta commented Sep 6, 2022

Dima is working on API refactoring #139453

@jughosta jughosta removed their assignment Sep 9, 2022
@jughosta
Copy link
Contributor

jughosta commented Sep 9, 2022

Reposting the note from the meta issue here:

Currently we use the sidebar to submit existing fields to field statistics this should be improved 2 approaches: centralize field existence to the saved search data fetching OR decentralize & decouple (e.h. using the same hook)

useEffect(
() => {
// For an external embeddable like the Field stats
// it is useful to know what fields are populated in the docs fetched
// or what fields are selected by the user
const fieldCnts = fieldCounts.current ?? {};
const availableFields = props.columns.length > 0 ? props.columns : Object.keys(fieldCnts);
availableFields$.next({
fetchStatus: FetchStatus.COMPLETE,
fields: availableFields,
});
},
// Using columns.length here instead of columns to avoid array reference changing
// eslint-disable-next-line react-hooks/exhaustive-deps
[
selectedDataView,
availableFields$,
fieldCounts.current,
documentState.result,
props.columns.length,
]
);

@kertal
Copy link
Member Author

kertal commented Sep 9, 2022

@jughosta thx, I think one of the next steps could be, create a hook in Discover that could fetch the existing fields, and use it in field statistics, remove the logic of the observable that we populate in the sidebar to use in the Field Statistic tab. feels this would be a good start for the field statistics work later on (that we should discuss separately)

@ninoslavmiskovic ninoslavmiskovic added the impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. label Sep 29, 2022
@jughosta jughosta changed the title [Discover] Fieldlist - Refactor available list [Discover] Field list - refactor sections to align with Lens implementation Oct 5, 2022
@jughosta jughosta changed the title [Discover] Field list - refactor sections to align with Lens implementation [Discover] Field List - refactor sections to align with Lens implementation Oct 5, 2022
jughosta added a commit that referenced this issue Dec 1, 2022
…to Discover (#144412)

Closes #135678

## Summary

This PR continues the work started in
#142758 to bring field list
grouping from Lens into Discover.

- [x] Integrate new components and hooks into Discover page
- [x] Refactor fields grouping logic
- [x] Render Popular fields under a new separate section
- [x] Remove "Hide empty fields" switch
- [x] Adjust filtering logic
- [x] Refactor fields existence logic in Discover
- [x] Add "Unmapped fields" section
- [x] Highlight the matching term when searching for a field 
- [x] Show field icons when in SQL mode
- [x] Add tooltips to field list section headings
- [x] Add tests, clean up

<img width="340" alt="Screenshot 2022-11-15 at 15 39 27"
src="https://user-images.githubusercontent.com/1415710/201947349-726ffc3a-a17f-411b-be92-81d97879765a.png">

For testing on Discover page:
Please check different use cases and toggling Advanced Settings:
- regular vs ad-hoc data views
- data views with and without a time field
- data views with unmapped and empty fields
- data views with a lot of fields
- data views with some fields being filtered out via data view
configuration
- updating query, filters, and time range
- regular and SQL mode
- searching by a field name in the sidebar
- applying a field filter in the sidebar
- adding, editing, and removing a field
- Field Statistics table when some columns are selected or no columns
are selected
- multifields in the field popover should work as before (icon should
change from "+" to "x" when subfield is selected as a column)
- `discover:searchOnPageLoad` should not show fields if turned off
- `discover:searchFieldsFromSource` should show multifields right in the
fields list if enabled
- `discover:enableSql` should show Selected and Available fields only
when enabled
- `discover:showLegacyFieldTopValues` should show old (green) field
stats in its popover
- `doc_table:legacy`

On Lens page:
- scroll position should reset when data view is switched or when
searching by a field name
- regular and SQL mode

### 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] 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: Michael Marcialis <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Stratoula Kalafateli <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.7 candidate Feature:Discover Discover Application impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants