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

[Regression] Histogram aggregation always shows an error message #63484

Merged
merged 36 commits into from
Apr 22, 2020

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Apr 14, 2020

Closes: #62624

Summary

this PR closes a couple of issues related to SearchSource class (public/search/search_source/search_source.ts).

We shouldn't use getters/getters approach for static contract. This PR solves this problem, and now we pass all the necessary services directly from all places where SearchSource should be created.

Important note:
We cannot add SearchSource to the search runtime contract because the searchService is one of the required dependencies here.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@elastic elastic deleted a comment from kibanamachine Apr 15, 2020
@elastic elastic deleted a comment from kibanamachine Apr 15, 2020
@alexwizp alexwizp requested review from lukeelmers and lizozom April 15, 2020 17:19
@elastic elastic deleted a comment from kibanamachine Apr 16, 2020
@elastic elastic deleted a comment from kibanamachine Apr 17, 2020
@alexwizp alexwizp removed the request for review from a team April 22, 2020 09:27
@elastic elastic deleted a comment from kibanamachine Apr 22, 2020
@alexwizp alexwizp requested a review from lizozom April 22, 2020 09:38
@alexwizp alexwizp changed the title [WIP][Regression] Histogram aggregation always shows an error message [Regression] Histogram aggregation always shows an error message Apr 22, 2020
@alexwizp alexwizp removed the WIP Work in progress label Apr 22, 2020
@alexwizp alexwizp requested a review from flash1293 April 22, 2020 09:38
Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

Tested and LGTM
Bug is not reproduced anymore.
@ppisljar mind throwing a quick look

fieldFormats: fieldFormatsServiceMock.createStartContract(),
notifications: notificationServiceMock.createStartContract(),
});
const getInternalStartServices: GetInternalStartServicesFn = () =>
Copy link
Contributor

Choose a reason for hiding this comment

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

//nit
I see this block repeating in every agg metric - maybe extract it for future flexibility

private searchStrategyId?: string;
private parent?: SearchSource;
private requestStartHandlers: Array<
(searchSource: ISearchSource, options?: FetchOptions) => Promise<unknown>
(searchSource: SearchSource, options?: FetchOptions) => Promise<unknown>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this change from ISearchSource?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SearchSource is a correct type here

# Conflicts:
#	x-pack/plugins/maps/public/layers/sources/es_source/es_source.d.ts
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

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.

Reviewed Kibana app code and tested Visualize (input control vis, histogram bar chart) and Discover - didn't notice any problems. LGTM 👍

@alexwizp alexwizp added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:fix labels Apr 22, 2020
@alexwizp alexwizp merged commit 0a6da70 into elastic:master Apr 22, 2020
alexwizp added a commit to alexwizp/kibana that referenced this pull request Apr 22, 2020
…stic#63484)

* WIP [Regression] Histogram aggregation always shows an error message

Closes: elastic#62624

* make getInternalStartServices private

* fix ts issues

* remove createSearchSource from static contract

* fix some jest test

* move searh_source to static contract

* fix types

* fix function tests

* fix jest / add createStartServicesGetter

* fix comments: saved_object_management

* maps: fix PR comments

* maps: update types

* fix heck_published_api_changes

* move searchSource into runtime contract

* cleanup

* fix ts error

* cleanup

* remove extra dependencies

* fix Discover

* fix Discover JEST

* fix PR comments

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
# Conflicts:
#	src/plugins/data/public/public.api.md
alexwizp added a commit that referenced this pull request Apr 22, 2020
) (#64211)

* WIP [Regression] Histogram aggregation always shows an error message

Closes: #62624

* make getInternalStartServices private

* fix ts issues

* remove createSearchSource from static contract

* fix some jest test

* move searh_source to static contract

* fix types

* fix function tests

* fix jest / add createStartServicesGetter

* fix comments: saved_object_management

* maps: fix PR comments

* maps: update types

* fix heck_published_api_changes

* move searchSource into runtime contract

* cleanup

* fix ts error

* cleanup

* remove extra dependencies

* fix Discover

* fix Discover JEST

* fix PR comments

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
# Conflicts:
#	src/plugins/data/public/public.api.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:NP Migration Feature:Search Querying infrastructure in Kibana release_note:skip Skip the PR/issue when compiling release notes v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Regression] Histogram aggregation always shows an error message
7 participants