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

[Discover] Enable tags for saved searches #136162

Merged

Conversation

davismcphee
Copy link
Contributor

@davismcphee davismcphee commented Jul 11, 2022

Summary

This PR adds support for tagging saved searches in Discover. Users can add tags to new or existing searches through the save modal, and a new saved object finder component with support for filtering by tags was added to the open search flyout.

A-la-carte deployment: https://davismcphee-pr-136162-discover-feature-saved-search-tags.kbndev.co

Resolves #117832.

Notes

  • Originally the POC for this work modified the common saved object finder component used throughout Kibana, but this created circular dependencies, so I added a separate saved_objects_finder plugin instead.
  • I also did a pass at updating each area that uses the existing saved object finder to use the new plugin (see [POC] Share new saved object finder across plugins #139317 for POC), but this created so many test failures that after discussing with the Data Discovery team, we felt it would be best to use the new finder only in Discover for now, and allow other areas to adopt it when they need tagging support. Both finders support the same props and features apart from tagging, so it shouldn't be hard to migrate, but in the interest of time and not breaking everyone's stuff, I didn't include any of that in this PR.
  • I noticed some of the tagging functional tests had been marked flaky, so I did a flaky test build too to make sure my new ones were ok: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1051.

Examples

Save modal:
save_modal

The new finder is based on the Saved Objects screen in Stack Management, so a screenshot for reference:
saved_objects_screen

With no filters:
no_filter

With tag filters:
tag_filter

Discover's flyout only supports saved searches, but the new finder supports multiple types of saved objects, and will only show the type column and filter if there are multiple types configured:
multiple_types

Mobile view:
mobile_view

Checklist

For maintainers

Sorry, something went wrong.

@davismcphee davismcphee added Feature:Discover Discover Application release_note:feature Makes this part of the condensed release notes Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. labels Jul 11, 2022
@davismcphee davismcphee self-assigned this Jul 11, 2022
@davismcphee davismcphee force-pushed the discover-feature-saved-search-tags branch from 49738da to 1eb25c4 Compare July 14, 2022 14:17
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

This look great from Discover side ❤️ , don't see a reason why not moving forward here. Only thing I was wondering, if it would make sense to give the tags column less width than the name column, since the name is more significant and high likely longer than the tags ... however ... I might be wrong .. it's also fine the way it's currently handled 👍

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

Let me start by saying that the team is quite excited to have the tagging feature (finally) be implemented on a new SO type. Thanks a lot for taking a shot at this!

Now, I have multiple remarks:

1. new taggable type registration

There are some missing steps that needs to be done to fully integrate with SOT, as described in x-pack/plugins/saved_objects_tagging/README.md:

  • Add the new SO type to the list of taggable types

export const taggableTypes = ['dashboard', 'visualization', 'map', 'lens'];

  • Update the SOT telemetry collector schema to add the new type

types: {
dashboard: perTypeSchema,
lens: perTypeSchema,
visualization: perTypeSchema,
map: perTypeSchema,
},

  • Add read-access to the tag SO type to your feature's capabilities

See

In order to be able to fetch the tags assigned to an object, the user must have read permission
for the `tag` saved object type. Which is why all features relying on SO tagging must update
their capabilities.

Also, having a FTR suite to cover the integration for the new type would be amazing, e.g

2. Integration with the SO finder component

I have no objection adding tagging support to the SO finder component, overall I think it would be a valuable addition.

The thing that bothers me more is the explicit, cyclic dependency it introduces between the savedObjects and savedObjectTaggingOss plugin (see comments). Let's discuss it, but overall the only viable option I see to break this dependency is to have the finder component be extracted out of the savedObjects plugin. That way, this new plugin (and later, package) would be able to depend on both SO and SOT to properly break this cyclic dependency.

Please tell me what you think.

src/plugins/saved_objects_management/public/index.ts Outdated Show resolved Hide resolved
x-pack/plugins/transform/public/app/app_dependencies.tsx Outdated Show resolved Hide resolved
x-pack/plugins/transform/public/plugin.ts Outdated Show resolved Hide resolved
src/plugins/saved_objects/kibana.json Outdated Show resolved Hide resolved
src/plugins/saved_objects/public/plugin.ts Outdated Show resolved Hide resolved
src/plugins/saved_objects/public/plugin.ts Outdated Show resolved Hide resolved
src/plugins/saved_objects/public/services.ts Outdated Show resolved Hide resolved
@alexfrancoeur
Copy link

@sebelga @lukeelmers @clintandrewhall, @davismcphee pinged our #content-management channel on this one. It feels to me that tags are very much aligned with content management and shared UX and it'd be great to have a consistent implementation here. We want to move more towards a standard schema so that any SO can simply plug into all of the amazing new things we're going to build. Is there a chance one of you, or someone on your team, can provide some feedback on the implementation?

@alexfrancoeur
Copy link

Hey @davismcphee I spoke with @ghudgins about this briefly. As long as tags are implemented the same as other saved objects, I view the addition to the SO flyout as an iterative improvement from the product perspective. It looks like @pgayvallet has added some comments on the implementation. As long as any of his concerns are addressed, I'm +1 on the product side. Thanks for reaching out, there's definitely overlap with content management but this feels additive to me 👍

@davismcphee davismcphee force-pushed the discover-feature-saved-search-tags branch from c2de1f3 to f867704 Compare August 2, 2022 19:29
@davismcphee davismcphee force-pushed the discover-feature-saved-search-tags branch 4 times, most recently from 09e6d14 to 3024e20 Compare August 17, 2022 19:26
@davismcphee davismcphee force-pushed the discover-feature-saved-search-tags branch from 4026e8a to 95473af Compare August 23, 2022 17:39
davismcphee and others added 14 commits August 23, 2022 15:49

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…o support tags

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…h flyout

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@kertal kertal requested a review from pgayvallet August 25, 2022 06:09
@kertal
Copy link
Member

kertal commented Aug 29, 2022

@elasticmachine merge upstream

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Done the Code review, and added obervations, no blocking stuff in sigh, tested manually locally, works as expected in Discover 👍 Will add a final review round once relevant changes is are addressed, and open questions about Codeownership are clarified!

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks again for wiring the tagging feature to searches!

A few nits and comments, feel free to ignore any or all of them.

src/plugins/saved_objects_finder/kibana.json Show resolved Hide resolved
Comment on lines +11 to +13
export { SavedObjectFinder } from './finder';

export const plugin = () => new SavedObjectsFinderPublicPlugin();
Copy link
Contributor

Choose a reason for hiding this comment

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

A perfect example of a 'fake' plugin that could be migrated to a package once its dependencies are.

src/plugins/visualizations/public/services.ts Outdated Show resolved Hide resolved
x-pack/plugins/transform/public/app/app_dependencies.tsx Outdated Show resolved Hide resolved
x-pack/plugins/transform/public/plugin.ts Outdated Show resolved Hide resolved
@@ -25,5 +25,6 @@ export default function ({ loadTestFile, getService }: FtrProviderContext) {
loadTestFile(require.resolve('./dashboard_integration'));
loadTestFile(require.resolve('./feature_control'));
loadTestFile(require.resolve('./maps_integration'));
loadTestFile(require.resolve('./discover_integration'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for this

- Type imports cleanup
- Added loading indicator for saved object finder
- Changed `savedObjectsPlugin.settings.getListingLimit()` to `uiSettings.get(LISTING_LIMIT_SETTING)`
- Removed old saved object finder comment
- Removed tag changes from transform plugin
- Change code owners of saved_objects_finder to Data Discovery

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@davismcphee
Copy link
Contributor Author

Thanks for all the feedback! PR has been updated based on the suggestions and should be good to review @kertal.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
savedObjectsFinder - 6 +6

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
discover 76 77 +1
savedObjectsFinder - 16 +16
savedObjectsManagement 112 117 +5
savedSearch 42 43 +1
total +23

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
discover 462.5KB 463.1KB +655.0B
savedObjectsFinder - 4.9KB +4.9KB
visualizations 197.6KB 197.7KB +132.0B
total +5.6KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
discover 25.8KB 26.1KB +337.0B
savedObjectsFinder - 3.1KB +3.1KB
savedObjectsManagement 19.2KB 19.3KB +68.0B
savedSearch 4.9KB 5.1KB +172.0B
visualizations 47.1KB 47.3KB +146.0B
total +3.8KB
Unknown metric groups

API count

id before after diff
discover 93 94 +1
savedObjectsFinder - 16 +16
savedObjectsManagement 125 130 +5
savedSearch 42 43 +1
total +23

async chunk count

id before after diff
savedObjectsFinder - 1 +1

ESLint disabled line counts

id before after diff
savedObjectsFinder - 1 +1

Total ESLint disabled count

id before after diff
savedObjectsFinder - 1 +1

History

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

cc @davismcphee

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

LGTM 👍 , great work Davis, had another look, all comments resolved. Tested locally, works as expected! Hurray for tagging in Discover!

@davismcphee davismcphee merged commit cb0a805 into elastic:main Sep 1, 2022
@davismcphee davismcphee deleted the discover-feature-saved-search-tags branch September 1, 2022 16:14
@kibanamachine kibanamachine added v8.5.0 backport:skip This commit does not require backporting labels Sep 1, 2022
Mpdreamz pushed a commit to Mpdreamz/kibana that referenced this pull request Sep 6, 2022
* [Discover] Add initial support for tags to saved search modal

* [Discover] Add tags to savedSearch types

* [Discover] Finish initial support for adding tags to saved searches

* [Discover] Start to convert saved object finder to a table in order to support tags

* [Discover] Add support for displaying saved search tags in open search flyout

* [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix'

* [Discover] Continue support for tags in saved object finder

* [Discover] Clean up saved object finder

* [Discover] Finish initial support for tags in saved object finder

* [Discover] Update SimpleSavedObject constructor to SimpleSavedObjectImpl

* [Discover] Remove orig files

* [Discover] Saved search tag type registration and telemetry

* [Discover] Create new saved_objects_finder plugin

* [Discover] Continue work creating saved_objects_finder plugin

* [Discover] Revert some changes in saved_objects

* [Discover] Revert some changes in saved_objects again

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* [Discover] Update saved_objects_finder i18n keys

* [Discover] Update docs

* [Discover] Add plugins to saved_object_finder and fix broken types

* [Discover] Finish creating saved_objects_finder plugin and use it in Discover

* [Discover] Update SavedObjectFinderProps type export, and update x-pack telemetry

* [Discover] Fix broken jest tests

* [Discover] Update saved_objects_finder API

* [Discover] Remove unused translations

* [Discover] Fix issue with initial saved object finder fetch

* [Discover] Fix some of the saved object finder jest tests

* [Discover] Clean up finder after merge

* [Discover] Fixing saved_object_finder.tsx

* [Discover] Add savedObjectsTaggingOss reference to saved_search plugin

* [Discover] Fix broken open_search_panel test

* [Discover] Removed allowed types from saved object finder

* [Discover] Removing type column when there's only one saved object type, and adjusting column widths

* [Discover] Fix issue where visible types were entirely removed, fixed pageSizeOptions

* [Discover] Add showFilter to open_search_panel's saved_objects_finder, fallback to all available types when no type filter is applied to saved_objects_finder, hide type column and filter button when there is only one type in metadata list

* [Discover] Fix remaining saved_object_finder Jest tests

* [Discover] Update snapshot

* [Discover] Fix failing functional tests

* [Discover] Add tagging Jest tests for saved_objects_finder

* [Discover] Fix small bugs in saved_object_finder, update Jest tests, add functional tests for Discover saved search tagging

* [Discover] Removed unused variable in functional test

* [Discover] Update Discover Jest tests with tagging tests

* [Discover] Remove translations

* [Discover] Updating saved_objects_finder to use static export vs preconfigured component, adding lazy load support

* [Discover] Move saved_object_finder service deps to a 'services' prop, fix broken open_search_panel Jest test

* [Discover] Fix broken Jest test

* [Discover] Fix broken Jest test from merge

* [Discover] Fix discover tags integration test description

* - Updated tags prop to be `string | undefined`
- Type imports cleanup
- Added loading indicator for saved object finder
- Changed `savedObjectsPlugin.settings.getListingLimit()` to `uiSettings.get(LISTING_LIMIT_SETTING)`
- Removed old saved object finder comment
- Removed tag changes from transform plugin
- Change code owners of saved_objects_finder to Data Discovery

* [Discover] Fixed LISTING_LIMIT_SETTING import

* [Discover] Revert tags saving change that introduced a bug

* [CI] Auto-commit changed files from 'node scripts/build_plugin_list_docs'

* [Discover] Try again to fix LISTING_LIMIT_SETTINGS import

* [Discover] Fix failing snapshot

Co-authored-by: kibanamachine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Discover Discover Application release_note:feature Makes this part of the condensed release notes Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. Team:Fleet Team label for Observability Data Collection Fleet team v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Discover] Enable tags for saved searches
10 participants