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

[GS] add savedObjects result provider #68619

Merged
merged 17 commits into from
Jul 6, 2020

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Jun 9, 2020

Summary

Fix #65222

Checklist

@pgayvallet pgayvallet added release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.9.0 v8.0.0 labels Jun 16, 2020
@joshdover
Copy link
Contributor

Are we blocked on anything for this one?

@pgayvallet
Copy link
Contributor Author

Are we blocked on anything for this one?

I was waiting on #68620 and #68894 to finalize the PR.

@pgayvallet
Copy link
Contributor Author

@ryankeairns The PR should be ready soon.

In the meantime, a status regarding the current 'searchable' saved objects. For the record, the rules to be searchable by the GS API are the same that from the savedObject management section:

  • The type must not be hidden
  • The type declaration must specify the management.defaultSearchField property
  • The type declaration must specify the management.getInAppUrl property

Currently, these types match these criteria and are findable by the GS API:

[ 
  'index-pattern',
  'query',
  'search',
  'visualization',
  'canvas-workpad',
  'map',
  'dashboard',
  'lens' 
]
  • If some searchable types should be removed, please tell me so that I introduce a white/black list mechanism
  • If some types required for V1 are not in the list (see 2nd list below), could you please list them? Note that I can't adapt the types myself and will have to open issues for the owning teams to add the required management metadatas to their SO types.

For the record, a quick list of all our registered SO types:

[ 'config',
  'task',
  'telemetry',
  'ui-metric',
  'application_usage_totals',
  'application_usage_transactional',
  'timelion-sheet',
  'url',
  'index-pattern',
  'query',
  'kql-telemetry',
  'search',
  'sample-data-telemetry',
  'upgrade-assistant-reindex-operation',
  'upgrade-assistant-telemetry',
  'file-upload-telemetry',
  'visualization',
  'tsvb-validation-telemetry',
  'canvas-element',
  'canvas-workpad',
  'maps-telemetry',
  'map',
  'graph-workspace',
  'dashboard',
  'lens',
  'lens-ui-telemetry',
  'space',
  'action',
  'action_task_params',
  'cases',
  'cases-comments',
  'cases-configure',
  'cases-user-actions',
  'alert',
  'infrastructure-ui-source',
  'metrics-explorer-view',
  'inventory-view',
  'uptime-dynamic-settings',
  'ml-telemetry',
  'apm-indices',
  'apm-telemetry' 
]

@pgayvallet pgayvallet marked this pull request as ready for review June 23, 2020 11:50
@pgayvallet pgayvallet requested a review from a team as a code owner June 23, 2020 11:50
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

Copy link
Contributor

@mshustov mshustov left a comment

Choose a reason for hiding this comment

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

some optional nits, LGTM overall

import { GlobalSearchResultProvider } from '../../../../global_search/server';
import { mapToResults } from './map_object_to_result';

export const createSavedObjectsResultProvider = (): GlobalSearchResultProvider => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we need the factory pattern here?

.pipe(
map((batch) => batch.results),
// remove test types
map((results) => results.filter((r) => !r.type.startsWith('test_'))),
Copy link
Contributor

Choose a reason for hiding this comment

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

map + reduce combination can be substituted by flatMap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

flatMap would emit an individual event for each result. Could be replace with flatMap + toArray, but map + reduce is probably more explicit.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@pgayvallet pgayvallet merged commit 04aaba8 into elastic:master Jul 6, 2020
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Jul 6, 2020
* create server-side skeleton

* add base implementation & tests

* add unit test for provider

* remove useless contracts

* add preference search option

* implement score from find results

* fix types

* add FTR test

* fix test plugin types

* address ome review comments

* add multi results test

* use `getVisibleTypes`
pgayvallet added a commit that referenced this pull request Jul 6, 2020
* create server-side skeleton

* add base implementation & tests

* add unit test for provider

* remove useless contracts

* add preference search option

* implement score from find results

* fix types

* add FTR test

* fix test plugin types

* address ome review comments

* add multi results test

* use `getVisibleTypes`
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 6, 2020
* master:
  [APM] Fix flaky e2e’s (elastic#70790)
  [Discover] Migrate server side saved object from data to discover plugin (elastic#70342)
  [APM] Update docs on running API tests (elastic#70765)
  test: 💍 delete a flaky test (elastic#70785)
  [Security Solution] Refactor GlobalTime to useGlobalTime hook and cle… (elastic#69345)
  Remove IE11 mention from PR template [skip ci] (elastic#70486)
  [GS] add savedObjects result provider (elastic#68619)
  remove snapshot from disabled test suite. (elastic#70769)
@ryankeairns
Copy link
Contributor

@pgayvallet apologies for the delayed response, I've been out of the office for the past two weeks.

The list for V1 looks great and there are no other objects that I, personally, expected to appear from the second list. That said, there are some interesting items in that second list (e.g. cases) that I'm sure other teams would like to have included. As part of demoing and promoting this new feature, @alexfrancoeur and I can communicate to other teams how they can get their content registered.

Out of curiosity, is there a way to test the search API and see what sort of results are returned?

Thank you for all of your hard work, we are beyond excited to get this into Kibana and start trying it out!

@alexfrancoeur
Copy link

Thanks for the tag @ryankeairns. I have some comments and a few follow up questions listed below.

Generally, this list looks good to me but I see a few saved objects that we'll probably want to return eventually. I would think graph-workspace is something we'll want to add the list of returned SO's. As time goes on and the alerting framework GA's, we'll probably want to provide quick access to alert configurations as well. Maybe we can open up some issues to track? Happy to do so if needed.

query is an interesting saved object to return. Just out of curiosity, where would the URL route you to?

One last question, are the saved objects returned filtered through feature control and sub-feature control settings for the role that is searching?

@ryankeairns
Copy link
Contributor

+1 to creating issues for those we know we want to include sooner rather than later.

gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 7, 2020
* actions/feature:
  improved copy
  [APM] Fix flaky e2e’s (elastic#70790)
  [Discover] Migrate server side saved object from data to discover plugin (elastic#70342)
  [APM] Update docs on running API tests (elastic#70765)
  test: 💍 delete a flaky test (elastic#70785)
  [Security Solution] Refactor GlobalTime to useGlobalTime hook and cle… (elastic#69345)
  Remove IE11 mention from PR template [skip ci] (elastic#70486)
  [GS] add savedObjects result provider (elastic#68619)
  remove snapshot from disabled test suite. (elastic#70769)
@pgayvallet
Copy link
Contributor Author

Out of curiosity, is there a way to test the search API and see what sort of results are returned?

Unfortunately as it's not 'plugged' anywhere yet, the only solution I see to test the returned results would be to directly query the endpoint, with curl, postman or any other tool.

POST http://kibana_instance/internal/global_search/find
payload: { term: 'my search term' }

not that the application results would not be listed, as this would only return the server-side results. But all searchable SO results would be returned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[GS] SavedObject results provider
7 participants