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 tag and dashboard suggestion results #85144

Merged
merged 11 commits into from
Dec 9, 2020

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Dec 7, 2020

Summary

Fix #83888

PR Adds 'filter by tag' and 'filter by type' suggestions when the user searches for a term matching a search type or a tag name.

Screenshots

Screenshot 2020-12-07 at 16 11 25

Screenshot 2020-12-07 at 16 11 14

Checklist

Delete any items that are not applicable to this PR.

@pgayvallet
Copy link
Contributor Author

@ryankeairns @alexfrancoeur the PR is still in draft, but I think it is functionally complete. Could you take a quick look see if everything is looking good?

Some remarks:

  • I can't easily retrieve the icon associated with a type for the filter by type suggestion. We will need to go with a static icon that doesn't depend on the suggested type (dashboard, vis, etc). I wasn't sure which icon was best here. Could you tell me which one to use?

  • I did as much as I could regarding the option display / rendering. I can't change the go to label to apply for example.

  • Now that we have this, do we want to remove the 'Filter by type or tag' in the footer? I think having both is fine.

@ryankeairns
Copy link
Contributor

@ryankeairns @alexfrancoeur the PR is still in draft, but I think it is functionally complete. Could you take a quick look see if everything is looking good?

Some remarks:

  • I can't easily retrieve the icon associated with a type for the filter by type suggestion. We will need to go with a static icon that doesn't depend on the suggested type (dashboard, vis, etc). I wasn't sure which icon was best here. Could you tell me which one to use?
  • I did as much as I could regarding the option display / rendering. I can't change the go to label to apply for example.
  • Now that we have this, do we want to remove the 'Filter by type or tag' in the footer? I think having both is fine.

I just did a quick pass, this is looking really nice!

  • The only thing I've noticed thus far is that the tag names seem to be case sensitive (e.g. I tested with 'Ryan).

In response to your questions:

  1. Icon for type: - We've used savedObjectsApp in some places for generically referring to objects, but that may not be accurate here (?). The other options that come to mind are empty or apps until we can possibly better determine the true type. Do any of those resonate with you?
  2. The display looks good, generally speaking. @myasonik , did the latest changes to EuiSelectableSitewideTemplate enable us to change the 'Go to' badge label, or should I create a separate issue to make that customizable?
  3. I agree with having both. The footer text helps to increase discoverability of the syntax.

Thanks for working on this, its quite helpful.

@alexfrancoeur
Copy link

alexfrancoeur commented Dec 7, 2020

I can't easily retrieve the icon associated with a type for the filter by type suggestion. We will need to go with a static icon that doesn't depend on the suggested type (dashboard, vis, etc). I wasn't sure which icon was best here. Could you tell me which one to use?

I'll defer to Ryan here, but I'm +1 on a static icon to simplify

I did as much as I could regarding the option display / rendering. I can't change the go to label to apply for example.

This feels like a nice to have to me. Technically, you're still going to a filtered list?

Now that we have this, do we want to remove the 'Filter by type or tag' in the footer? I think having both is fine.

I agree, a constant reminder of filter types might be useful.

The PR is looking great! A few quick observations and questions below

Does exact match have to be case sensitive? Because it's an exact match, I'm guessing yes, but thought I'd post the question.

image

image

I think there might be some issues filtering on tags with white space by using the default suggestion. Adding quotes around the string will provide the appropriate filter

image

image

image

@pgayvallet
Copy link
Contributor Author

Does exact match have to be case sensitive? Because it's an exact match, I'm guessing yes, but thought I'd post the question.

Na, just forget to make the match case-insensitive. Will fix.

I think there might be some issues filtering on tags with white space by using the default suggestion. Adding quotes around the string will provide the appropriate filter

Yea, already have a TODO for this 😄

suggestedSearch: `type:${searchTerm}`, // TODO: escape if necessary

@myasonik
Copy link
Contributor

myasonik commented Dec 7, 2020

The display looks good, generally speaking. @myasonik , did the latest changes to EuiSelectableSitewideTemplate enable us to change the 'Go to' badge label, or should I create a separate issue to make that customizable?

I didn't cover it in the previous PR because the badge question is a bigger effort to fix and has a few technical issues in the way of implementing it. I meant to open a ticket for it earlier but forgot to. Done now!
elastic/eui#4361

@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.11.0 v8.0.0 labels Dec 8, 2020
@pgayvallet pgayvallet marked this pull request as ready for review December 8, 2020 11:31
@pgayvallet pgayvallet requested a review from a team December 8, 2020 11:31
@pgayvallet pgayvallet requested a review from a team as a code owner December 8, 2020 11:31
@pgayvallet pgayvallet requested a review from a team December 8, 2020 11:31
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

KibanaApp changes LGTM, code review only

Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

Works as expected; tested the whitespace and case fixes.
The filter icon makes sense, as well 🥇

I can't wait to hear the feedback, this search feature is really powerful.

@MichaelMarcialis
Copy link
Contributor

  1. The other options that come to mind are empty or apps until we can possibly better determine the true type. Do any of those resonate with you?

+1. I like the idea of using apps as the static icon for type filter suggestions.

Copy link

@alexfrancoeur alexfrancoeur left a comment

Choose a reason for hiding this comment

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

It's all coming together, this looks fantastic. Whitespace and case sensitivity tested on my end. Thanks Pierre, this is amazing! :shipit:

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

@pgayvallet is a global search machiiine! The implementation looks great, only small question/suggestion.

Comment on lines 58 to 61
return typeRegistry
.getVisibleTypes()
.filter((type) => type.management?.defaultSearchField && type.management?.getInAppUrl)
.map((type) => type.name);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we DRY this up with lines 26-29 to ensure the implementations of these two functions are consistent?

@kibanamachine
Copy link
Contributor

💛 Build succeeded, but was flaky


Test Failures

Plugin Functional Tests.x-pack/test/plugin_api_integration/test_suites/task_manager/health_route·ts.task_manager health should return a breakdown of idleTasks in the task manager workload

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has not failed recently on tracked branches

[00:00:00]       │
[00:00:00]         └-: task_manager
[00:00:00]           └-> "before all" hook
[00:00:00]           └-: health
[00:00:00]             └-> "before all" hook
[00:00:00]             └-> should return basic configuration of task manager
[00:00:00]               └-> "before each" hook: global before each
[00:00:00]               └- ✓ pass  (14ms) "task_manager health should return basic configuration of task manager"
[00:00:00]             └-> should return the task manager workload
[00:00:00]               └-> "before each" hook: global before each
[00:00:02]               │ info [o.e.c.m.MetadataCreateIndexService] [kibana-ci-immutable-centos-tests-xxl-1607500060156011205] [.ds-ilm-history-5-000001] creating index, cause [initialize_data_stream], templates [ilm-history], shards [1]/[0]
[00:00:02]               │ info [o.e.c.m.MetadataCreateDataStreamService] [kibana-ci-immutable-centos-tests-xxl-1607500060156011205] adding data stream [ilm-history-5] with write index [.ds-ilm-history-5-000001] and backing indices []
[00:00:03]               │ info [o.e.x.i.IndexLifecycleTransition] [kibana-ci-immutable-centos-tests-xxl-1607500060156011205] moving index [.ds-ilm-history-5-000001] from [null] to [{"phase":"new","action":"complete","name":"complete"}] in policy [ilm-history-ilm-policy]
[00:00:03]               │ info [o.e.c.r.a.AllocationService] [kibana-ci-immutable-centos-tests-xxl-1607500060156011205] current.health="GREEN" message="Cluster health status changed from [YELLOW] to [GREEN] (reason: [shards started [[.ds-ilm-history-5-000001][0]]])." previous.health="YELLOW" reason="shards started [[.ds-ilm-history-5-000001][0]]"
[00:00:03]               │ info [o.e.x.i.IndexLifecycleTransition] [kibana-ci-immutable-centos-tests-xxl-1607500060156011205] moving index [.ds-ilm-history-5-000001] from [{"phase":"new","action":"complete","name":"complete"}] to [{"phase":"hot","action":"unfollow","name":"wait-for-indexing-complete"}] in policy [ilm-history-ilm-policy]
[00:00:03]               │ info [o.e.x.i.IndexLifecycleTransition] [kibana-ci-immutable-centos-tests-xxl-1607500060156011205] moving index [.ds-ilm-history-5-000001] from [{"phase":"hot","action":"unfollow","name":"wait-for-indexing-complete"}] to [{"phase":"hot","action":"unfollow","name":"wait-for-follow-shard-tasks"}] in policy [ilm-history-ilm-policy]
[00:00:03]               │ info [o.e.c.m.MetadataCreateIndexService] [kibana-ci-immutable-centos-tests-xxl-1607500060156011205] [.kibana_task_manager_test_result] creating index, cause [auto(bulk api)], templates [], shards [1]/[1]
[00:00:03]               │ info [o.e.c.m.MetadataMappingService] [kibana-ci-immutable-centos-tests-xxl-1607500060156011205] [.kibana_task_manager_test_result/6-dICffzRdGUHQxhDg-3xQ] create_mapping
[00:00:05]               └- ✓ pass  (5.1s) "task_manager health should return the task manager workload"
[00:00:05]             └-> should return a breakdown of idleTasks in the task manager workload
[00:00:05]               └-> "before each" hook: global before each
[00:00:05]               └- ✖ fail: task_manager health should return a breakdown of idleTasks in the task manager workload
[00:00:05]               │       Error: expected 19 to sort of equal 20
[00:00:05]               │       + expected - actual
[00:00:05]               │ 
[00:00:05]               │       -19
[00:00:05]               │       +20
[00:00:05]               │       
[00:00:05]               │       at Assertion.assert (/dev/shm/workspace/parallel/3/kibana/packages/kbn-expect/expect.js:100:11)
[00:00:05]               │       at Assertion.eql (/dev/shm/workspace/parallel/3/kibana/packages/kbn-expect/expect.js:244:8)
[00:00:05]               │       at Context.<anonymous> (test/plugin_api_integration/test_suites/task_manager/health_route.ts:162:61)
[00:00:05]               │       at Object.apply (/dev/shm/workspace/parallel/3/kibana/packages/kbn-test/src/functional_test_runner/lib/mocha/wrap_function.js:84:16)
[00:00:05]               │ 
[00:00:05]               │ 

Stack Trace

Error: expected 19 to sort of equal 20
    at Assertion.assert (/dev/shm/workspace/parallel/3/kibana/packages/kbn-expect/expect.js:100:11)
    at Assertion.eql (/dev/shm/workspace/parallel/3/kibana/packages/kbn-expect/expect.js:244:8)
    at Context.<anonymous> (test/plugin_api_integration/test_suites/task_manager/health_route.ts:162:61)
    at Object.apply (/dev/shm/workspace/parallel/3/kibana/packages/kbn-test/src/functional_test_runner/lib/mocha/wrap_function.js:84:16) {
  actual: '19',
  expected: '20',
  showDiff: true
}

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
globalSearch 23 24 +1
globalSearchBar 35 37 +2
total +3

Distributable file count

id before after diff
default 47739 47742 +3

Page load bundle

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

id before after diff
globalSearch 17.0KB 19.8KB +2.8KB
globalSearchBar 43.4KB 48.6KB +5.2KB
globalSearchProviders 8.8KB 8.8KB +79.0B
lens 53.1KB 53.2KB +39.0B
savedObjectsTagging 48.8KB 49.1KB +305.0B
savedObjectsTaggingOss 8.4KB 8.5KB +95.0B
total +8.5KB

History

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

@pgayvallet pgayvallet merged commit 73fbf2a into elastic:master Dec 9, 2020
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Dec 9, 2020
* initial draft

* polish

* fix mocks

* add tests

* tests on suggestions

* add comment

* add FTR tests

* factorize getSearchableTypes

* move to bottom
pgayvallet added a commit that referenced this pull request Dec 9, 2020
* initial draft

* polish

* fix mocks

* add tests

* tests on suggestions

* add comment

* add FTR tests

* factorize getSearchableTypes

* move to bottom
jloleysens added a commit to jloleysens/kibana that referenced this pull request Dec 9, 2020
…k-field-to-hot-phase

* 'master' of github.com:elastic/kibana: (429 commits)
  simplify popover open state logic (elastic#85379)
  [Logs UI][Metrics UI] Move actions to the kibana header (elastic#84648)
  [Search Source] Do not pick scripted fields if * provided (elastic#85133)
  [Search] Session SO polling (elastic#84225)
  [Transform] Replace legacy elasticsearch client (elastic#84932)
  [Uptime]Refactor header and action menu (elastic#83779)
  Fix agg select external link (elastic#85380)
  [ILM] Show forcemerge in hot when rollover is searchable snapshot is enabled (elastic#85292)
  clear using keyboard (elastic#85042)
  [GS] add tag and dashboard suggestion results (elastic#85144)
  [ML] API integration tests - skip GetAnomaliesTableData
  Add ECS field for event.code. (elastic#85109)
  [Functional][TSVB] Wait for markdown textarea to be cleaned (elastic#85128)
  skip flaky suite (elastic#62060)
  skip flaky suite (elastic#85098)
  Bump highlight.js to v9.18.5 (elastic#84296)
  Add `server.publicBaseUrl` config (elastic#85075)
  [Alerting & Actions ] More debug logging (elastic#85149)
  [Security Solution][Case] Manual attach alert to a case (elastic#82996)
  Loosen UUID regex to accept uuidv1 or uuidv4 (elastic#85338)
  ...

# Conflicts:
#	x-pack/plugins/index_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.helpers.tsx
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/hot_phase/hot_phase.tsx
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared_fields/index.ts
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/warm_phase/warm_phase.tsx
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/edit_policy.tsx
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/i18n_texts.ts
#	x-pack/plugins/index_lifecycle_management/server/routes/api/policies/register_create_route.ts
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.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[GS] add tag:xxx or type:xxx filter suggestion option
9 participants