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 setting to turn extending numeric precision on or off #8837

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
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/8837.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
feat:
- Add setting to turn extending numeric precision on or off ([#8837](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8837))
2 changes: 1 addition & 1 deletion src/plugins/console/opensearch_dashboards.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"version": "opensearchDashboards",
"server": true,
"ui": true,
"requiredPlugins": ["devTools"],
"requiredPlugins": ["devTools", "data"],
"optionalPlugins": ["usageCollection", "home", "dataSource"],
"requiredBundles": [
"opensearchUiShared",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ const dummyArgs: OpenSearchRequestArgs = {
};

describe('test sendRequestToOpenSearch', () => {
beforeEach(() => {
jest.clearAllMocks();
});

it('test request success, json', () => {
const mockHttpResponse = createMockHttpResponse(
200,
Expand All @@ -47,9 +51,9 @@ describe('test sendRequestToOpenSearch', () => {
});
});

it('test request success, json with long numerals', () => {
const longPositive = BigInt(Number.MAX_SAFE_INTEGER) * 2n;
const longNegative = BigInt(Number.MIN_SAFE_INTEGER) * 2n;
it('test request success, json with long numerals when precision enabled', () => {
const longPositive = BigInt(Number.MAX_SAFE_INTEGER) * 2n + 1n;
const longNegative = BigInt(Number.MIN_SAFE_INTEGER) * 2n + 1n;
const mockHttpResponse = createMockHttpResponse(
200,
'ok',
Expand All @@ -60,14 +64,59 @@ describe('test sendRequestToOpenSearch', () => {
}
);

jest.spyOn(opensearch, 'send').mockResolvedValue(mockHttpResponse);
sendRequestToOpenSearch(dummyArgs).then((result) => {
const send = jest.spyOn(opensearch, 'send');
send.mockResolvedValue(mockHttpResponse);
sendRequestToOpenSearch({
...dummyArgs,
withLongNumeralsSupport: true,
}).then((result) => {
expect(send).toHaveBeenCalledWith(
expect.anything(),
dummyArgs.requests[0].method,
dummyArgs.requests[0].url,
dummyArgs.requests[0].data.join('\n') + '\n',
undefined,
true
);
const value = (result as any)[0].response.value;
expect(value).toMatch(new RegExp(`"long-max": ${longPositive}[,\n]`));
expect(value).toMatch(new RegExp(`"long-min": ${longNegative}[,\n]`));
});
});

it('test request success, json with long numerals when precision disabled', () => {
const longPositive = BigInt(Number.MAX_SAFE_INTEGER) * 2n + 1n;
const longNegative = BigInt(Number.MIN_SAFE_INTEGER) * 2n + 1n;
const mockHttpResponse = createMockHttpResponse(
200,
'ok',
[['Content-Type', 'application/json, utf-8']],
{
'long-max': Number(longPositive),
'long-min': Number(longNegative),
}
);

const send = jest.spyOn(opensearch, 'send');
send.mockResolvedValue(mockHttpResponse);
sendRequestToOpenSearch({
...dummyArgs,
withLongNumeralsSupport: false,
}).then((result) => {
expect(send).toHaveBeenCalledWith(
expect.anything(),
dummyArgs.requests[0].method,
dummyArgs.requests[0].url,
dummyArgs.requests[0].data.join('\n') + '\n',
undefined,
false
);
const value = (result as any)[0].response.value;
expect(value).toMatch(new RegExp(`"long-max": ${Number(longPositive)}[,\n]`));
expect(value).toMatch(new RegExp(`"long-min": ${Number(longNegative)}[,\n]`));
});
});

it('test request success, text', () => {
const mockHttpResponse = createMockHttpResponse(
200,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export interface OpenSearchRequestArgs {
http: HttpSetup;
requests: any;
dataSourceId?: string;
withLongNumeralsSupport?: boolean;
}

export interface OpenSearchRequestObject {
Expand Down Expand Up @@ -104,7 +105,8 @@ export function sendRequestToOpenSearch(
opensearchMethod,
opensearchPath,
opensearchData,
args.dataSourceId
args.dataSourceId,
args.withLongNumeralsSupport
);
if (reqId !== CURRENT_REQ_ID) {
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
* under the License.
*/

import { UI_SETTINGS } from '../../../../../data/common';

jest.mock('./send_request_to_opensearch', () => ({ sendRequestToOpenSearch: jest.fn() }));
jest.mock('../../contexts/editor_context/editor_registry', () => ({
instance: { getInputEditor: jest.fn() },
Expand Down Expand Up @@ -74,9 +76,63 @@ describe('useSendCurrentRequestToOpenSearch', () => {

const { result } = renderHook(() => useSendCurrentRequestToOpenSearch(), { wrapper: contexts });
await act(() => result.current());

expect(sendRequestToOpenSearch).toHaveBeenCalledWith({
requests: ['test'],
http: mockContextValue.services.http,
});

// Second call should be the request success
const [, [requestSucceededCall]] = (dispatch as jest.Mock).mock.calls;
expect(requestSucceededCall).toEqual({ type: 'requestSuccess', payload: { data: [] } });
});

it('calls sendRequestToOpenSearch turning withLongNumeralsSupport on when long-numerals is enabled', async () => {
// Set up mocks
(mockContextValue.services.settings.toJSON as jest.Mock).mockReturnValue({});
(mockContextValue.services.uiSettings.get as jest.Mock).mockImplementation((key: string) =>
Promise.resolve(key === UI_SETTINGS.DATA_WITH_LONG_NUMERALS ? true : undefined)
);

// This request should succeed
(sendRequestToOpenSearch as jest.Mock).mockResolvedValue([]);
(editorRegistry.getInputEditor as jest.Mock).mockImplementation(() => ({
getRequestsInRange: () => ['test'],
}));

const { result } = renderHook(() => useSendCurrentRequestToOpenSearch(), { wrapper: contexts });
await act(() => result.current());
expect(sendRequestToOpenSearch).toHaveBeenCalledWith({
requests: ['test'],
http: mockContextValue.services.http,
withLongNumeralsSupport: true,
});

// Second call should be the request success
const [, [requestSucceededCall]] = (dispatch as jest.Mock).mock.calls;
expect(requestSucceededCall).toEqual({ type: 'requestSuccess', payload: { data: [] } });
});

it('calls sendRequestToOpenSearch turning withLongNumeralsSupport off when long-numerals is disabled', async () => {
// Set up mocks
(mockContextValue.services.settings.toJSON as jest.Mock).mockReturnValue({});
(mockContextValue.services.uiSettings.get as jest.Mock).mockImplementation((key: string) =>
Promise.resolve(key === UI_SETTINGS.DATA_WITH_LONG_NUMERALS ? false : undefined)
);

// This request should succeed
(sendRequestToOpenSearch as jest.Mock).mockResolvedValue([]);
(editorRegistry.getInputEditor as jest.Mock).mockImplementation(() => ({
getRequestsInRange: () => ['test'],
}));

const { result } = renderHook(() => useSendCurrentRequestToOpenSearch(), { wrapper: contexts });
await act(() => result.current());

expect(sendRequestToOpenSearch).toHaveBeenCalledWith({
requests: ['test'],
http: mockContextValue.services.http,
withLongNumeralsSupport: false,
});

// Second call should be the request success
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,11 @@ import { track } from './track';

// @ts-ignore
import { retrieveAutoCompleteInfo } from '../../../lib/mappings/mappings';
import { UI_SETTINGS } from '../../../../../data/common';

export const useSendCurrentRequestToOpenSearch = (dataSourceId?: string) => {
const {
services: { history, settings, notifications, trackUiMetric, http },
services: { history, settings, notifications, trackUiMetric, http, uiSettings },
} = useServicesContext();

const dispatch = useRequestActionContext();
Expand All @@ -64,7 +65,14 @@ export const useSendCurrentRequestToOpenSearch = (dataSourceId?: string) => {
// Fire and forget
setTimeout(() => track(requests, editor, trackUiMetric), 0);

const results = await sendRequestToOpenSearch({ http, requests, dataSourceId });
const withLongNumeralsSupport = await uiSettings.get(UI_SETTINGS.DATA_WITH_LONG_NUMERALS);

const results = await sendRequestToOpenSearch({
http,
requests,
dataSourceId,
withLongNumeralsSupport,
});

results.forEach(({ request: { path, method, data } }) => {
try {
Expand Down Expand Up @@ -112,5 +120,14 @@ export const useSendCurrentRequestToOpenSearch = (dataSourceId?: string) => {
});
}
}
}, [dispatch, http, dataSourceId, settings, notifications.toasts, trackUiMetric, history]);
}, [
dispatch,
http,
dataSourceId,
settings,
notifications.toasts,
trackUiMetric,
history,
uiSettings,
]);
};
5 changes: 3 additions & 2 deletions src/plugins/console/public/lib/opensearch/opensearch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ export async function send(
method: string,
path: string,
data: any,
dataSourceId?: string
dataSourceId?: string,
withLongNumeralsSupport?: boolean
): Promise<HttpResponse> {
return await http.post<HttpResponse>('/api/console/proxy', {
query: {
Expand All @@ -57,7 +58,7 @@ export async function send(
body: data,
prependBasePath: true,
asResponse: true,
withLongNumeralsSupport: true,
withLongNumeralsSupport,
});
}

Expand Down
1 change: 1 addition & 0 deletions src/plugins/data/common/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,4 +108,5 @@ export const UI_SETTINGS = {
QUERY_DATAFRAME_HYDRATION_STRATEGY: 'query:dataframe:hydrationStrategy',
SEARCH_QUERY_LANGUAGE_BLOCKLIST: 'search:queryLanguageBlocklist',
NEW_HOME_PAGE: 'home:useNewHomePage',
DATA_WITH_LONG_NUMERALS: 'data:withLongNumerals',
Copy link
Member

Choose a reason for hiding this comment

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

nit: i think i originally did data as well. but then was convinced to use search because then we can group it with the search category. not a strong opinion on this though

} as const;
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import { IndexPatternsService, IndexPattern } from '.';
import { fieldFormatsMock } from '../../field_formats/mocks';
import { stubbedSavedObjectIndexPattern } from '../../../../../fixtures/stubbed_saved_object_index_pattern';
import { UiSettingsCommon, SavedObjectsClientCommon, SavedObject } from '../types';
import { UI_SETTINGS } from '../../constants';

const createFieldsFetcher = jest.fn().mockImplementation(() => ({
getFieldsForWildcard: jest.fn().mockImplementation(() => {
Expand All @@ -51,6 +52,7 @@ function setDocsourcePayload(id: string | null, providedPayload: any) {
describe('IndexPatterns', () => {
let indexPatterns: IndexPatternsService;
let savedObjectsClient: SavedObjectsClientCommon;
const uiSettingsGet = jest.fn();

beforeEach(() => {
const indexPatternObj = { id: 'id', version: 'a', attributes: { title: 'title' } };
Expand Down Expand Up @@ -83,9 +85,12 @@ describe('IndexPatterns', () => {
};
});

uiSettingsGet.mockClear();
uiSettingsGet.mockReturnValue(Promise.resolve(false));

indexPatterns = new IndexPatternsService({
uiSettings: ({
get: () => Promise.resolve(false),
get: uiSettingsGet,
getAll: () => {},
} as any) as UiSettingsCommon,
savedObjectsClient: (savedObjectsClient as unknown) as SavedObjectsClientCommon,
Expand Down Expand Up @@ -249,4 +254,13 @@ describe('IndexPatterns', () => {

expect(indexPatterns.savedObjectToSpec(savedObject)).toMatchSnapshot();
});

test('correctly detects long-numerals support', async () => {
expect(await indexPatterns.isLongNumeralsSupported()).toBe(false);

uiSettingsGet.mockImplementation((key: string) =>
Promise.resolve(key === UI_SETTINGS.DATA_WITH_LONG_NUMERALS ? true : undefined)
);
expect(await indexPatterns.isLongNumeralsSupported()).toBe(true);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -747,6 +747,10 @@
indexPatternCache.clear(indexPatternId);
return this.savedObjectsClient.delete('index-pattern', indexPatternId);
}

isLongNumeralsSupported() {
return this.config.get(UI_SETTINGS.DATA_WITH_LONG_NUMERALS);

Check warning on line 752 in src/plugins/data/common/index_patterns/index_patterns/index_patterns.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/common/index_patterns/index_patterns/index_patterns.ts#L752

Added line #L752 was not covered by tests
}
}

export type IndexPatternsContract = PublicMethodsOf<IndexPatternsService>;
11 changes: 11 additions & 0 deletions src/plugins/data/server/ui_settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -535,6 +535,17 @@ export function getUiSettings(
}),
schema: schema.string(),
},
[UI_SETTINGS.DATA_WITH_LONG_NUMERALS]: {
name: i18n.translate('data.advancedSettings.data.withLongNumeralsTitle', {
defaultMessage: 'Extend Numeric Precision',
}),
value: true,
description: i18n.translate('data.advancedSettings.data.withLongNumeralsText', {
defaultMessage:
"Turn on for precise handling of extremely large numbers. Turn off to optimize performance when high precision for large values isn't required.",
}),
schema: schema.boolean(),
},
[UI_SETTINGS.TIMEPICKER_REFRESH_INTERVAL_DEFAULTS]: {
name: i18n.translate('data.advancedSettings.timepicker.refreshIntervalDefaultsTitle', {
defaultMessage: 'Time filter refresh interval',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ async function mountDoc(update = false, indexPatternGetter: any = null) {
};
const indexPatternService = {
get: indexPatternGetter ? indexPatternGetter : jest.fn(() => Promise.resolve(indexPattern)),
isLongNumeralsSupported: jest.fn(),
} as any;

const props = {
Expand Down
Loading
Loading