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

Add loading indicator and counter to query result #8212

Merged
merged 5 commits into from
Sep 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelogs/fragments/8212.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
feat:
- Add loading indicator and counter to query result ([#8212](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8212))

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import React from 'react';
import { mountWithIntl, shallowWithIntl } from 'test_utils/enzyme_helpers';
import { QueryResult } from './query_result';

enum ResultStatus {
UNINITIALIZED = 'uninitialized',
LOADING = 'loading', // initial data load
READY = 'ready', // results came back
NO_RESULTS = 'none', // no results came back
ERROR = 'error', // error occurred
}

describe('Query Result', () => {
it('shows loading status', () => {
const props = {
queryStatus: {
status: ResultStatus.LOADING,
startTime: Number.NEGATIVE_INFINITY,
},
};
const component = shallowWithIntl(<QueryResult {...props} />);
expect(component).toMatchSnapshot();
});

it('shows ready status with complete message', () => {
const props = {
queryStatus: {
status: ResultStatus.READY,
startTime: new Date().getTime(),
},
};
const component = mountWithIntl(<QueryResult {...props} />);
const loadingIndicator = component.find(`[data-test-subj="queryResultLoading"]`);
expect(loadingIndicator.exists()).toBeFalsy();
expect(component.find('EuiText').text()).toEqual('Completed');
});

it('shows ready status with complete in miliseconds message', () => {
const props = {
queryStatus: {
status: ResultStatus.READY,
startTime: new Date().getTime(),
elapsedMs: 500,
},
};
const component = mountWithIntl(<QueryResult {...props} />);
expect(component.find('EuiText').text()).toEqual('Completed in 500 ms');
});

it('shows ready status with complete in seconds message', () => {
const props = {
queryStatus: {
status: ResultStatus.READY,
startTime: new Date().getTime(),
elapsedMs: 2000,
},
};
const component = mountWithIntl(<QueryResult {...props} />);
expect(component.find('EuiText').text()).toEqual('Completed in 2.0 s');
});

it('shows ready status with split seconds', () => {
const props = {
queryStatus: {
status: ResultStatus.READY,
startTime: new Date().getTime(),
elapsedMs: 2700,
},
};
const component = mountWithIntl(<QueryResult {...props} />);
expect(component.find('EuiText').text()).toEqual('Completed in 2.7 s');
});

it('show error status with error message', () => {
const props = {
queryStatus: {
status: ResultStatus.ERROR,
body: {
error: {
reason: 'error reason',
details: 'error details',
},
statusCode: 400,
},
},
};
const component = shallowWithIntl(<QueryResult {...props} />);
expect(component.find(`[data-test-subj="queryResultError"]`).text()).toMatchInlineSnapshot(
`"<EuiPopover />"`
);
component.find(`[data-test-subj="queryResultError"]`).simulate('click');
expect(component).toMatchSnapshot();
});

it('returns null when error body is empty', () => {
const props = {
queryStatus: {
status: ResultStatus.ERROR,
},
};
const component = shallowWithIntl(<QueryResult {...props} />);
expect(component).toEqual({});
});
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a unit test for this functionality?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import './_recent_query.scss';
import { EuiButtonEmpty, EuiPopover, EuiText, EuiPopoverTitle } from '@elastic/eui';

import React, { useState } from 'react';
import React, { useEffect, useState } from 'react';

export enum ResultStatus {
UNINITIALIZED = 'uninitialized',
Expand All @@ -28,21 +28,77 @@
statusCode?: number;
};
elapsedMs?: number;
startTime?: number;
}

// This is the time in milliseconds that the query will wait before showing the loading spinner
amsiglan marked this conversation as resolved.
Show resolved Hide resolved
const BUFFER_TIME = 3000;

export function QueryResult(props: { queryStatus: QueryStatus }) {
const [isPopoverOpen, setPopover] = useState(false);
const [elapsedTime, setElapsedTime] = useState(0);
const onButtonClick = () => {
setPopover(!isPopoverOpen);
};

const updateElapsedTime = () => {
const time = Date.now() - (props.queryStatus.startTime || 0);
if (time > BUFFER_TIME) {
setElapsedTime(time);

Check warning on line 47 in src/plugins/data/public/query/query_string/language_service/lib/query_result.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/query/query_string/language_service/lib/query_result.tsx#L47

Added line #L47 was not covered by tests
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the interval below and the logic here, elapsedTime will be
0 for first run
0 for second run
3000 for third run
4000 for fourth run

Is that expected?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think 1s interval is a reasonable time to get elapsedTime here? since @ashwin-pc also requests we show loading time in seconds

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its rounded down to seconds so that should not be a problem. Take a look at the video attached Miki :)

} else {
setElapsedTime(0);

Check warning on line 49 in src/plugins/data/public/query/query_string/language_service/lib/query_result.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/query/query_string/language_service/lib/query_result.tsx#L49

Added line #L49 was not covered by tests
}
};

useEffect(() => {
const interval = setInterval(updateElapsedTime, 1000);

return () => clearInterval(interval);
});

if (props.queryStatus.status === ResultStatus.LOADING) {
if (elapsedTime < BUFFER_TIME) {
return null;
}
const time = Math.floor(elapsedTime / 1000);
return (

Check warning on line 64 in src/plugins/data/public/query/query_string/language_service/lib/query_result.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/query/query_string/language_service/lib/query_result.tsx#L63-L64

Added lines #L63 - L64 were not covered by tests
<EuiButtonEmpty
color="text"
size="xs"
onClick={() => {}}
isLoading
data-test-subj="queryResultLoading"
>
{i18n.translate('data.query.languageService.queryResults.completeTime', {
defaultMessage: `Loading ${time} s`,
})}
</EuiButtonEmpty>
);
}

if (props.queryStatus.status === ResultStatus.READY) {
let message;
if (!props.queryStatus.elapsedMs) {
message = i18n.translate('data.query.languageService.queryResults.completeTime', {
defaultMessage: `Completed`,
});
} else if (props.queryStatus.elapsedMs < 1000) {
message = i18n.translate(
'data.query.languageService.queryResults.completeTimeInMiliseconds',
{
defaultMessage: `Completed in ${props.queryStatus.elapsedMs} ms`,
}
);
} else {
message = i18n.translate('data.query.languageService.queryResults.completeTimeInSeconds', {
defaultMessage: `Completed in ${(props.queryStatus.elapsedMs / 1000).toFixed(1)} s`,
});
}

return (
<EuiButtonEmpty iconSide="left" iconType={'checkInCircleEmpty'} size="xs" onClick={() => {}}>
<EuiText size="xs" color="subdued">
{props.queryStatus.elapsedMs
? `Completed in ${props.queryStatus.elapsedMs} ms`
: 'Completed'}
<EuiText size="xs" color="subdued" data-test-subj="queryResultCompleteMsg">
{message}
</EuiText>
</EuiButtonEmpty>
);
Expand All @@ -55,16 +111,25 @@
return (
<EuiPopover
button={
<EuiButtonEmpty iconSide="left" iconType={'alert'} size="xs" onClick={onButtonClick}>
<EuiButtonEmpty
iconSide="left"
iconType={'alert'}
size="xs"
onClick={onButtonClick}
data-test-subj="queryResultErrorBtn"
>
<EuiText size="xs" color="subdued">
{'Error'}
{i18n.translate('data.query.languageService.queryResults.error', {
defaultMessage: `Error`,
})}
</EuiText>
</EuiButtonEmpty>
}
isOpen={isPopoverOpen}
closePopover={() => setPopover(false)}
panelPaddingSize="s"
anchorPosition={'downRight'}
data-test-subj="queryResultError"
>
<EuiPopoverTitle>ERRORS</EuiPopoverTitle>
abbyhu2000 marked this conversation as resolved.
Show resolved Hide resolved
<div style={{ width: '250px' }}>
Expand All @@ -82,7 +147,7 @@
{i18n.translate('data.query.languageService.queryResults.details', {
defaultMessage: `Details:`,
})}
</strong>{' '}
</strong>
{props.queryStatus.body.error.details}
</p>
</EuiText>
Expand Down
1 change: 0 additions & 1 deletion src/plugins/data/public/ui/query_editor/query_editor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import {
EuiText,
PopoverAnchorPosition,
} from '@elastic/eui';
import { BehaviorSubject } from 'rxjs';
import classNames from 'classnames';
import { isEqual } from 'lodash';
import React, { Component, createRef, RefObject } from 'react';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
statusCode?: number;
};
elapsedMs?: number;
startTime?: number;
};
}

Expand Down Expand Up @@ -119,12 +120,14 @@
);
}, [savedSearch, services.uiSettings, timefilter]);

const startTime = Date.now();

Check warning on line 123 in src/plugins/discover/public/application/view_components/utils/use_search.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/discover/public/application/view_components/utils/use_search.ts#L123

Added line #L123 was not covered by tests
const data$ = useMemo(
() =>
new BehaviorSubject<SearchData>({
status: shouldSearchOnPageLoad() ? ResultStatus.LOADING : ResultStatus.UNINITIALIZED,
queryStatus: { startTime },
}),
[shouldSearchOnPageLoad]
[shouldSearchOnPageLoad, startTime]
);
const refetch$ = useMemo(() => new Subject<SearchRefetch>(), []);

Expand Down Expand Up @@ -161,7 +164,6 @@
dataset = searchSource.getField('index');

let elapsedMs;

try {
// Only show loading indicator if we are fetching when the rows are empty
if (fetchStateRef.current.rows?.length === 0) {
abbyhu2000 marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -267,14 +269,14 @@
}
}, [
indexPattern,
interval,
timefilter,
toastNotifications,
interval,
data,
services,
sort,
savedSearch?.searchSource,
data$,
sort,
shouldSearchOnPageLoad,
inspectorAdapters.requests,
]);
Expand Down
Loading