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 query assist to query enhancements plugin #14

Merged
merged 10 commits into from
Jun 25, 2024

Conversation

joshuali925
Copy link
Collaborator

@joshuali925 joshuali925 commented Jun 18, 2024

This PR picks

banner screenshot:

image

query assist screenshot:

image

TODO items, will send PRs later

  • add UT/IT for query assist
  • support keyboard navigation in suggestions component
  • implement load more logic in suggestions component if needed
  • add URL for Natural Language Query Generation for PPL inside banner
  • use data source and index from datasource and dataset selectors when they are implemented in discover

it adds query assist specific logic in query enhancements plugin to show a UI above the PPL search bar if user has configured PPL agent.

Issues Resolved: #6820

* add query assist to query enhancements

Signed-off-by: Joshua Li <[email protected]>

* align language to uppercase

Signed-off-by: Joshua Li <[email protected]>

* pick PR 6167

Signed-off-by: Joshua Li <[email protected]>

* use useState hooks for query assist

There is a bug in data explorer `AppContainer` where memorized
`DiscoverCanvas` gets unmounted after `setQuery`. PR 6167 works around
it by memorizing `AppContainer`. As query assist is no longer being
unmounted, we can use proper hooks to persist state now.

Signed-off-by: Joshua Li <[email protected]>

* Revert "pick PR 6167"

This reverts commit acb0d41937e30bd76c666a225407743243692d11.

Wait for official 6167 to merge to avoid conflict

Signed-off-by: Joshua Li <[email protected]>

* address comments for PR 6894

Signed-off-by: Joshua Li <[email protected]>

---------

Signed-off-by: Joshua Li <[email protected]>
(cherry picked from commit 016e0f2f73efd8bb0649151908c67dd7ac09d174)
…assist (#6933)

* pass dependencies to isEnabled func

Signed-off-by: Joshua Li <[email protected]>

* add lazy and memo to search bar extensions

Signed-off-by: Joshua Li <[email protected]>

* move ppl specific string out from query assist

Signed-off-by: Joshua Li <[email protected]>

* prevent setstate after hook unmounts

Signed-off-by: Joshua Li <[email protected]>

* add max-height to search bar extensions

Signed-off-by: Joshua Li <[email protected]>

* prevent setstate after component unmounts

Signed-off-by: Joshua Li <[email protected]>

* move ml-commons API to common/index.ts

Signed-off-by: Joshua Li <[email protected]>

* improve i18n and accessibility usages

Signed-off-by: Joshua Li <[email protected]>

* add hard-coded suggestions for sample data indices

Signed-off-by: Joshua Li <[email protected]>

---------

Signed-off-by: Joshua Li <[email protected]>
(cherry picked from commit 4aade0f993559b0bae9cbcee8e889868afa88547)
* disable query assist for non-default datasource

Signed-off-by: Joshua Li <[email protected]>

* disable query assist input when loading

Signed-off-by: Joshua Li <[email protected]>

* support MDS for query assist

Signed-off-by: Joshua Li <[email protected]>

* add unit tests for agents

Signed-off-by: Joshua Li <[email protected]>

* Revert "add unit tests for agents"

This reverts commit 983514ee11362c5efe4cdb59802b3ff402b61ef2.
The test configs are not yet setup in query_enhancements plugins.

Signed-off-by: Joshua Li <[email protected]>

---------

Signed-off-by: Joshua Li <[email protected]>
(cherry picked from commit 328e08e688c39de1f47fee1c357e9928c0576390)
Signed-off-by: Joshua Li <[email protected]>
@kavilla
Copy link
Owner

kavilla commented Jun 20, 2024

will review this today as i was holding off for the OSD PR to be merged first.

Signed-off-by: Joshua Li <[email protected]>
This is a temporary solution given that in discover the index pattern
selector will be removed. Before datasource and dataset selectors are
added, query assist will rely on this index pattern selector to
determine which index user wants to query.

Signed-off-by: Joshua Li <[email protected]>

process.env.TZ = 'UTC';

module.exports = {
Copy link
Owner

Choose a reason for hiding this comment

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

does this have something that we dont have available already within the global jest config?

https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/dev/jest/config.js

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i don't think global config applies to plugin tests?

@@ -6,6 +6,7 @@
"scripts": {
"build": "yarn plugin-helpers build",
"plugin-helpers": "node ../../scripts/plugin_helpers",
"test": "../../node_modules/.bin/jest --config ./test/jest.config.js",
Copy link
Owner

Choose a reason for hiding this comment

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

similar to below will yarn test:jest not capture these tests?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

in queryEnhancements yarn test:jest is not defined

Copy link
Owner

Choose a reason for hiding this comment

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

yeah i mean like like if i was in the root repo and ran like

yarn test:jest {pathtotest} does it work?

@joshuali925
Copy link
Collaborator Author

i copied the test configs from another plugin just so i can run tests. they can be removed if this plugin is going into core

Copy link
Owner

@kavilla kavilla left a comment

Choose a reason for hiding this comment

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

some comments but overall awesome! i would be fine merging as is if you want to give my comments a. once over

common/query_assist/index.ts Outdated Show resolved Hide resolved
Copy link
Owner

Choose a reason for hiding this comment

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

nit: is this technically a mark?
nit: i think we can either remove the logo or call it mark to match the pattern:
https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/core/server/core_app/assets/logos/opensearch_mark.svg

so like query_assist_mark.svg

and to match the current strucutre can we add duplicates for light and dark ? im not sure if there is a light mode or dark mode. might need ux feedback.

Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

opensearch_mark is inside the logo directory, i'm not really sure what the difference is between a mark and a logo, but i'll rename it to mark. currently we only have one version, i can duplicate but why are there three versions: mark, mark_on_light, mark_on_dark?

indexPatterns: IndexPatternsContract,
indexPattern: IIndexPattern | string | undefined
): Promise<string | undefined> => {
if (!indexPattern || typeof indexPattern !== 'object' || !indexPattern.id) return undefined;
Copy link
Owner

Choose a reason for hiding this comment

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

nit: would this be needed if i got this in?

opensearch-project/OpenSearch-Dashboards#7092
#18

where it registers schemas from observability? we might have to just do the same for query assist?

server/routes/query_assist/routes.ts Outdated Show resolved Hide resolved
server/routes/query_assist/routes.ts Outdated Show resolved Hide resolved
server/routes/query_assist/routes.ts Outdated Show resolved Hide resolved

// babelrc doesn't respect NODE_PATH anymore but using require does.
// Alternative to install them locally in node_modules
module.exports = function (api) {
Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not sure, i'm getting

ERROR [queryEnhancements] depends on [@osd/babel-preset] but it's not using the local package. Update its package.json to the expected value below.
 info Additional debugging info:

 │ info actual: "@osd/babel-preset": "1.0.0"
 │      expected: "@osd/babel-preset": "link:../../packages/osd-babel-preset"

Copy link
Owner

Choose a reason for hiding this comment

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

oh wow. we might need to open a bug into core.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

common/query_assist/index.ts Outdated Show resolved Hide resolved
common/query_assist/index.ts Outdated Show resolved Hide resolved
@kavilla kavilla merged commit 90ab0bf into kavilla:main Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants