-
Notifications
You must be signed in to change notification settings - Fork 925
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
[MDS] TSVB Support #6298
[MDS] TSVB Support #6298
Changes from 15 commits
b9434b1
6a00949
910e93a
9b9c681
7be618d
7b7da2f
cae4f7f
8d7522c
e2a738b
62eedbe
44a06ee
9d4abc4
26cd786
c880943
080eb4c
aadcd35
64c45e8
817e342
d4617d3
710eaf7
a0635bf
106a09e
f4f237f
2a611a1
1436a53
a6300f4
76ed1a4
fe2b794
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
import { RequestHandlerContext } from 'src/core/server'; | ||
|
||
export const decideLegacyClient = ( | ||
requestContext: RequestHandlerContext, | ||
dataSourceId: string | null | ||
) => { | ||
return !!dataSourceId && !!requestContext.dataSource | ||
? requestContext.dataSource.opensearch.legacy.getClient(dataSourceId).callAPI | ||
: requestContext.core.opensearch.legacy.client.callAsCurrentUser; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,6 +41,13 @@ import { | |
EuiText, | ||
} from '@elastic/eui'; | ||
import { FieldSelect } from './aggs/field_select'; | ||
import { createDataSourcePickerHandler } from './lib/create_data_source_change_handler'; | ||
import { | ||
getSavedObjectsClient, | ||
getNotifications, | ||
getDataSourceManagementSetup, | ||
getHideLocalCluster, | ||
} from '../../services'; | ||
import { createSelectHandler } from './lib/create_select_handler'; | ||
import { createTextHandler } from './lib/create_text_handler'; | ||
import { YesNo } from './yes_no'; | ||
|
@@ -112,6 +119,14 @@ export const IndexPattern = ({ fields, prefix, onChange, disabled, model: _model | |
}; | ||
|
||
const model = { ...defaults, ..._model }; | ||
|
||
const dataSourceManagementEnabled = | ||
!!getDataSourceManagementSetup().dataSourceManagement || false; | ||
const handleDataSourceSelectChange = createDataSourcePickerHandler(onChange); | ||
const DataSourceSelector = dataSourceManagementEnabled | ||
? getDataSourceManagementSetup().dataSourceManagement.ui.DataSourceSelector | ||
: undefined; | ||
|
||
const isDefaultIndexPatternUsed = model.default_index_pattern && !model[indexPatternName]; | ||
const intervalValidation = validateIntervalValue(model[intervalName]); | ||
const selectedTimeRangeOption = timeRangeOptions.find( | ||
|
@@ -157,18 +172,46 @@ export const IndexPattern = ({ fields, prefix, onChange, disabled, model: _model | |
</EuiFlexItem> | ||
</EuiFlexGroup> | ||
)} | ||
{!!dataSourceManagementEnabled && ( | ||
<EuiFlexGroup> | ||
<EuiFlexItem> | ||
<EuiFormRow | ||
id={htmlId('dataSource')} | ||
label={i18n.translate('visTypeTimeseries.indexPattern.dataSourceLabel', { | ||
defaultMessage: 'Data source', | ||
})} | ||
> | ||
<DataSourceSelector | ||
savedObjectsClient={getSavedObjectsClient().client} | ||
notifications={getNotifications().toasts} | ||
onSelectedDataSource={handleDataSourceSelectChange} | ||
defaultOption={model.data_source_id ? [{ id: model.data_source_id }] : [{ id: '' }]} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shall we let the selector to decide the default data source if not provided? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah fixed it. I realized there could be a case where the |
||
disabled={false} | ||
fullWidth={false} | ||
removePrepend={true} | ||
isClearable={false} | ||
hideLocalCluster={!!getHideLocalCluster().hideLocalCluster} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need to set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed all instances of |
||
/> | ||
</EuiFormRow> | ||
</EuiFlexItem> | ||
</EuiFlexGroup> | ||
)} | ||
<EuiFlexGroup> | ||
<EuiFlexItem> | ||
<EuiFormRow | ||
id={htmlId('indexPattern')} | ||
label={i18n.translate('visTypeTimeseries.indexPatternLabel', { | ||
defaultMessage: 'Index pattern', | ||
defaultMessage: 'Index name', | ||
})} | ||
helpText={ | ||
isDefaultIndexPatternUsed && | ||
i18n.translate('visTypeTimeseries.indexPattern.searchByDefaultIndex', { | ||
defaultMessage: 'Default index pattern is used. To query all indexes use *', | ||
}) | ||
isDefaultIndexPatternUsed | ||
? i18n.translate('visTypeTimeseries.indexPattern.searchByDefaultIndex', { | ||
defaultMessage: 'Default index pattern is used. To query all indexes use *', | ||
}) | ||
: i18n.translate('visTypeTimeseries.indexPattern.searchByIndex', { | ||
defaultMessage: | ||
'Use an asterisk (*) to match multiple indices. Spaces and the characters , /, ?, ", <, >, | are not allowed.', | ||
}) | ||
} | ||
> | ||
<EuiFieldText | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
import _ from 'lodash'; | ||
|
||
import { PanelSchema } from 'src/plugins/vis_type_timeseries/common/types'; | ||
import { DATA_SOURCE_ID_KEY } from '../../../../common/constants'; | ||
|
||
export const createDataSourcePickerHandler = (handleChange: (e: PanelSchema) => void) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we have a test for this function? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added test cases. |
||
return (selectedOptions: []): void => { | ||
return handleChange?.({ | ||
[DATA_SOURCE_ID_KEY]: _.get(selectedOptions, '[0].id', null), | ||
} as PanelSchema); | ||
}; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,7 +28,14 @@ | |
* under the License. | ||
*/ | ||
|
||
import { I18nStart, SavedObjectsStart, IUiSettingsClient, CoreStart } from 'src/core/public'; | ||
import { | ||
I18nStart, | ||
SavedObjectsStart, | ||
IUiSettingsClient, | ||
CoreStart, | ||
NotificationsStart, | ||
} from 'src/core/public'; | ||
import { DataSourceManagementPluginSetup } from 'src/plugins/data_source_management/public'; | ||
import { createGetterSetter } from '../../opensearch_dashboards_utils/public'; | ||
import { ChartsPluginSetup } from '../../charts/public'; | ||
import { DataPublicPluginStart } from '../../data/public'; | ||
|
@@ -52,3 +59,15 @@ export const [getI18n, setI18n] = createGetterSetter<I18nStart>('I18n'); | |
export const [getChartsSetup, setChartsSetup] = createGetterSetter<ChartsPluginSetup>( | ||
'ChartsPluginSetup' | ||
); | ||
|
||
export const [getDataSourceManagementSetup, setDataSourceManagementSetup] = createGetterSetter<{ | ||
dataSourceManagement: DataSourceManagementPluginSetup | undefined; | ||
}>('DataSourceManagementSetup'); | ||
|
||
export const [getHideLocalCluster, setHideLocalCluster] = createGetterSetter<{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. similar to my previous comment, this also can be removed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed all instances of |
||
hideLocalCluster: boolean | undefined; | ||
}>('HideLocalCluster'); | ||
|
||
export const [getNotifications, setNotifications] = createGetterSetter<NotificationsStart>( | ||
'Notifications' | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,26 +39,28 @@ import { | |
IndexPatternsFetcher, | ||
} from '../../../data/server'; | ||
import { ReqFacade } from './search_strategies/strategies/abstract_search_strategy'; | ||
import { decideLegacyClient } from '../../../data_source/server'; | ||
|
||
export async function getFields( | ||
requestContext: RequestHandlerContext, | ||
request: OpenSearchDashboardsRequest, | ||
framework: Framework, | ||
indexPattern: string | ||
indexPattern: string, | ||
dataSourceId: string | null | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can dataSourceId be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 to @BionIT. Instead of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed, after some investigation, it's possible to set a schema as optional via |
||
) { | ||
// NOTE / TODO: This facade has been put in place to make migrating to the New Platform easier. It | ||
// removes the need to refactor many layers of dependencies on "req", and instead just augments the top | ||
// level object passed from here. The layers should be refactored fully at some point, but for now | ||
// this works and we are still using the New Platform services for these vis data portions. | ||
const client = decideLegacyClient(requestContext, dataSourceId); | ||
|
||
const reqFacade: ReqFacade = { | ||
requestContext, | ||
...request, | ||
framework, | ||
payload: {}, | ||
pre: { | ||
indexPatternsService: new IndexPatternsFetcher( | ||
requestContext.core.opensearch.legacy.client.callAsCurrentUser | ||
), | ||
indexPatternsService: new IndexPatternsFetcher(client), | ||
}, | ||
getUiSettingsService: () => requestContext.core.uiSettings.client, | ||
getSavedObjectsClient: () => requestContext.core.savedObjects.client, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: let's use optional parameter instead of null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have decideClient in https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/plugins/data/server/search/opensearch_search/decide_client.ts.
Can we use same file for
decideLegacyClient
?Can we use request to get dataSourceId similar to decideClient?
OR
maybe refactor decideClient to return legacy client base on additional parameter?
I can see in decideClient we are supporting LongNumerals. Is it something we need to take for legacy client? Can you double check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this function
OpenSearch-Dashboards/src/plugins/data/server/index_patterns/routes.ts
Line 158 in b4130aa
Looks like we have added this function while adding MDS feature. can we use same one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrote a workaround to use
decideClient()
but this will need a refactor because other plugins may need this function as well. Raised a new issue to address this #6417