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 #62624

Closed
sulemanof opened this issue Apr 6, 2020 · 6 comments · Fixed by #63484
Closed

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

sulemanof opened this issue Apr 6, 2020 · 6 comments · Fixed by #63484
Assignees
Labels
bug Fixes for quality problems that affect the customer experience Feature:Search Querying infrastructure in Kibana regression

Comments

@sulemanof
Copy link
Contributor

Kibana version: latest

Elasticsearch version: latest

Describe the bug:

The Histogram agg always shows an error message:

image

Steps to reproduce:

  1. Open an area chart
  2. Select a histogram bucket, fill the fields, press apply
  3. An error with text "Unable to retrieve max and min values ..." is shown (file location : src/plugins/data/public/search/aggs/buckets/histogram.ts)

Any additional context:

The problem is related to getter/setter functionality. The actual error is :

Error: UiSettings was not set. at get (http://localhost:5620/bundles/plugin/visualizations/visualizations.plugin.js:106047:23) at SearchSource

which comes from : https://github.com/elastic/kibana/blob/master/src/plugins/data/public/search/search_source/search_source.ts#L374

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@timroes
Copy link
Contributor

timroes commented Apr 7, 2020

@alexwizp Could we first verify this is not happening in 7.7, otherwise we'll need to mark this as a blocker for 7.7.

@lukeelmers lukeelmers changed the title [Regression] Histogram aggreagation always shows an error message [Regression] Histogram aggregation always shows an error message Apr 7, 2020
@lukeelmers lukeelmers added the bug Fixes for quality problems that affect the customer experience label Apr 7, 2020
@alexwizp
Copy link
Contributor

alexwizp commented Apr 7, 2020

@timroes not a blocker for 7.7. Cannot reproduce it on that branch

@alexwizp alexwizp added the v7.8.0 label Apr 7, 2020
@lukeelmers
Copy link
Member

Just caught up with @alexwizp -- He has identified the root cause of this bug, which is that the SearchSource class is exported statically, however it still relies on runtime services. So in some cases when calling fetch there are failures because the service getters it depends on have not been set in the downstream plugin's bundle. (This is yet another reason why moving away from the createGetterSetter pattern is important, because it can create the illusion of code being static when in fact it is not).

There are three possible ways to resolve this:

  1. Move SearchSource into the data.search runtime contract, perhaps under the __LEGACY namespace.
    • Could perhaps use a pattern similar to what we did with aggs where we have a createSearchSource wrapper function which returns an instantiated SearchSource class, with the runtime dependencies already injected.
    • Pro: contract of SearchSource doesn't change for downstream apps
    • Con: apps need to have an explicit dependency on data in their kibana.json and pass SearchSource down to where it is used in their app.
  2. Pass the stateful dependencies to the SearchSource constructor & still export it statically
    • Pro: apps can still have the simplicity of importing SearchSource statically
    • Con: apps will still need to pass in the runtime dependencies for the services SearchSource relies on (uiSettings, injectedMetadata, search service). In the end this doesn't make Option 2 any different from Option 1.
  3. Combination of 1 and 2. SearchSource is exported statically with the stateful services provided via configuration, or you can use the runtime createSearchSource as a convenience.

cc @lukasolson @lizozom in case you have any opinions on this either way.

@lukeelmers lukeelmers added Feature:Search Querying infrastructure in Kibana and removed v7.8.0 labels Apr 7, 2020
alexwizp added a commit to alexwizp/kibana that referenced this issue Apr 13, 2020
@lukeelmers
Copy link
Member

Thinking more about this, I think we should go with Option 1 in my list above (expose SearchSource on runtime contract). Mostly because I don't know why somebody would be needing to use SearchSource as a static import without also needing the underlying search service, so we might as well expose it on the runtime contract.

We could optionally do the refactoring in Option 2, which would improve the readability of the code, but I don't really see a use case for making SearchSource a static export (unless someone else can think of one).

@alexwizp
Copy link
Contributor

@lukeelmers thank you, I also think that it should be in our runtime contract

alexwizp added a commit to alexwizp/kibana that referenced this issue Apr 14, 2020
alexwizp added a commit to alexwizp/kibana that referenced this issue Apr 14, 2020
alexwizp added a commit that referenced this issue Apr 22, 2020
)

* 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>
alexwizp added a commit to alexwizp/kibana that referenced this issue 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 to alexwizp/kibana that referenced this issue 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 issue 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:Search Querying infrastructure in Kibana regression
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants