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

[MDS] Support for Timeline #6385

Merged
merged 14 commits into from
Apr 16, 2024
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [Multiple Datasource] Add default icon in multi-selectable picker ([#6357](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6357))
- [Workspace] Add APIs to support plugin state in request ([#6303](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6303))
- [Workspace] Filter left nav menu items according to the current workspace ([#6234](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6234))
- [Multiple Datasource] Add multi data source support to Timeline ([#6385](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6385))
- [Multiple Datasource] Refactor data source selector component to include placeholder and add tests ([#6372](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6372))
- Replace control characters before logging ([#6402](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6402))
- [Dynamic Configurations] Improve dynamic configurations by adding cache and simplifying client fetch ([#6364](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6364))
Expand Down
1 change: 1 addition & 0 deletions src/plugins/vis_type_timeline/opensearch_dashboards.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"opensearchDashboardsVersion": "opensearchDashboards",
"server": true,
"ui": true,
"optionalPlugins": ["dataSource"],
"requiredPlugins": ["visualizations", "data", "expressions"],
"requiredBundles": ["opensearchDashboardsUtils", "opensearchDashboardsReact", "visDefaultEditor"]
}
Original file line number Diff line number Diff line change
Expand Up @@ -129,10 +129,12 @@ export function getTimelineRequestHandler({
});
} catch (e) {
if (e && e.body) {
const errorTitle =
e.body.attributes && e.body.attributes.title ? ` (${e.body.attributes.title})` : '';
const err = new Error(
`${i18n.translate('timeline.requestHandlerErrorTitle', {
defaultMessage: 'Timeline request error',
})}: ${e.body.title} ${e.body.message}`
})}${errorTitle}: ${e.body.message}`
);
err.stack = e.stack;
throw err;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,9 @@ export default function chainRunner(tlConfig) {
let sheet;

function throwWithCell(cell, exception) {
throw new Error(' in cell #' + (cell + 1) + ': ' + exception.message);
const e = new Error(exception.message);
e.name = `Expression parse error in cell #${cell + 1}`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

does the name field matter?

Copy link
Member Author

@huyaboo huyaboo Apr 16, 2024

Choose a reason for hiding this comment

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

Yes: server-side error messages are propagated to the client and the toast error message will show (this is existing behavior and UX is aligned). This is needed to make the toast more readable (stack information is kept hidden from client).

throw e;
}

// Invokes a modifier function, resolving arguments into series as needed
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import { savedObjectsClientMock } from '../../../../core/server/mocks';
import { fetchDataSourceIdByName } from './fetch_data_source_id';
import { OpenSearchFunctionConfig } from '../types';

jest.mock('./services', () => ({
getDataSourceEnabled: jest
.fn()
.mockReturnValueOnce({ enabled: false })
.mockReturnValue({ enabled: true }),
}));

describe('fetchDataSourceIdByName()', () => {
const validId = 'some-valid-id';
const config: OpenSearchFunctionConfig = {
q: null,
metric: null,
split: null,
index: null,
timefield: null,
kibana: null,
opensearchDashboards: null,
interval: null,
};
const client = savedObjectsClientMock.create();
client.find = jest.fn().mockImplementation((props) => {
if (props.search === '"No Results With Filter"') {
return Promise.resolve({
saved_objects: [
{
id: 'some-non-matching-id',
attributes: {
title: 'No Results With Filter Some Suffix',
},
},
],
});
}
if (props.search === '"Duplicate Title"') {
return Promise.resolve({
saved_objects: [
{
id: 'duplicate-id-1',
attributes: {
title: 'Duplicate Title',
},
},
{
id: 'duplicate-id-2',
attributes: {
title: 'Duplicate Title',
},
},
],
});
}
if (props.search === '"Some Data Source"') {
return Promise.resolve({
saved_objects: [
{
id: validId,
attributes: {
title: 'Some Data Source',
},
},
],
});
}
if (props.search === '"Some Prefix"') {
return Promise.resolve({
saved_objects: [
{
id: 'some-id-2',
attributes: {
title: 'Some Prefix B',
},
},
{
id: validId,
attributes: {
title: 'Some Prefix',
},
},
],
});
}

return Promise.resolve({ saved_objects: [] });
});

it('should return undefined if data_source_name is not present', async () => {
expect(await fetchDataSourceIdByName(config, client)).toBe(undefined);
});

it('should return undefined if data_source_name is an empty string', async () => {
expect(await fetchDataSourceIdByName({ ...config, data_source_name: '' }, client)).toBe(
undefined
);
});

it('should throw errors when MDS is disabled', async () => {
await expect(
fetchDataSourceIdByName({ ...config, data_source_name: 'Some Data Source' }, client)
).rejects.toThrowError(
'To query from multiple data sources, first enable the data source feature'
);
});

it.each([
{
dataSourceName: 'Non-existent Data Source',
expectedResultCount: 0,
},
{
dataSourceName: 'No Results With Filter',
expectedResultCount: 0,
},
{
dataSourceName: 'Duplicate Title',
expectedResultCount: 2,
},
])(
'should throw errors when non-existent or duplicate data_source_name is provided',
async ({ dataSourceName, expectedResultCount }) => {
await expect(
fetchDataSourceIdByName({ ...config, data_source_name: dataSourceName }, client)
).rejects.toThrowError(
`Expected exactly 1 result for data_source_name "${dataSourceName}" but got ${expectedResultCount} results`
);
}
);

it.each([
{
dataSourceName: 'Some Data Source',
},
{
dataSourceName: 'Some Prefix',
},
])(
'should return valid id when data_source_name exists and is unique',
async ({ dataSourceName }) => {
expect(
await fetchDataSourceIdByName({ ...config, data_source_name: dataSourceName }, client)
).toBe(validId);
}
);
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import { SavedObjectsClientContract } from 'src/core/server';
import { DataSourceAttributes } from 'src/plugins/data_source/common/data_sources';
import { getDataSourceEnabled } from './services';
import { OpenSearchFunctionConfig } from '../types';

export const fetchDataSourceIdByName = async (
Copy link
Member

Choose a reason for hiding this comment

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

Didn't we define similar function for vega? In vega, we are fetching data source id by name, right? Correct me if I'm missing something.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes but Vega fetches data source id client-side while Timeline fetches data source id information server-side. I agree that this is something that needs to be refactored into one utility function in data source plugin and I cut a new issue #6417

Copy link
Member

Choose a reason for hiding this comment

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

Is there an ETA to refactor this function? Just asking because if we don't actively track and then we will keep using these two separate function when they are performing same 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.

I'm not sure if this is a huge priority at the moment. Most plugins are not blocked by this refactor, but once the UI components are finalized (these are blocking issues), I will pick up this task. This should be started within this sprint or at the most, next sprint.

config: OpenSearchFunctionConfig,
client: SavedObjectsClientContract
) => {
if (!config.data_source_name) {
return undefined;
}

if (!getDataSourceEnabled().enabled) {
throw new Error('To query from multiple data sources, first enable the data source feature');
}

const dataSources = await client.find<DataSourceAttributes>({
type: 'data-source',
perPage: 100,
search: `"${config.data_source_name}"`,
searchFields: ['title'],
fields: ['id', 'title'],
});

const possibleDataSourceIds = dataSources.saved_objects.filter(
(obj) => obj.attributes.title === config.data_source_name
);

if (possibleDataSourceIds.length !== 1) {
throw new Error(
`Expected exactly 1 result for data_source_name "${config.data_source_name}" but got ${possibleDataSourceIds.length} results`
);
}

return possibleDataSourceIds.pop()?.id;
};
10 changes: 10 additions & 0 deletions src/plugins/vis_type_timeline/server/lib/services.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import { createGetterSetter } from '../../../opensearch_dashboards_utils/common';

export const [getDataSourceEnabled, setDataSourceEnabled] = createGetterSetter<{
enabled: boolean;
}>('DataSource');
10 changes: 9 additions & 1 deletion src/plugins/vis_type_timeline/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import { TypeOf, schema } from '@osd/config-schema';
import { RecursiveReadonly } from '@osd/utility-types';
import { deepFreeze } from '@osd/std';

import { DataSourcePluginSetup } from 'src/plugins/data_source/server';
import { PluginStart } from '../../data/server';
import { CoreSetup, PluginInitializerContext } from '../../../core/server';
import { configSchema } from '../config';
Expand All @@ -42,11 +43,16 @@ import { functionsRoute } from './routes/functions';
import { validateOpenSearchRoute } from './routes/validate_es';
import { runRoute } from './routes/run';
import { ConfigManager } from './lib/config_manager';
import { setDataSourceEnabled } from './lib/services';

const experimentalLabel = i18n.translate('timeline.uiSettings.experimentalLabel', {
defaultMessage: 'experimental',
});

export interface TimelinePluginSetupDeps {
dataSource?: DataSourcePluginSetup;
}

export interface TimelinePluginStartDeps {
data: PluginStart;
}
Expand All @@ -57,7 +63,7 @@ export interface TimelinePluginStartDeps {
export class Plugin {
constructor(private readonly initializerContext: PluginInitializerContext) {}

public async setup(core: CoreSetup): void {
public async setup(core: CoreSetup, { dataSource }: TimelinePluginSetupDeps): void {
const config = await this.initializerContext.config
.create<TypeOf<typeof configSchema>>()
.pipe(first())
Expand All @@ -80,6 +86,8 @@ export class Plugin {
);
};

setDataSourceEnabled({ enabled: !!dataSource });

const logger = this.initializerContext.logger.get('timeline');

const router = core.http.createRouter();
Expand Down
5 changes: 4 additions & 1 deletion src/plugins/vis_type_timeline/server/routes/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,10 @@ export function runRoute(
} else {
return response.internalError({
body: {
message: err.toString(),
attributes: {
title: err.name,
},
message: err.message,
},
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import { OPENSEARCH_SEARCH_STRATEGY } from '../../../../data/server';
import Datasource from '../../lib/classes/datasource';
import buildRequest from './lib/build_request';
import toSeriesList from './lib/agg_response_to_series_list';
import { fetchDataSourceIdByName } from '../../lib/fetch_data_source_id';

export default new Datasource('es', {
args: [
Expand Down Expand Up @@ -112,6 +113,14 @@ export default new Datasource('es', {
defaultMessage: `**DO NOT USE THIS**. It's fun for debugging fit functions, but you really should use the interval picker`,
}),
},
{
name: 'data_source_name',
types: ['string', 'null'], // If null, the query will proceed with local cluster
bandinib-amzn marked this conversation as resolved.
Show resolved Hide resolved
help: i18n.translate('timeline.help.functions.opensearch.args.dataSourceNameHelpText', {
defaultMessage:
'Specify a data source to query from. This will only work if multiple data sources is enabled',
}),
},
],
help: i18n.translate('timeline.help.functions.opensearchHelpText', {
defaultMessage: 'Pull data from an opensearch instance',
Expand Down Expand Up @@ -148,7 +157,15 @@ export default new Datasource('es', {

const opensearchShardTimeout = tlConfig.opensearchShardTimeout;

const body = buildRequest(config, tlConfig, scriptedFields, opensearchShardTimeout);
const dataSourceId = await fetchDataSourceIdByName(config, tlConfig.savedObjectsClient);

const body = buildRequest(
config,
tlConfig,
scriptedFields,
opensearchShardTimeout,
dataSourceId
);

const deps = (await tlConfig.getStartServices())[1];

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import { buildAggBody } from './agg_body';
import createDateAgg from './create_date_agg';
import { UI_SETTINGS } from '../../../../../data/server';

export default function buildRequest(config, tlConfig, scriptedFields, timeout) {
export default function buildRequest(config, tlConfig, scriptedFields, timeout, dataSourceId) {
const bool = { must: [] };

const timeFilter = {
Expand Down Expand Up @@ -105,6 +105,7 @@ export default function buildRequest(config, tlConfig, scriptedFields, timeout)
}

return {
...(!!dataSourceId && { dataSourceId }),
params: request,
};
}
15 changes: 15 additions & 0 deletions src/plugins/vis_type_timeline/server/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,18 @@
*/

export { TimelineFunctionInterface, TimelineFunctionConfig } from './lib/classes/timeline_function';

export interface OpenSearchFunctionConfig {
q: string | null;
metric: string | null;
split: string | null;
index: string | null;
timefield: string | null;
kibana: boolean | null;
opensearchDashboards: boolean | null;
/**
* @deprecated This property should not be set in the Timeline expression. Users should use the interval picker React component instead
*/
interval: string | null;
data_source_name?: string | null;
}
Loading