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
Merged

Conversation

huyaboo
Copy link
Member

@huyaboo huyaboo commented Apr 9, 2024

Description

Previously, MDS did not support Timeline. This PR adds a new field data_source_name similar to #5927 that enables users to specify a unique data source in which to query data from. This allows users to query from multiple indices and multiple data sources via the .opensearch() function.

Issues Resolved

Closes #6009 and is related to #6410

Screenshot

Screen.Recording.2024-04-10.at.3.27.37.PM.mov

Testing the changes

If MDS is disabled:

  • Adding a data_source_name will throw an error server-side and a toast message will be shown indicating that MDS is disabled
  • image
  • Local Cluster queries function as usual
  • image

If MDS is enabled:

  1. Create a data source(s): In the recording, Source A and Source B were connected
  2. Navigate to Timeline visualization
  3. Add data_source_name="<data source name>" at the end of the .opensearch() expression. Order matters with Timeline expressions as arguments can be named or unnamed.
  4. Assuming the data source has the index and data, visualization will update

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

@bandinib-amzn
Copy link
Member

Before rebase:
All checks have passed
64 successful, 1 skipped, and 1 neutral checks

CHANGELOG.md Outdated Show resolved Hide resolved
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.

Comment on lines 33 to 34
throw new Error(
`Expected exactly 1 result for data_source_name "${config.data_source_name}" but got ${possibleDataSourceIds.length} results`
Copy link
Member

Choose a reason for hiding this comment

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

Is this user facing error?

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. Timeline parses the expression server-side and throws an Error if the expression is incorrectly written. This will behave the same way.

Comment on lines 40 to 44
/**
* @deprecated The interval picker should be used instead
*/
interval: string | null;
Copy link
Member

Choose a reason for hiding this comment

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

what is meaning of comment? It is deprecated and we are still using it?

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Member Author

@huyaboo huyaboo Apr 11, 2024

Choose a reason for hiding this comment

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

In the Timeline expression, there is an option for the user to specify an interval. However, as noted in the opensearch function args, there is a picker react component that users should use instead. I assume interval is kept as to not break any existing visualization so I marked it as deprecated.

{
name: 'interval', // You really shouldn't use this, use the interval picker instead
types: ['string', 'null'],
help: i18n.translate('timeline.help.functions.opensearch.args.intervalHelpText', {
defaultMessage: `**DO NOT USE THIS**. It's fun for debugging fit functions, but you really should use the interval picker`,
}),
},

zhongnansu
zhongnansu previously approved these changes Apr 11, 2024
Comment on lines 40 to 44
/**
* @deprecated The interval picker should be used instead
*/
interval: string | null;
Copy link
Member

Choose a reason for hiding this comment

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

+1

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.

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.

) => {
if (config.data_source_name) {
if (!getDataSourceEnabled().enabled) {
throw new Error('To query from multiple data sources, first enable the data sources feature');
Copy link
Member

Choose a reason for hiding this comment

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

nit: shouldn't be enable the data source feature?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

await expect(
fetchDataSourceIdByName({ ...config, data_source_name: 'Some Data Source' }, client)
).rejects.toThrowError(
'To query from multiple data sources, first enable the data sources feature'
Copy link
Member

Choose a reason for hiding this comment

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

this also needs to fix as well, right? as per this commit a274600

Signed-off-by: Huy Nguyen <[email protected]>
bandinib-amzn
bandinib-amzn previously approved these changes Apr 16, 2024
@@ -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).

config: OpenSearchFunctionConfig,
client: SavedObjectsClientContract
) => {
if (config.data_source_name) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Refactored

@bandinib-amzn bandinib-amzn merged commit 73e5d78 into opensearch-project:main Apr 16, 2024
130 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 16, 2024
* Add MDS support to Timeline

Signed-off-by: Huy Nguyen <[email protected]>

* Refactor to function and add unit tests

Signed-off-by: Huy Nguyen <[email protected]>

* Fix typo in args

Signed-off-by: Huy Nguyen <[email protected]>

* Refactor build request to pass unit tests

Signed-off-by: Huy Nguyen <[email protected]>

* Add to CHANGELOG

Signed-off-by: Huy Nguyen <[email protected]>

* Refactor error messages + address comments

Signed-off-by: Huy Nguyen <[email protected]>

* Fix ut

Signed-off-by: Huy Nguyen <[email protected]>

* Change to data source feature

Signed-off-by: Huy Nguyen <[email protected]>

* Fix UT

Signed-off-by: Huy Nguyen <[email protected]>

* Address comments

Signed-off-by: Huy Nguyen <[email protected]>

---------

Signed-off-by: Huy Nguyen <[email protected]>
(cherry picked from commit 73e5d78)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
@huyaboo huyaboo deleted the timeline branch April 18, 2024 00:15
zhongnansu pushed a commit that referenced this pull request May 13, 2024
* Add MDS support to Timeline

Signed-off-by: Huy Nguyen <[email protected]>

* Refactor to function and add unit tests

Signed-off-by: Huy Nguyen <[email protected]>

* Fix typo in args

Signed-off-by: Huy Nguyen <[email protected]>

* Refactor build request to pass unit tests

Signed-off-by: Huy Nguyen <[email protected]>

* Add to CHANGELOG

Signed-off-by: Huy Nguyen <[email protected]>

* Refactor error messages + address comments

Signed-off-by: Huy Nguyen <[email protected]>

* Fix ut

Signed-off-by: Huy Nguyen <[email protected]>

* Change to data source feature

Signed-off-by: Huy Nguyen <[email protected]>

* Fix UT

Signed-off-by: Huy Nguyen <[email protected]>

* Address comments

Signed-off-by: Huy Nguyen <[email protected]>

---------

Signed-off-by: Huy Nguyen <[email protected]>
(cherry picked from commit 73e5d78)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: ZilongX <[email protected]>
@zhongnansu zhongnansu added the multiple datasource multiple datasource project label Jun 6, 2024
@zhongnansu zhongnansu added the enhancement New feature or request label Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature][MDS] Add data source support to Timeline visualization
4 participants