Skip to content

Commit

Permalink
[Discover] Allow to store the configured ES|QL visualization v3 (#175227
Browse files Browse the repository at this point in the history
)

- Resolves #167887

## Summary

On Discover page user can see a visualization for data view and ES|QL
modes. For ES|QL mode it's also allowed to customize the visualization.
This PR allows to save such customization together with a saved search.

In more details, various types of Lens visualization can be shown on
Discover page:
- If in the default (data view) mode, Unified Histogram shows a
"formBased" histogram (`type:
UnifiedHistogramSuggestionType.histogramForDataView` in this PR)
- If in the ES|QL mode, 2 scenarios are possible (so far only these are
customizable):
- If Lens has suggestions for the current query, Unified Histogram shows
one of them (`type: UnifiedHistogramSuggestionType.lensSuggestion` in
this PR) Example query: `from kibana_sample_data_logs | stats avg(bytes)
by message.keyword`
- If Lens suggestion list is empty, Unified Histogram shows a
"textBased" histogram (`type:
UnifiedHistogramSuggestionType.histogramForESQL` in this PR). Example
query: `from kibana_sample_data_logs | limit 10`
   
The main flow is that Unified Histogram first picks a suggestion (one of
`UnifiedHistogramSuggestionType` type), then calculates lens attributes
which are necessary to build Lens embeddable. With a saved search we are
saving those calculated lens attributes under `savedSearch.visContext`.
For handling this logic, I refactored `useLensSuggestion`,
`getLensAttributes` into `LensVisService`.

Restoring a saved customization adds complexity to the flow as it should
pick now not just any available suggestion but the suggestion which
matches to the previously saved lens attributes.

Changes to the current query, time range, time field etc can make the
current vis context incompatible and we have to drop the vis
customization. This PR already includes this logic of invalidating the
stored lens attributes if they are not compatible any more. New vis
context will override the previous one when user presses Save for the
current search. Until then, we try to restore the customization from the
previously saved vis context (for example when the query changes back to
the compatible one).


What can invalidate the saved vis context and drop the user's
customization:
- data view id
- data view time field name
- query/filters
- time range if it has a different time interval
- text based columns affect what lens suggestions are available

Flow of creating a new search:

![1](https://github.com/elastic/kibana/assets/1415710/9274d895-cedb-454a-9a9d-3b0cf600d801)

Flow of editing a saved search:

![2](https://github.com/elastic/kibana/assets/1415710/086ce4a0-f679-4d96-892b-631bcfee7ee3)





<details>
<summary>Previous details</summary>

- Previous approach #174373
(saving current suggestion instead of lens attributes)
- Previous approach #174783
(saving lens attributes but it's based on existing hooks)

But I was stuck with how to make "Unsaved changes" badge work well when
user tries to revert changes.

For testing in ES|QL mode I use `from kibana_sample_data_logs | limit
10` as query, customize color of a lens histogram, and save it with a
saved search. Next I check 2 cases:
1. edit query limit `from kibana_sample_data_logs | limit 100`, see that
vis customization gets reset which is expected, press "Revert changes"
in the "Unsaved changes" badge => notice that reset did not work
2. edit only histogram color, press "Revert changes" in the "Unsaved
changes" badge => notice that reset did not work

Here are some nuances with the state management I am seeing which
together do not allow to successfully revert unsaved changes:
- For ES|QL histogram lens attributes include a modified query `from
kibana_sample_data_logs | limit 10 | EVAL timestamp=DATE_TRUNC(30
second, @timestamp) | stats results = count(*) by timestamp | rename
timestamp as "@timestamp every 30 second"` which means that not only
changes to the original query but also a different time interval
invalidates the saved lens attributes.
- In ES|QL mode, `query` prop update is delayed for
`UnifiedHistogramContainer` component until Discover finishes the
documents fetch
https://github.com/elastic/kibana/blob/fc2ec957fe78900967da26c80817aea8a0bd2c65/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts#L346
which means that Discover should make a request on revert changes. And
It's not happening for (2) as it does not make sense for Discover to
trigger refetch if only `visContext` changes so we should find another
way. With (1) there is another problem that Discover `visContext` state
gets hijacked by lens attributes invalidation logic (as query is not
sync yet to UnifiedHistogram) before fetch is completed or get [a chance
to be
fired](https://github.com/elastic/kibana/blob/6038f92b1fcaeedf635a0eab68fd9cdadd1103d3/src/plugins/discover/public/application/main/hooks/utils/build_state_subscribe.ts#L51-L54).
I tried delaying `externalVisContext` prop update too (to keep in sync
with `query` update) but it does not help
https://github.com/elastic/kibana/blob/fc2ec957fe78900967da26c80817aea8a0bd2c65/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts#L437
- Unified Histogram should signal to Discover to start a refetch when
current suggestion changes
https://github.com/elastic/kibana/blob/fc2ec957fe78900967da26c80817aea8a0bd2c65/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts#L289
- for some reason this logic is required for "Revert changes" to work as
it triggers the refetch. I would expect Discover on its own to notice
the change in query and refetch data but it does not seem to be the
case.

</details>

<details>
<summary>Other challenges</summary>

- [ ] Since we are starting to save lens attributes inside a saved
search object (similar to how Dashboard saves lens vis by value), we
should integrate Lens migrations into saved search migrations logic. I
made a quick PoC how it could look like here
jughosta@4529711
This showed that adding Lens plugin as a dependency to saved search
plugin causes lots of circular deps in Kibana. To resolve that I am
suggesting to spit saved search plugin into 2 plugins
#174939 - not the best solution
but it seems impossible to split lens plugins instead.
    Updates here: 
- [x] revert the code regarding migrations and saved search plugin split
- [x] create a github issue to handle client side migrations once their
API is available #179151
- [x] Discover syncs app state with URL which means that the new
`visContext` (large lens attributes object) ends up in the URL too. We
should exclude `visContext` from URL sync as it can make the URL too
long.
    Updates here:  we are not using appState for this any more
- [x] Changes from #171081 would
need to be refactored and integrated into the new `LensVisService`.
- [x]  Refactor after #177790
- [x] Handle a case when no chart is available for current ES|QL query
- [ ] For ES|QL histogram lens attributes include a modified query `from
kibana_sample_data_logs | limit 10 | EVAL timestamp=DATE_TRUNC(30
second, @timestamp) | stats results = count(*) by timestamp | rename
timestamp as "@timestamp every 30 second"` which means that not only
changes to the original query but also a different time range can reset
the customization of lens vis as it gets a different time interval based
on current time range
- New update from Stratoula: 
- [ ] would it help to persist response of `onApplyCb` instead of lens
attributes? <= the shape does not seem to be different and it works as
it is so I'm keeping lens attributes
- [x] use new `getLensAttributes` from
#174677

</details>

10x flaky test
https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5578

### Checklist

- [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] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Matthias Wilhelm <matthias.wilhelm@elastic.co>
Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
  • Loading branch information
4 people authored Apr 8, 2024
1 parent 8f88fc8 commit c708c49
Show file tree
Hide file tree
Showing 77 changed files with 3,452 additions and 1,111 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ describe('getLensAttributesFromSuggestion', () => {
timeFieldName: '@timestamp',
isPersisted: () => false,
toSpec: () => ({}),
toMinimalSpec: () => ({}),
} as unknown as DataView;
const query: AggregateQuery = { esql: 'from foo | limit 10' };

Expand Down
24 changes: 17 additions & 7 deletions packages/kbn-visualization-utils/src/get_lens_attributes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,17 @@ export const getLensAttributesFromSuggestion = ({
query: Query | AggregateQuery;
suggestion: Suggestion | undefined;
dataView?: DataView;
}) => {
}): {
references: Array<{ name: string; id: string; type: string }>;
visualizationType: string;
state: {
visualization: {};
datasourceStates: Record<string, unknown>;
query: Query | AggregateQuery;
filters: Filter[];
};
title: string;
} => {
const suggestionDatasourceState = Object.assign({}, suggestion?.datasourceState);
const suggestionVisualizationState = Object.assign({}, suggestion?.visualizationState);
const datasourceStates =
Expand All @@ -35,11 +45,11 @@ export const getLensAttributesFromSuggestion = ({
};
const visualization = suggestionVisualizationState;
const attributes = {
title: suggestion
? suggestion.title
: i18n.translate('visualizationUtils.config.suggestion.title', {
defaultMessage: 'New suggestion',
}),
title:
suggestion?.title ??
i18n.translate('visualizationUtils.config.suggestion.title', {
defaultMessage: 'New suggestion',
}),
references: [
{
id: dataView?.id ?? '',
Expand All @@ -55,7 +65,7 @@ export const getLensAttributesFromSuggestion = ({
...(dataView &&
dataView.id &&
!dataView.isPersisted() && {
adHocDataViews: { [dataView.id]: dataView.toSpec(false) },
adHocDataViews: { [dataView.id]: dataView.toMinimalSpec() },
}),
},
visualizationType: suggestion ? suggestion.visualizationId : 'lnsXY',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ describe('checking migration metadata changes on all registered SO types', () =>
"risk-engine-configuration": "aea0c371a462e6d07c3ceb3aff11891b47feb09d",
"rules-settings": "892a2918ebaeba809a612b8d97cec0b07c800b5f",
"sample-data-telemetry": "37441b12f5b0159c2d6d5138a494c9f440e950b5",
"search": "cf69e2bf8ae25c10af21887cd6effc4a9ea73064",
"search": "7598e4a701ddcaa5e3f44f22e797618a48595e6f",
"search-session": "b2fcd840e12a45039ada50b1355faeafa39876d1",
"search-telemetry": "b568601618744720b5662946d3103e3fb75fe8ee",
"security-rule": "07abb4d7e707d91675ec0495c73816394c7b521f",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import React, { useCallback, useMemo } from 'react';
import { UnifiedHistogramContainer } from '@kbn/unified-histogram-plugin/public';
import { css } from '@emotion/react';
import useObservable from 'react-use/lib/useObservable';
import { Datatable } from '@kbn/expressions-plugin/common';
import type { Datatable } from '@kbn/expressions-plugin/common';
import { useDiscoverHistogram } from './use_discover_histogram';
import { type DiscoverMainContentProps, DiscoverMainContent } from './discover_main_content';
import { useAppStateSelector } from '../../services/discover_app_state_container';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,15 @@

import { useQuerySubscriber } from '@kbn/unified-field-list/src/hooks/use_query_subscriber';
import {
canImportVisContext,
UnifiedHistogramApi,
UnifiedHistogramExternalVisContextStatus,
UnifiedHistogramFetchStatus,
UnifiedHistogramState,
UnifiedHistogramVisContext,
} from '@kbn/unified-histogram-plugin/public';
import { isEqual } from 'lodash';
import { useCallback, useEffect, useRef, useMemo, useState } from 'react';
import { useCallback, useEffect, useMemo, useRef, useState } from 'react';
import {
debounceTime,
distinctUntilChanged,
Expand All @@ -26,6 +29,9 @@ import {
} from 'rxjs';
import useObservable from 'react-use/lib/useObservable';
import type { RequestAdapter } from '@kbn/inspector-plugin/common';
import type { DatatableColumn } from '@kbn/expressions-plugin/common';
import type { SavedSearch } from '@kbn/saved-search-plugin/common';
import type { Filter } from '@kbn/es-query';
import { useDiscoverCustomization } from '../../../../customizations';
import { useDiscoverServices } from '../../../../hooks/use_discover_services';
import { FetchStatus } from '../../../types';
Expand All @@ -35,7 +41,11 @@ import type { DiscoverStateContainer } from '../../services/discover_state';
import { addLog } from '../../../../utils/add_log';
import { useInternalStateSelector } from '../../services/discover_internal_state_container';
import type { DiscoverAppState } from '../../services/discover_app_state_container';
import { RecordRawType } from '../../services/discover_data_state_container';
import { DataDocumentsMsg, RecordRawType } from '../../services/discover_data_state_container';
import { useSavedSearch } from '../../services/discover_state_provider';

const EMPTY_TEXT_BASED_COLUMNS: DatatableColumn[] = [];
const EMPTY_FILTERS: Filter[] = [];

export interface UseDiscoverHistogramProps {
stateContainer: DiscoverStateContainer;
Expand All @@ -52,6 +62,7 @@ export const useDiscoverHistogram = ({
}: UseDiscoverHistogramProps) => {
const services = useDiscoverServices();
const savedSearchData$ = stateContainer.dataState.data$;
const savedSearchState = useSavedSearch();

/**
* API initialization
Expand Down Expand Up @@ -219,15 +230,18 @@ export const useDiscoverHistogram = ({
[stateContainer]
);

const [initialTextBasedProps] = useState(() =>
getUnifiedHistogramPropsForTextBased({
documentsValue: savedSearchData$.documents$.getValue(),
savedSearch: stateContainer.savedSearchState.getState(),
})
);

const {
dataView: textBasedDataView,
query: textBasedQuery,
columns,
} = useObservable(textBasedFetchComplete$, {
dataView: stateContainer.internalState.getState().dataView!,
query: stateContainer.appState.getState().query,
columns: savedSearchData$.documents$.getValue().textBasedQueryColumns ?? [],
});
columns: textBasedColumns,
} = useObservable(textBasedFetchComplete$, initialTextBasedProps);

useEffect(() => {
if (!isPlainRecord) {
Expand Down Expand Up @@ -316,14 +330,53 @@ export const useDiscoverHistogram = ({

const histogramCustomization = useDiscoverCustomization('unified_histogram');

const filtersMemoized = useMemo(
() => [...(filters ?? []), ...customFilters],
[filters, customFilters]
);
const filtersMemoized = useMemo(() => {
const allFilters = [...(filters ?? []), ...customFilters];
return allFilters.length ? allFilters : EMPTY_FILTERS;
}, [filters, customFilters]);

// eslint-disable-next-line react-hooks/exhaustive-deps
const timeRangeMemoized = useMemo(() => timeRange, [timeRange?.from, timeRange?.to]);

const onVisContextChanged = useCallback(
(
nextVisContext: UnifiedHistogramVisContext | undefined,
externalVisContextStatus: UnifiedHistogramExternalVisContextStatus
) => {
switch (externalVisContextStatus) {
case UnifiedHistogramExternalVisContextStatus.manuallyCustomized:
// if user customized the visualization manually
// (only this action should trigger Unsaved changes badge)
stateContainer.savedSearchState.updateVisContext({
nextVisContext,
});
stateContainer.internalState.transitions.setOverriddenVisContextAfterInvalidation(
undefined
);
break;
case UnifiedHistogramExternalVisContextStatus.automaticallyOverridden:
// if the visualization was invalidated as incompatible and rebuilt
// (it will be used later for saving the visualization via Save button)
stateContainer.internalState.transitions.setOverriddenVisContextAfterInvalidation(
nextVisContext
);
break;
case UnifiedHistogramExternalVisContextStatus.automaticallyCreated:
case UnifiedHistogramExternalVisContextStatus.applied:
// clearing the value in the internal state so we don't use it during saved search saving
stateContainer.internalState.transitions.setOverriddenVisContextAfterInvalidation(
undefined
);
break;
case UnifiedHistogramExternalVisContextStatus.unknown:
// using `{}` to overwrite the value inside the saved search SO during saving
stateContainer.internalState.transitions.setOverriddenVisContextAfterInvalidation({});
break;
}
},
[stateContainer]
);

return {
ref,
getCreationOptions,
Expand All @@ -333,12 +386,18 @@ export const useDiscoverHistogram = ({
filters: filtersMemoized,
timeRange: timeRangeMemoized,
relativeTimeRange,
columns,
columns: isPlainRecord ? textBasedColumns : undefined,
onFilter: histogramCustomization?.onFilter,
onBrushEnd: histogramCustomization?.onBrushEnd,
withDefaultActions: histogramCustomization?.withDefaultActions,
disabledActions: histogramCustomization?.disabledActions,
isChartLoading: isSuggestionLoading,
// visContext should be in sync with current query
externalVisContext:
isPlainRecord && canImportVisContext(savedSearchState?.visContext)
? savedSearchState?.visContext
: undefined,
onVisContextChanged: isPlainRecord ? onVisContextChanged : undefined,
};
};

Expand Down Expand Up @@ -412,12 +471,13 @@ const createAppStateObservable = (state$: Observable<DiscoverAppState>) => {
const createFetchCompleteObservable = (stateContainer: DiscoverStateContainer) => {
return stateContainer.dataState.data$.documents$.pipe(
distinctUntilChanged((prev, curr) => prev.fetchStatus === curr.fetchStatus),
filter(({ fetchStatus }) => fetchStatus === FetchStatus.COMPLETE),
map(({ textBasedQueryColumns }) => ({
dataView: stateContainer.internalState.getState().dataView!,
query: stateContainer.appState.getState().query!,
columns: textBasedQueryColumns ?? [],
}))
filter(({ fetchStatus }) => [FetchStatus.COMPLETE, FetchStatus.ERROR].includes(fetchStatus)),
map((documentsValue) => {
return getUnifiedHistogramPropsForTextBased({
documentsValue,
savedSearch: stateContainer.savedSearchState.getState(),
});
})
);
};

Expand All @@ -430,7 +490,27 @@ const createTotalHitsObservable = (state$?: Observable<UnifiedHistogramState>) =

const createCurrentSuggestionObservable = (state$: Observable<UnifiedHistogramState>) => {
return state$.pipe(
map((state) => state.currentSuggestion),
map((state) => state.currentSuggestionContext),
distinctUntilChanged(isEqual)
);
};

function getUnifiedHistogramPropsForTextBased({
documentsValue,
savedSearch,
}: {
documentsValue: DataDocumentsMsg | undefined;
savedSearch: SavedSearch;
}) {
const columns = documentsValue?.textBasedQueryColumns || EMPTY_TEXT_BASED_COLUMNS;

const nextProps = {
dataView: savedSearch.searchSource.getField('index')!,
query: savedSearch.searchSource.getField('query'),
columns,
};

addLog('[UnifiedHistogram] delayed next props for text-based', nextProps);

return nextProps;
}
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,13 @@ export const getTopNavBadges = ({
if (hasUnsavedChanges && !defaultBadges?.unsavedChangesBadge?.disabled) {
entries.push({
data: getTopNavUnsavedChangesBadge({
onRevert: stateContainer.actions.undoSavedSearchChanges,
onRevert: async () => {
const lensEditFlyoutCancelButton = document.getElementById('lnsCancelEditOnFlyFlyout');
if (lensEditFlyoutCancelButton) {
lensEditFlyoutCancelButton.click?.();
}
await stateContainer.actions.undoSavedSearchChanges();
},
onSave:
services.capabilities.discover.save && !isManaged
? async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,9 @@ export async function onSaveSearch({
}) {
const { uiSettings, savedObjectsTagging } = services;
const dataView = state.internalState.getState().dataView;
const overriddenVisContextAfterInvalidation =
state.internalState.getState().overriddenVisContextAfterInvalidation;

const onSave = async ({
newTitle,
newCopyOnSave,
Expand All @@ -116,6 +119,7 @@ export async function onSaveSearch({
const currentSampleSize = savedSearch.sampleSize;
const currentDescription = savedSearch.description;
const currentTags = savedSearch.tags;
const currentVisContext = savedSearch.visContext;
savedSearch.title = newTitle;
savedSearch.description = newDescription;
savedSearch.timeRestore = newTimeRestore;
Expand All @@ -134,6 +138,11 @@ export async function onSaveSearch({
if (savedObjectsTagging) {
savedSearch.tags = newTags;
}

if (overriddenVisContextAfterInvalidation) {
savedSearch.visContext = overriddenVisContextAfterInvalidation;
}

const saveOptions: SaveSavedSearchOptions = {
onTitleDuplicate,
copyOnSave: newCopyOnSave,
Expand All @@ -159,10 +168,12 @@ export async function onSaveSearch({
savedSearch.rowsPerPage = currentRowsPerPage;
savedSearch.sampleSize = currentSampleSize;
savedSearch.description = currentDescription;
savedSearch.visContext = currentVisContext;
if (savedObjectsTagging) {
savedSearch.tags = currentTags;
}
} else {
state.internalState.transitions.resetOnSavedSearchChange();
state.appState.resetInitialState();
}
onSaveCb?.();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@ import {
createStateContainerReactHelpers,
ReduxLikeStateContainer,
} from '@kbn/kibana-utils-plugin/common';
import { DataView, DataViewListItem } from '@kbn/data-views-plugin/common';
import { Filter } from '@kbn/es-query';
import type { DataView, DataViewListItem } from '@kbn/data-views-plugin/common';
import type { Filter } from '@kbn/es-query';
import type { DataTableRecord } from '@kbn/discover-utils/types';
import type { UnifiedHistogramVisContext } from '@kbn/unified-histogram-plugin/public';

export interface InternalState {
dataView: DataView | undefined;
Expand All @@ -22,6 +23,7 @@ export interface InternalState {
adHocDataViews: DataView[];
expandedDoc: DataTableRecord | undefined;
customFilters: Filter[];
overriddenVisContextAfterInvalidation: UnifiedHistogramVisContext | {} | undefined; // it will be used during saved search saving
}

export interface InternalStateTransitions {
Expand All @@ -40,6 +42,12 @@ export interface InternalStateTransitions {
state: InternalState
) => (dataView: DataTableRecord | undefined) => InternalState;
setCustomFilters: (state: InternalState) => (customFilters: Filter[]) => InternalState;
setOverriddenVisContextAfterInvalidation: (
state: InternalState
) => (
overriddenVisContextAfterInvalidation: UnifiedHistogramVisContext | {} | undefined
) => InternalState;
resetOnSavedSearchChange: (state: InternalState) => () => InternalState;
}

export type DiscoverInternalStateContainer = ReduxLikeStateContainer<
Expand All @@ -59,6 +67,7 @@ export function getInternalStateContainer() {
savedDataViews: [],
expandedDoc: undefined,
customFilters: [],
overriddenVisContextAfterInvalidation: undefined,
},
{
setDataView: (prevState: InternalState) => (nextDataView: DataView) => ({
Expand Down Expand Up @@ -112,6 +121,16 @@ export function getInternalStateContainer() {
...prevState,
customFilters,
}),
setOverriddenVisContextAfterInvalidation:
(prevState: InternalState) =>
(overriddenVisContextAfterInvalidation: UnifiedHistogramVisContext | {} | undefined) => ({
...prevState,
overriddenVisContextAfterInvalidation,
}),
resetOnSavedSearchChange: (prevState: InternalState) => () => ({
...prevState,
overriddenVisContextAfterInvalidation: undefined,
}),
},
{},
{ freeze: (state) => state }
Expand Down
Loading

0 comments on commit c708c49

Please sign in to comment.