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.

Sign up for GitHub

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

Jump to bottom

[data.query] Add getState() api to retrieve whole QueryState #132035

Merged
merged 6 commits into from
May 20, 2022
Merged

[data.query] Add getState() api to retrieve whole QueryState #132035

merged 6 commits into from
May 20, 2022

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented May 11, 2022 •
edited
Loading

Summary

Close #129487

This is an API enhancements that exposes a single API for retrieving whole QueryState without a need to use filterManager timeFilter queryStringManager separately. We are adding this for USaFe initiative.

Also adds a GlobalQueryStateFromUrl interface to decouple _g query param from the URL state interface from the more generic QueryState which might change soon. See the discussion #132035 (comment)

@Dosant Dosant changed the title D/2022 05 11 query state [data.query] Add getState() api to retrieve whole QueryState May 12, 2022
@Dosant Dosant requested a review from ppisljar May 12, 2022 10:37
@Dosant Dosant added Team:AppServicesSv release_note:skip Skip the PR/issue when compiling release notes v8.3.0 labels May 12, 2022
ppisljar
ppisljar reviewed May 12, 2022
View reviewed changes
src/plugins/data/public/query/query_state.ts
queryString: QueryStringContract;
}): QueryState {
return {
time: timefilter.getTime(),
Copy link
Member

Choose a reason for hiding this comment

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

can we name this timeRange

Copy link
Contributor Author

@Dosant Dosant May 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This would be problematic because QueryState is existing interface we use with URL syncing and time is how this key is preserved in URLs.

I think to make it timeRange we will have to create a separate interface and keep both around. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

i think it makes sense to decouple the two, we'll be modifying this in the short future for universal search purposes and it sounds we'll be limited there if we keep the coupling

Copy link
Member

Choose a reason for hiding this comment

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

but we can wait with that until we need to do some changes that would be incompatible, for the first step i think this should be fine. what do you think @flash1293

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, not sure why we want to decouple them - it seems like using the same interface for state syncing and communication between unified search and apps would be fine. Is there something I'm missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current QueryState interface reflects a state in the URL (partially _g and _a portion of it), so must be very careful with what we change there.
We agreed with Peter, that would be safer to decouple and be explicit about what interface describes a state in the URL and what is for internal data transfer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, didn't consider that. Also, no objections for this being called time for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with a "partial" decoupling so in case the states state deviates, we will catch it on typescript level and can adjust then 30e9c66

I introduced GlobalQueryStateFromUrl state which for now matches QueryState. Everywhere where QueryState was used in the URL context I replaced it with GlobalQueryStateFromUrl

ppisljar
ppisljar reviewed May 12, 2022
View reviewed changes
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

i think its fine if on QueryState all the keys are optional (for example i think there is high chance that is esql we won't be sending in any filters or when you have non-time based index there is no time range)

@Dosant Dosant force-pushed the d/2022-05-11-query-state branch from d991700 to ea51e2c Compare May 12, 2022 13:46
@Dosant Dosant requested a review from ppisljar May 12, 2022 13:47
@Dosant
Copy link
Contributor Author

Dosant commented May 16, 2022

@elasticmachine merge upstream

@Dosant
Copy link
Contributor Author

Dosant commented May 18, 2022

@elasticmachine merge upstream

ppisljar
ppisljar approved these changes May 18, 2022
View reviewed changes
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

LGTM

@Dosant Dosant marked this pull request as ready for review May 18, 2022 15:39
@Dosant Dosant requested review from a team as code owners May 18, 2022 15:39
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServicesSv)

@qn895
Copy link
Member

qn895 commented May 18, 2022

ML changes LGTM

ThomThomson
ThomThomson approved these changes May 18, 2022
View reviewed changes
Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

Presentation team type changes LGTM!

nreese
nreese approved these changes May 18, 2022
View reviewed changes
Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

kibana-gis changes LGTM
code review

kertal
kertal approved these changes May 19, 2022
View reviewed changes
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, just a change of type in Discover locator

@Dosant
Copy link
Contributor Author

Dosant commented May 19, 2022

@elasticmachine merge upstream

flash1293
flash1293 approved these changes May 19, 2022
View reviewed changes
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.

Code LGTM, only type change in VisEditors code

@Dosant
Copy link
Contributor Author

Dosant commented May 20, 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
data 507 508 +1

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
data 2863 2869 +6

Page load bundle

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

id before after diff
data 414.3KB 414.4KB +173.0B
Unknown metric groups

API count

id before after diff
data 3475 3482 +7

References to deprecated APIs

id before after diff
dashboard 70 74 +4
discover 32 34 +2
lens 18 20 +2
maps 305 307 +2
monitoring 2 4 +2
visualizations 11 13 +2
total +14

History

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

@Dosant Dosant merged commit f88b140 into elastic:main May 20, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qn895 qn895 qn895 approved these changes

@ThomThomson ThomThomson ThomThomson approved these changes

@ppisljar ppisljar ppisljar approved these changes

@flash1293 flash1293 flash1293 approved these changes

@kertal kertal kertal approved these changes

@nreese nreese nreese approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes v8.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[data.query] expose API to retrieve whole QueryState
10 participants
@Dosant @elasticmachine @qn895 @kibana-ci @nreese @kertal @flash1293 @ppisljar @ThomThomson @kibanamachine