Skip to content

Commit

Permalink
[8.x] [Discover] Address chart performance issues for non-transformat…
Browse files Browse the repository at this point in the history
…ional and non-time-based ES|QL queries (#200583) (#201185)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Discover] Address chart performance issues for non-transformational
and non-time-based ES|QL queries
(#200583)](#200583)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Julia
Rechkunova","email":"julia.rechkunova@elastic.co"},"sourceCommit":{"committedDate":"2024-11-21T13:10:21Z","message":"[Discover]
Address chart performance issues for non-transformational and
non-time-based ES|QL queries (#200583)\n\n- Closes
https://github.com/elastic/kibana/issues/199608\r\n\r\n##
Summary\r\n\r\nThis PR changes the logic around when suggestions from
lens API are\r\nused. Previously for non-transformational query and
non-time-based data\r\nit would try to render one of lens suggestions
supplying chart data via\r\n`table` prop. Now, it would not render any
chart.\r\n\r\nBefore:\r\n- Data view mode => Static histogram
configuration\r\n- ES|QL mode and non-transformational query => _**Gets
lens\r\nsuggestions.**_ If histogram chart is not possible, **_takes the
first\r\nlens suggestion for rendering the chart_**\r\n- ES|QL mode and
transformational query => Gets lens suggestions. Takes\r\nthe first lens
suggestion for rendering the chart.\r\n\r\nAfter:\r\n- Data view mode =>
Static histogram configuration (same)\r\n- ES|QL mode and
non-transformational query => ~~_**Gets lens\r\nsuggestions.**_~~ If
histogram chart is not possible, **_renders\r\nnothing_** (updated)\r\n-
ES|QL mode and transformational query => Gets lens suggestions.
Takes\r\nthe first lens suggestion for rendering the chart.
(same)\r\n\r\n### Testing\r\n\r\nAs per originally reported case:\r\n1.
`node
scripts/es_archiver\r\n--kibana-url=http://elastic:changeme@localhost:5601\r\n--es-url=http://elastic:changeme@localhost:9200
load\r\ntest/functional/fixtures/es_archiver/many_fields`\r\n\r\n2.
Navigate to Discover, switch to ES|QL mode and enter
`FROM\r\nindices-stats | LIMIT 10`\r\n3. No chart is
expected.\r\n\r\nAlso there should be no regression
for\r\nhttps://github.com//pull/195863\r\n\r\n###
Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Davis McPhee
<davis.mcphee@elastic.co>\r\nCo-authored-by: Stratoula Kalafateli
<efstratia.kalafateli@elastic.co>","sha":"b9439e658d5cca8dd89f7b3d7cc399c72e4e5103","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","v9.0.0","Team:DataDiscovery","Feature:ES|QL","backport:version","v8.17.0","v8.18.0"],"title":"[Discover]
Address chart performance issues for non-transformational and
non-time-based ES|QL
queries","number":200583,"url":"https://github.com/elastic/kibana/pull/200583","mergeCommit":{"message":"[Discover]
Address chart performance issues for non-transformational and
non-time-based ES|QL queries (#200583)\n\n- Closes
https://github.com/elastic/kibana/issues/199608\r\n\r\n##
Summary\r\n\r\nThis PR changes the logic around when suggestions from
lens API are\r\nused. Previously for non-transformational query and
non-time-based data\r\nit would try to render one of lens suggestions
supplying chart data via\r\n`table` prop. Now, it would not render any
chart.\r\n\r\nBefore:\r\n- Data view mode => Static histogram
configuration\r\n- ES|QL mode and non-transformational query => _**Gets
lens\r\nsuggestions.**_ If histogram chart is not possible, **_takes the
first\r\nlens suggestion for rendering the chart_**\r\n- ES|QL mode and
transformational query => Gets lens suggestions. Takes\r\nthe first lens
suggestion for rendering the chart.\r\n\r\nAfter:\r\n- Data view mode =>
Static histogram configuration (same)\r\n- ES|QL mode and
non-transformational query => ~~_**Gets lens\r\nsuggestions.**_~~ If
histogram chart is not possible, **_renders\r\nnothing_** (updated)\r\n-
ES|QL mode and transformational query => Gets lens suggestions.
Takes\r\nthe first lens suggestion for rendering the chart.
(same)\r\n\r\n### Testing\r\n\r\nAs per originally reported case:\r\n1.
`node
scripts/es_archiver\r\n--kibana-url=http://elastic:changeme@localhost:5601\r\n--es-url=http://elastic:changeme@localhost:9200
load\r\ntest/functional/fixtures/es_archiver/many_fields`\r\n\r\n2.
Navigate to Discover, switch to ES|QL mode and enter
`FROM\r\nindices-stats | LIMIT 10`\r\n3. No chart is
expected.\r\n\r\nAlso there should be no regression
for\r\nhttps://github.com//pull/195863\r\n\r\n###
Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Davis McPhee
<davis.mcphee@elastic.co>\r\nCo-authored-by: Stratoula Kalafateli
<efstratia.kalafateli@elastic.co>","sha":"b9439e658d5cca8dd89f7b3d7cc399c72e4e5103"}},"sourceBranch":"main","suggestedTargetBranches":["8.17","8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/200583","number":200583,"mergeCommit":{"message":"[Discover]
Address chart performance issues for non-transformational and
non-time-based ES|QL queries (#200583)\n\n- Closes
https://github.com/elastic/kibana/issues/199608\r\n\r\n##
Summary\r\n\r\nThis PR changes the logic around when suggestions from
lens API are\r\nused. Previously for non-transformational query and
non-time-based data\r\nit would try to render one of lens suggestions
supplying chart data via\r\n`table` prop. Now, it would not render any
chart.\r\n\r\nBefore:\r\n- Data view mode => Static histogram
configuration\r\n- ES|QL mode and non-transformational query => _**Gets
lens\r\nsuggestions.**_ If histogram chart is not possible, **_takes the
first\r\nlens suggestion for rendering the chart_**\r\n- ES|QL mode and
transformational query => Gets lens suggestions. Takes\r\nthe first lens
suggestion for rendering the chart.\r\n\r\nAfter:\r\n- Data view mode =>
Static histogram configuration (same)\r\n- ES|QL mode and
non-transformational query => ~~_**Gets lens\r\nsuggestions.**_~~ If
histogram chart is not possible, **_renders\r\nnothing_** (updated)\r\n-
ES|QL mode and transformational query => Gets lens suggestions.
Takes\r\nthe first lens suggestion for rendering the chart.
(same)\r\n\r\n### Testing\r\n\r\nAs per originally reported case:\r\n1.
`node
scripts/es_archiver\r\n--kibana-url=http://elastic:changeme@localhost:5601\r\n--es-url=http://elastic:changeme@localhost:9200
load\r\ntest/functional/fixtures/es_archiver/many_fields`\r\n\r\n2.
Navigate to Discover, switch to ES|QL mode and enter
`FROM\r\nindices-stats | LIMIT 10`\r\n3. No chart is
expected.\r\n\r\nAlso there should be no regression
for\r\nhttps://github.com//pull/195863\r\n\r\n###
Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Davis McPhee
<davis.mcphee@elastic.co>\r\nCo-authored-by: Stratoula Kalafateli
<efstratia.kalafateli@elastic.co>","sha":"b9439e658d5cca8dd89f7b3d7cc399c72e4e5103"}},{"branch":"8.17","label":"v8.17.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.x","label":"v8.18.0","branchLabelMappingKey":"^v8.18.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Julia Rechkunova <julia.rechkunova@elastic.co>
  • Loading branch information
kibanamachine and jughosta authored Nov 21, 2024
1 parent 06f1b16 commit c832adb
Show file tree
Hide file tree
Showing 7 changed files with 197 additions and 106 deletions.
8 changes: 5 additions & 3 deletions src/plugins/unified_histogram/public/__mocks__/lens_vis.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export const getLensVisMock = async ({
breakdownField,
dataView,
allSuggestions,
hasHistogramSuggestionForESQL,
isTransformationalESQL,
table,
}: {
filters: QueryParams['filters'];
Expand All @@ -44,7 +44,7 @@ export const getLensVisMock = async ({
timeRange?: TimeRange | null;
breakdownField: DataViewField | undefined;
allSuggestions?: Suggestion[];
hasHistogramSuggestionForESQL?: boolean;
isTransformationalESQL?: boolean;
table?: Datatable;
}): Promise<{
lensService: LensVisService;
Expand All @@ -60,7 +60,9 @@ export const getLensVisMock = async ({
if ('query' in context && context.query === query) {
return allSuggestions;
}
return hasHistogramSuggestionForESQL ? [histogramESQLSuggestionMock] : [];
return !isTransformationalESQL && dataView.isTimeBased()
? [histogramESQLSuggestionMock]
: [];
}
: lensApi.suggestions,
});
Expand Down
142 changes: 113 additions & 29 deletions src/plugins/unified_histogram/public/chart/chart.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ async function mountComponent({
isPlainRecord,
hasDashboardPermissions,
isChartLoading,
hasHistogramSuggestionForESQL,
isTransformationalESQL,
}: {
customToggle?: ReactElement;
noChart?: boolean;
Expand All @@ -57,7 +57,7 @@ async function mountComponent({
isPlainRecord?: boolean;
hasDashboardPermissions?: boolean;
isChartLoading?: boolean;
hasHistogramSuggestionForESQL?: boolean;
isTransformationalESQL?: boolean;
} = {}) {
(searchSourceInstanceMock.fetch$ as jest.Mock).mockImplementation(
jest.fn().mockReturnValue(of({ rawResponse: { hits: { total: noHits ? 0 : 2 } } }))
Expand Down Expand Up @@ -87,7 +87,9 @@ async function mountComponent({

const requestParams = {
query: isPlainRecord
? { esql: 'from logs | limit 10' }
? isTransformationalESQL
? { esql: 'from logs | limit 10 | stats var0 = avg(bytes) by extension' }
: { esql: 'from logs | limit 10' }
: {
language: 'kuery',
query: '',
Expand All @@ -108,7 +110,7 @@ async function mountComponent({
breakdownField: undefined,
columns: [],
allSuggestions,
hasHistogramSuggestionForESQL,
isTransformationalESQL,
})
).lensService;

Expand Down Expand Up @@ -211,12 +213,111 @@ describe('Chart', () => {
expect(component.find('[data-test-subj="unifiedHistogramChart"]').exists()).toBeTruthy();
});

test('render when is text based and not timebased', async () => {
const component = await mountComponent({ isPlainRecord: true, dataView: dataViewMock });
test('should render when is text based, transformational and non-time-based', async () => {
const component = await mountComponent({
isPlainRecord: true,
dataView: dataViewMock,
isTransformationalESQL: true,
});
expect(
component.find('[data-test-subj="unifiedHistogramToggleChartButton"]').exists()
).toBeTruthy();
expect(component.find('[data-test-subj="unifiedHistogramChart"]').exists()).toBeTruthy();
expect(
component.find('[data-test-subj="unifiedHistogramEditFlyoutVisualization"]').exists()
).toBeTruthy();
expect(
component.find('[data-test-subj="unifiedHistogramSaveVisualization"]').exists()
).toBeTruthy();
});

test('should not render when is text based, non-transformational and non-time-based', async () => {
const component = await mountComponent({
isPlainRecord: true,
dataView: dataViewMock,
isTransformationalESQL: false,
});
expect(
component.find('[data-test-subj="unifiedHistogramToggleChartButton"]').exists()
).toBeTruthy();
expect(component.find('[data-test-subj="unifiedHistogramChart"]').exists()).toBeFalsy();
expect(
component.find('[data-test-subj="unifiedHistogramEditFlyoutVisualization"]').exists()
).toBeFalsy();
expect(
component.find('[data-test-subj="unifiedHistogramSaveVisualization"]').exists()
).toBeFalsy();
});

test('should not render when is text based, non-transformational, non-time-based and suggestions are available', async () => {
const component = await mountComponent({
allSuggestions: allSuggestionsMock,
isPlainRecord: true,
dataView: dataViewMock,
isTransformationalESQL: false,
});
expect(
component.find('[data-test-subj="unifiedHistogramToggleChartButton"]').exists()
).toBeTruthy();
expect(component.find('[data-test-subj="unifiedHistogramChart"]').exists()).toBeFalsy();
expect(
component.find('[data-test-subj="unifiedHistogramEditFlyoutVisualization"]').exists()
).toBeFalsy();
expect(
component.find('[data-test-subj="unifiedHistogramSaveVisualization"]').exists()
).toBeFalsy();
});

test('should render when is text based, non-transformational and time-based', async () => {
const component = await mountComponent({
isPlainRecord: true,
isTransformationalESQL: false,
});
expect(
component.find('[data-test-subj="unifiedHistogramToggleChartButton"]').exists()
).toBeTruthy();
expect(component.find('[data-test-subj="unifiedHistogramChart"]').exists()).toBeTruthy();
expect(
component.find('[data-test-subj="unifiedHistogramEditFlyoutVisualization"]').exists()
).toBeTruthy();
expect(
component.find('[data-test-subj="unifiedHistogramSaveVisualization"]').exists()
).toBeTruthy();
});

test('should render when is text based, transformational and time-based', async () => {
const component = await mountComponent({
isPlainRecord: true,
isTransformationalESQL: true,
});
expect(
component.find('[data-test-subj="unifiedHistogramToggleChartButton"]').exists()
).toBeTruthy();
expect(component.find('[data-test-subj="unifiedHistogramChart"]').exists()).toBeTruthy();
expect(
component.find('[data-test-subj="unifiedHistogramEditFlyoutVisualization"]').exists()
).toBeTruthy();
expect(
component.find('[data-test-subj="unifiedHistogramSaveVisualization"]').exists()
).toBeTruthy();
});

test('should not render when is text based, transformational and no suggestions available', async () => {
const component = await mountComponent({
allSuggestions: [],
isPlainRecord: true,
isTransformationalESQL: true,
});
expect(
component.find('[data-test-subj="unifiedHistogramToggleChartButton"]').exists()
).toBeTruthy();
expect(component.find('[data-test-subj="unifiedHistogramChart"]').exists()).toBeFalsy();
expect(
component.find('[data-test-subj="unifiedHistogramEditFlyoutVisualization"]').exists()
).toBeFalsy();
expect(
component.find('[data-test-subj="unifiedHistogramSaveVisualization"]').exists()
).toBeFalsy();
});

test('render progress bar when text based and request is loading', async () => {
Expand Down Expand Up @@ -267,42 +368,25 @@ describe('Chart', () => {
expect(component.find(BreakdownFieldSelector).exists()).toBeFalsy();
});

it('should render the edit on the fly button when chart is visible and suggestions exist', async () => {
const component = await mountComponent({
allSuggestions: allSuggestionsMock,
isPlainRecord: true,
});
expect(
component.find('[data-test-subj="unifiedHistogramEditFlyoutVisualization"]').exists()
).toBeTruthy();
});

it('should not render the edit on the fly button when chart is visible and suggestions dont exist', async () => {
it('should not render the save button when text-based and the dashboard save by value permissions are false', async () => {
const component = await mountComponent({
allSuggestions: [],
hasHistogramSuggestionForESQL: false,
isPlainRecord: true,
});
expect(
component.find('[data-test-subj="unifiedHistogramEditFlyoutVisualization"]').exists()
).toBeFalsy();
});

it('should render the save button when chart is visible and suggestions exist', async () => {
const component = await mountComponent({
allSuggestions: allSuggestionsMock,
isTransformationalESQL: false,
isPlainRecord: true,
hasDashboardPermissions: false,
});
expect(component.find('[data-test-subj="unifiedHistogramChart"]').exists()).toBeTruthy();
expect(
component.find('[data-test-subj="unifiedHistogramSaveVisualization"]').exists()
).toBeTruthy();
).toBeFalsy();
});

it('should not render the save button when the dashboard save by value permissions are false', async () => {
const component = await mountComponent({
allSuggestions: allSuggestionsMock,
hasDashboardPermissions: false,
});
expect(component.find('[data-test-subj="unifiedHistogramChart"]').exists()).toBeTruthy();
expect(
component.find('[data-test-subj="unifiedHistogramSaveVisualization"]').exists()
).toBeFalsy();
Expand Down
15 changes: 0 additions & 15 deletions src/plugins/unified_histogram/public/layout/helpers.ts

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -765,7 +765,7 @@ describe('LensVisService attributes', () => {
columns: [],
isPlainRecord: true,
allSuggestions: [], // none available
hasHistogramSuggestionForESQL: true,
isTransformationalESQL: false,
});
expect(lensVis.visContext?.attributes.state.query).toStrictEqual(histogramQuery);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ describe('LensVisService suggestions', () => {
],
isPlainRecord: true,
allSuggestions: [],
hasHistogramSuggestionForESQL: false,
isTransformationalESQL: true,
});

expect(lensVis.currentSuggestionContext?.type).toBe(UnifiedHistogramSuggestionType.unsupported);
Expand Down Expand Up @@ -115,7 +115,7 @@ describe('LensVisService suggestions', () => {
],
isPlainRecord: true,
allSuggestions: [],
hasHistogramSuggestionForESQL: true,
isTransformationalESQL: false,
});

expect(lensVis.currentSuggestionContext?.type).toBe(
Expand Down Expand Up @@ -153,7 +153,7 @@ describe('LensVisService suggestions', () => {
],
isPlainRecord: true,
allSuggestions: [],
hasHistogramSuggestionForESQL: true,
isTransformationalESQL: false,
});

expect(lensVis.currentSuggestionContext?.type).toBe(
Expand Down Expand Up @@ -191,7 +191,7 @@ describe('LensVisService suggestions', () => {
],
isPlainRecord: true,
allSuggestions: [],
hasHistogramSuggestionForESQL: true,
isTransformationalESQL: true,
});

expect(lensVis.currentSuggestionContext?.type).toBe(UnifiedHistogramSuggestionType.unsupported);
Expand Down Expand Up @@ -225,7 +225,7 @@ describe('LensVisService suggestions', () => {
],
isPlainRecord: true,
allSuggestions: [],
hasHistogramSuggestionForESQL: true,
isTransformationalESQL: false,
});

expect(lensVis.currentSuggestionContext?.type).toBe(
Expand Down Expand Up @@ -276,7 +276,7 @@ describe('LensVisService suggestions', () => {
],
isPlainRecord: true,
allSuggestions: allSuggestionsMock,
hasHistogramSuggestionForESQL: true,
isTransformationalESQL: false,
});

expect(lensVis.currentSuggestionContext?.type).toBe(
Expand Down Expand Up @@ -307,7 +307,7 @@ describe('LensVisService suggestions', () => {
],
isPlainRecord: true,
allSuggestions: [],
hasHistogramSuggestionForESQL: true,
isTransformationalESQL: false,
});

expect(lensVis.currentSuggestionContext?.type).toBe(
Expand Down
Loading

0 comments on commit c832adb

Please sign in to comment.