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

KQL/Kuery support for TSVB, timelion and Vega visualizations #17722

Closed
stacey-gammon opened this issue Apr 16, 2018 · 13 comments
Closed

KQL/Kuery support for TSVB, timelion and Vega visualizations #17722

stacey-gammon opened this issue Apr 16, 2018 · 13 comments
Assignees
Labels
Feature:Dashboard Dashboard related features Feature:Timelion Timelion app and visualization Feature:TSVB TSVB (Time Series Visual Builder) Feature:Vega Vega visualizations Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@stacey-gammon
Copy link
Contributor

stacey-gammon commented Apr 16, 2018

Currently none of the above visualizations listen for KQL/Kuery queries, if that feature is enabled.

screen shot 2018-04-16 at 1 08 43 pm

screen shot 2018-04-16 at 1 09 21 pm

The reason for this is the usage of the broken dashboard_context for generating the DSL for a query. This plainly ignores if the query is actually written in KQL/Kquery.

This class should be removed completely and rather the correct way of implementing this should be used:

The request handler of a visualization will get passed in the filters and query in the params object (2nd argument to the request handler). Every visualization should use that query and filters instead of trying to access any global state (like dashboard_context does). Accessing global state in any visualization will break other features that we are planning like Custom time ranges per dashboard panel (#17776). It's an anti pattern that makes the whole architecture unstable and hard to maintain.

A visualization needs to look into filters/queries passed to it's request handler. To compile the query, there are APIs for Lucene and Kuery. We should imho look into creating one central "Query API" (cc @elastic/kibana-discovery), that accepts any query object and that itself will check what query type that query is and use the appropriate compiler. That way we don't duplicate that work into each individual visualization.

That compiled DSL needs then to be merged into the query done by the appropriate visualization. Since KQL/Kuery needs to be compiled against an index pattern, all visualizations need to figure out the appropriate index pattern, for their given index pattern string (maybe we could move that into the Query compile API?).

@stacey-gammon stacey-gammon added bug Fixes for quality problems that affect the customer experience Feature:Dashboard Dashboard related features :Discovery Feature:Visualizations Generic visualization features (in case no more specific feature label is available) triage_needed labels Apr 16, 2018
@timroes
Copy link
Contributor

timroes commented Apr 17, 2018

@elastic/kibana-discovery I am not exactly aware how to work with KQL yet. We would need to open a PR that has an else if in there for KQL and converts it to ES query. Do you think you have capacity to open that, or could you give us the APIs needed for that?

@timroes timroes added Feature:Timelion Timelion app and visualization Feature:TSVB TSVB (Time Series Visual Builder) Feature:Vega Vega visualizations labels Apr 17, 2018
@Bargs
Copy link
Contributor

Bargs commented Apr 17, 2018

Dashboard should not treat the query like a filter anymore. The only reason that was necessary in the past was because queries did not get merged when the SearchSource hierarchy was flattened. I changed that back in 6.0, queries now get merged, and if my memory serves I also updated dashboard so it wouldn't put the query in the filters anymore. I'm not sure how this dashboard_context.js gets used, but it looks like it may have re-introduced that behavior. We also really need to stop passing around ES query DSL.

Also, as long as SearchSource is being used, no special handling of KQL is necessary.

@stacey-gammon stacey-gammon self-assigned this May 8, 2018
@stacey-gammon
Copy link
Contributor Author

I assigned myself, will look into it when I can but no promises on a soon-ish time.

@stacey-gammon
Copy link
Contributor Author

This came up in a meeting today with @ppisljar, @timroes , @Bargs and @spalger.

Turns out there is no simple fix for this, and once we pass filters, queries, etc down from dashboard to embeddables directly, it will negate the need for this entire file. The problem with a quick fix is that even though there is an available conversion function available, something like fromKuery, it assumes a single index pattern, but those visualization types can use multiple index patterns.

So for now, we are living with this limitation since they are all experimental features, until we can clean up search source issues and pass down filters/queries, etc directly (issue pending for that goal).

@stacey-gammon stacey-gammon removed their assignment May 15, 2018
@stacey-gammon stacey-gammon removed triage_needed bug Fixes for quality problems that affect the customer experience labels May 15, 2018
@stacey-gammon
Copy link
Contributor Author

Got rid of the bug label since technically we are considering this an unsupported feature.

@timroes
Copy link
Contributor

timroes commented Jul 4, 2018

Blocked by #20457

@timroes timroes added the blocked label Jul 4, 2018
@timroes timroes removed the blocked label Aug 7, 2018
@timroes timroes changed the title TSVB, timelion and Vega visualizations don't listen to kuery syntax KQL/Kuery support for TSVB, timelion and Vega visualizations Aug 7, 2018
@nyurik
Copy link
Contributor

nyurik commented Aug 7, 2018

From Vega's perspective, it needs an API to convert current dashboard's context to the MUST and MUST_NOT clauses for the ES query. This way Vega users can continue to write custom ES queries, and simply add %dashboard_context-must_clause% in the middle of their own query, which will be expanded with a list of dashboard-generated clauses. Vega authors already have to provide index pattern string.

@timroes timroes removed the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Sep 16, 2018
@timroes timroes added Team:Visualizations Visualization editors, elastic-charts and infrastructure and removed Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Visualizations Generic visualization features (in case no more specific feature label is available) labels Sep 16, 2018
@ppisljar ppisljar self-assigned this Sep 17, 2018
@markov00 markov00 assigned markov00 and unassigned ppisljar Nov 12, 2018
@syberyjskimisiek
Copy link

syberyjskimisiek commented Feb 19, 2019

Hi, kind of new here, I had a go at upgrading made by @PhaedrusTheGreek - transform_vis plugin. I removed dashboard context and replaced it with query/filters section, it installed sucessfully to my surprise however whenever I try to use second argument of myRequestHandler I somehow get "Cannot read property 'appState' of undefined" as a global error on a kibana when creating new vis. I cannot debug what's the actual argument passed to my custom Request Handler it comes either as undefined when the vis comes defined as an proper object. According to a documentation (from ES site for 6.6.0) it should be there along with uiState and timeRange? Or have I missed anything?

@PhaedrusTheGreek btw thanks mate this vizualisation of yours gives awesome flexibility in creating custom views :).

@syberyjskimisiek
Copy link

It seems that only one object is passed over (vis) not 2 as it was before, however documentations remains unchanged. I managed to launch transform_vis with success :). There are still a little tweaks to be done but I think it works now.

@Bargs
Copy link
Contributor

Bargs commented Apr 10, 2019

This was fixed in #28010

@Bargs Bargs closed this as completed Apr 10, 2019
@Classic92
Copy link

#17722 (comment)

I have the same problem like syberyjskimisiek
Is it possible to explain me this problem. I can install the Plugin transform_vis but i have also a problem with the appState. I can't see also the JavaScript section. Where is my mistake

@syberyjskimisiek
Copy link

@Classic92, if I remember correctly only one object is returned through myRequestHandler. This was the main change. You need to change your code to accept 1 object instead of 2, and somehow resolve the appState being undefined issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Dashboard Dashboard related features Feature:Timelion Timelion app and visualization Feature:TSVB TSVB (Time Series Visual Builder) Feature:Vega Vega visualizations Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

No branches or pull requests

8 participants