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] Support storing time with saved searches #138377

Merged
merged 25 commits into from
Aug 23, 2022

Conversation

jughosta
Copy link
Contributor

@jughosta jughosta commented Aug 9, 2022

Closes #9761

Summary

This PR adds support for storing the current time range and refresh interval with saved searches. Once such saved search is opened, it will restore the time filter. Embeddables on Dashboard won't use these data.

Screenshot 2022-08-09 at 13 04 02

Checklist

Delete any items that are not applicable to this PR.

@jughosta jughosta 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. v8.5.0 labels Aug 9, 2022
@jughosta jughosta self-assigned this Aug 9, 2022
label={
<FormattedMessage
id="discover.topNav.saveModal.storeTimeWithSearchFormRowLabel"
defaultMessage="Store time with search"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @gchaps ! We are adding a new toggle to the Save Search modal on Discover page. Do you have suggestions for the copy?

Screenshot 2022-08-09 at 13 04 02

Copy link
Contributor

Choose a reason for hiding this comment

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

How about something like this

Store time with saved search
Update the time filter and refresh interval to the current selection when using this search.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Thanks, @gchaps !

Screenshot 2022-08-11 at 09 31 49

@jughosta
Copy link
Contributor Author

@elasticmachine merge upstream

@jughosta jughosta marked this pull request as ready for review August 10, 2022 13:33
@jughosta jughosta requested a review from a team as a code owner August 10, 2022 13:33
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

@jughosta
Copy link
Contributor Author

@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.

Code LGTM, great work 🎉 ! Tested locally and a-la-carte and works as expected 👍 . One thing I noted but that's not specific to this PR, once you saved there's no good way to check if a saved search stores the time or not. So actually, you've to open and see that it does. This is the same with dashboard. ideally we would have a indicator in the saved search list if a saved search is configured this way ... or a tag that's automatically assigned (now thinking of he who will bring tags to Discover aka @davismcphee )

Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

Code changes LGTM, pulled and tested locally and works great! You'll make some users waiting five years very happy. Also I agree with @kertal about an indicator being a good idea, but also since it's the same in Dashboard I think it's good for now.

@kertal
Copy link
Member

kertal commented Aug 11, 2022

closing an issue with just four digits will not happen too often in the future, #9761. This is like resolving issues of your ancestors, let's spend a minute in silence ... or just don't write anything in slack for a minute

@jughosta
Copy link
Contributor Author

Thanks for the reviews! Going to merge after #138388

…h-search

# Conflicts:
#	src/plugins/discover/public/application/main/components/top_nav/on_save_search.tsx
#	src/plugins/discover/public/application/main/utils/persist_saved_search.ts
#	src/plugins/saved_search/public/services/saved_searches/types.ts
@jughosta
Copy link
Contributor Author

@elasticmachine merge upstream

@jughosta
Copy link
Contributor Author

@elasticmachine merge upstream

@jughosta
Copy link
Contributor Author

@elasticmachine merge upstream

@jughosta
Copy link
Contributor Author

@elasticmachine merge upstream

@rudolf rudolf requested a review from a team as a code owner August 22, 2022 20:38
Copy link
Contributor

@rudolf rudolf left a comment

Choose a reason for hiding this comment

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

The CI failure due to the field limit stems from the way esArchiver creates the .kibana index. It's hard to test these fixes without a PR like this one, so I decided to add a commit directly to this PR which will hopefully solve it.

"routing_partition_size": "1"
"routing_partition_size": "1",
"mapping": {
"total_fields": { "limit": 1500 }
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and in all the other mappings.json files I explicitly set the field limit to 1500. When FTR has an esArchive with the index name .kibana_$KIBANA_PACKAGE_VERSION_001 this creates an index with the current version number like .kibana_8.5.0_001. Because the index is already using the latest version number no migrations are applied and so the migration service will not update the field limit of this index.

So any time esArchiver creates such an "already migrated to the latest version" index we need to explicitly set the field limit. Because most tests don't delete the .kibana index once it exists, but merely "cleans" it by deleting all current saved objects, these archives can break other test suites that didn't even load the archive.

The alternative is to always delete the .kibana index for every test suite, but this is simply too time consuming and adds an incredible amount of time to each CI run.

Copy link
Member

Choose a reason for hiding this comment

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

thx @rudolf for taking care of this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @rudolf!

@kertal
Copy link
Member

kertal commented Aug 23, 2022

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
discover 525 527 +2

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 73 76 +3
savedSearch 39 42 +3
total +6

Async chunks

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

id before after diff
discover 461.1KB 462.7KB +1.6KB

Page load bundle

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

id before after diff
savedSearch 4.7KB 4.9KB +192.0B

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/saved-objects-service.html#_mappings

id before after diff
search 15 22 +7
Unknown metric groups

API count

id before after diff
discover 90 93 +3
savedSearch 39 42 +3
total +6

History

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

cc @jughosta

Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

Changes in SO/Spaces API integration tests LGTM.

@jughosta jughosta merged commit 1a70f6f into elastic:main Aug 23, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Aug 23, 2022
Mpdreamz pushed a commit to Mpdreamz/kibana that referenced this pull request Sep 6, 2022
* [Discover] Implement UI for storing time with a saved search

* [Discover] Save time range data with a saved search

* [Discover] Improve updating of values

* [Discover] Restore time range after loading a saved search

* [Discover] Add time range validation

* [Discover] Add refresh interval validation

* [Discover] Update how saved search gets restored

* [Discover] Improve tests

* [Discover] Update tests

* [Discover] Improve type imports

* [Discover] Update copy

* [Discover] Fix types after the merge

* [Discover] Update test name

* [Discover] Fix types

* [Discover] Update mapping

* [Discover] Update mapping

* Explicitly set field limit for .kibana_ esArchives

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Rudolf Meijering <[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. v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Store time with saved searches
9 participants