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

[Backport 2.x] [MDS] Support for Timeline #6493

Merged
merged 3 commits into from
May 13, 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
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}`;
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 (
config: OpenSearchFunctionConfig,
client: SavedObjectsClientContract
) => {
if (!config.data_source_name) {
return undefined;
}

if (!getDataSourceEnabled().enabled) {
Copy link
Member

@kavilla kavilla Apr 21, 2024

Choose a reason for hiding this comment

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

I can imagine a world where saved objects were imported from a cluster that enabled MDS to a new domain that didn't enable it. If the timeline vis was added to a dashboard now it will fail.

Do we want to consider that if it's not enabled it just queries to the local cluster?

Copy link
Member

Choose a reason for hiding this comment

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

Existing behavior with Timeline is that any invalid params are caught and a toast error is displayed, meaning before this merge, if a user types in data_source_name, a toast would be thrown anyway because this was not a named argument. This means that any syntax error or invalid param can and should be caught. If the visualization just queries a local cluster, then it can be confusing behavior especially since this change enables Timeline to query both local cluster and MDS queries within one visualization.

throw new Error('To query from multiple data sources, first enable the data source feature');
}

const dataSources = await client.find<DataSourceAttributes>({
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should need to query the cluster every time to get the source. If I put an auto refresh interval then it seems like this will hit the cluster twice per request. I think we should caching these data sources like index patterns. If the data source exists in that cache then we can just pull that, if not then the service will pull the data sources.

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
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