-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Enable KQL support for TSVB #26006
Enable KQL support for TSVB #26006
Conversation
136053c
to
1d136a0
Compare
💔 Build Failed |
💚 Build Succeeded |
Pinging @elastic/kibana-app |
@markov00 could you rebase/merge master? Thanks! |
@lukasolson let's wait for this #25994 to be approved and merged first. |
Using the buildEsQuery changed a bit the order of the must.bool array on the query
bb3d2a2
to
9db9e9d
Compare
💔 Build FailedFailure on
|
retest |
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that this PR doesn't add the query bar in visualize when you create a TSVB visualization. Does it make sense to at this point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what's happening but on a dashboard with a TSVB and KQL selected and an empty query, the vis always initially says "No data to display for the selected metrics" until I modify the query or time range or filters.
const processor = buildProcessorFunction(processors, resp, panel, column); | ||
return _.first(processor([])); | ||
// getting the index pattern fields | ||
const indexPatterns = indexPatternObjects.saved_objects |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's already a utility for this here:
export function getFromSavedObject(savedObject) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good! Made a couple of comments above. I was thinking, is this PR considered a breaking change since it changes the behavior for existing saved objects in dashboards?
Prior to this PR, if you had a TSVB on a dashboard that pointed to an index pattern that didn't exist, and you used KQL, it wouldn't affect the TSVB. However, after this PR, the KQL query will basically cause any TSVB's that point to a different index pattern than what is being queried to render without any results.
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few inline comments. One side note for the future: it would be nice to support autocomplete in the query bar in TSVB if the index selected matches a real index pattern.
// getting the index pattern fields | ||
const indexPatterns = indexPatternObjects.saved_objects | ||
.filter(obj => obj.attributes.title === indexPatternString) | ||
.map(indexPattern => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a util function you can use to do this instead if you want
kibana/src/ui/public/index_patterns/static_utils/index.js
Lines 28 to 37 in b99c516
export function getFromSavedObject(savedObject) { | |
if (!savedObject) { | |
return null; | |
} | |
return { | |
fields: JSON.parse(savedObject.attributes.fields), | |
title: savedObject.attributes.title, | |
}; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, but reside on public and we need to use it on server side. I should move that function in a shared folder and reuse it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh duh, I missed the fact that one was server and one public. No sweat if you don't wanna move that around in this PR.
@@ -45,6 +47,16 @@ function ReactEditorControllerProvider(Private, config) { | |||
</I18nProvider>, this.el); | |||
} | |||
|
|||
setDefaultIndexPattern = async (appState) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain what the purpose of this change is? It looks like you're trying to handle the case where the index pattern input is empty, but that probably isn't necessary any more now that we handle null index patterns properly in the KQL serializers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me try to check if it can work now without that part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this was introduced to fix the empty/null pattern error. I'm updating the PR removing that.
|
||
const queries = req.payload.query || []; | ||
const filters = !annotation.ignore_global_filters ? req.payload.filters : []; | ||
doc.query = buildEsQuery(indexPattern, queries, filters, esQueryConfig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the query bar really affect the annotations? I think annotations are likely to come from a different index, so queries in the global query bar will likely filter out all docs in the annotation index. Annotations also have options to ignore the global filter and the panel filter by which are enabled by default so if we did want to allow the query bar to affect annotations, I’d think we would want an option to ignore it as well. @alexfrancoeur or @AlonaNadler do you have any opinion here? I'm not super familiar with how annotations are generally used in the wild so I'm just guessing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite timely, I literally just closed two enhancement requests this week around annotations being filterable. They have been filterable for awhile now but it isn't too discoverable. https://github.com/elastic/enhancements/issues/4007, https://github.com/elastic/enhancements/issues/5141. I'd say it's important, we have controls in TSVB for it as of 5.5 #11260
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexfrancoeur Cool, so just to make sure I understand, you're saying we should allow the query bar to filter annotations, and add an additional boolean option called something like Ignore global query bar
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And what about the visualization, don't we need to add the Ignore global query bar
also here? So you can opt-out from the query bar as it works before this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think KQL should filter out TSVB just as the existing query bar and filters does today. There are options to ignore the filters or to persist annotations there. This functionality is generally applied to metrics as well in TSVB, not just annotations.
These boolean options should already be available in the TSVB UI today. If it's easier to sync live, I'm happy to touch base over zoom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexfrancoeur yes please a zoom sync will help :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexfrancoeur @Bargs @lukasolson with this commit 9384a2d I've updated the PR to mimic exactly how TSVB works with lucene syntax:
- flagging
yes
onIgnore global filters
on data, any filter added to a dashboard or any search written on the query bar will be ignored by the visualization - flagging
yes
onIgnore global filters
on annotations, any filter added to a dashboard or any search written on the query bar will be ignored by the annotations - flagging
no
onIgnore global filters
on data, any filter added to a dashboard or any search written on the query bar will filter the visualization data - flagging
no
onIgnore global filters
on annotations, any filter added to a dashboard or any search written on the query bar will filter the annotations
The current label Ignore global filters
is, IMHO, a bit misleading because doesn't seems to include queries on the querybar, but I think we can change that on a different PR.
If this is ok for you, we can merge this PR and backport it to 6.7
After fixing the null index pattern issue in kql there is no need to use set the default index pattern before rendering.
💚 Build Succeeded |
This commit mimic the behaviour of the ignore_global_filters currently used with lucene syntax: if you enable the ignore_global_filter option on data or on annotations, data or annotations are filtered out when using the filter bar and the search bar
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested with and without Ignore global filters
set for both panel data and annotations. Everything worked as I expected. LGTM!
@AlonaNadler we are aware that KQL autocomplete doesn't work in TSVB. @Bargs @lukasolson can describe better then me the reason, but the main problem is related to the fact that TSVB can use arbitrary index-patterns but the KQL query bar expect a saved index-pattern object. I will check the opt-in persist issue and the multivalue issue |
@matt @lukasolson @timroes I think we might be missing something in this PR, though I understand there is a technical distinction between KQL and autocomplete, from customer perspective autocomplete with syntax suggestions, field names and values is the main value customers get from KQL and the reason they will opt-in to KQL. Currently, the main issue is that users opt-in and use KQL and when they do that in a dashboard the TSVB charts don't get filtered, that behavior makes users opt-out since it perceived KQL is not reliable and doesn't always work. Having TSVB support for the KQL syntax without the autocomplete will not help when the TSVB charts are using different index pattern than the one in the regular charts or if users have a dashboard with TSVB charts only, in this case, users will not get any suggestions. I'm concerned that by only supporting the KQL syntax without autocompelte we are not solving the real problem My hope was that I will get suggestions and will be able to filter TSVB using KQL (using autocomplete) from a dashboard as phase1 |
It might not be too hard to add autocomplete support when the index pattern in TSVB matches an index pattern that exists in Kibana. But we won't be able to support autocomplete on arbitrary index patterns in TSVB in the short term. |
@Bargs that's totally make sense :) |
We could also suggest index patterns that exist in kibana as well as accept specific ones that are not saved in Kibana. I believe the roles UI has similar functionality. This would at least make it more likely that users are using existing index patterns. |
💚 Build Succeeded |
# Conflicts: # src/legacy/core_plugins/metrics/server/lib/vis_data/annorations/build_request_body.js # src/legacy/core_plugins/metrics/server/lib/vis_data/get_annotations.js # src/legacy/core_plugins/metrics/server/lib/vis_data/get_series_data.js # src/legacy/core_plugins/metrics/server/lib/vis_data/get_table_data.js # src/legacy/core_plugins/metrics/server/lib/vis_data/request_processors/annotations/query.js # src/legacy/core_plugins/metrics/server/lib/vis_data/series/__tests__/build_request_body.js # src/legacy/core_plugins/metrics/server/lib/vis_data/series/build_request_body.js # src/legacy/core_plugins/metrics/server/lib/vis_data/series/get_request_params.js
* Use buildEsQuery for table, series and annotations * Fix query test. Using the buildEsQuery changed a bit the order of the must.bool array on the query * Remove console.log * Remove console.error and comment * Fix wrong merge of PR elastic#26510 * Fix default/empty index_pattern When the user save the visualization without configuring manually an index_pattern, a default empty string is used instead of the default pattern. This leads to an empty visualization after the refactoring done in elastic#24832. Now it will update the index_pattern field with the default index pattern in visualize editor and on dashboard. * Remove unnecessary wrapping query in an array * Enable query bar on tsvb * Remove unnecessary setDefaultIndexPattern After fixing the null index pattern issue in kql there is no need to use set the default index pattern before rendering. * fix(tsvb-server): Ignore query bar search on ignore_global_filters param This commit mimic the behaviour of the ignore_global_filters currently used with lucene syntax: if you enable the ignore_global_filter option on data or on annotations, data or annotations are filtered out when using the filter bar and the search bar * Disable showQueryBar
* Enable KQL support for TSVB (#26006) * Use buildEsQuery for table, series and annotations * Fix query test. Using the buildEsQuery changed a bit the order of the must.bool array on the query * Remove console.log * Remove console.error and comment * Fix wrong merge of PR #26510 * Fix default/empty index_pattern When the user save the visualization without configuring manually an index_pattern, a default empty string is used instead of the default pattern. This leads to an empty visualization after the refactoring done in #24832. Now it will update the index_pattern field with the default index pattern in visualize editor and on dashboard. * Remove unnecessary wrapping query in an array * Enable query bar on tsvb * Remove unnecessary setDefaultIndexPattern After fixing the null index pattern issue in kql there is no need to use set the default index pattern before rendering. * fix(tsvb-server): Ignore query bar search on ignore_global_filters param This commit mimic the behaviour of the ignore_global_filters currently used with lucene syntax: if you enable the ignore_global_filter option on data or on annotations, data or annotations are filtered out when using the filter bar and the search bar * Disable showQueryBar * Fix non array query value on request
* Added a feature of rollup search on the UI side Signed-off-by: Alexey Antonov <alexwizp@gmail.com> * Rollup Feature - initial commit * Revert "Added a feature of rollup search on the UI side" This reverts commit 9568b09. # Conflicts: # src/legacy/core_plugins/metrics/public/components/index_pattern.js * Remove the 'label' property from the search strategies * Changed search by strategy from the last * add single search request * rollup_search_strategy add base implementation of isViable method * rollup_search_strategy add base implementation of isViable method -fix * Changed requests due to search request type * refactoring of import Base classes / remove '../../../../../../ * remove extra await * rollup_search_strategy. Refactoring of isRollupJobExists method * remove question * Add support of annotations and table data * skeleton for adding Search Strategy restrictions * Add rollup search capabilities * apply search strategy for annotations request * set fields capabilities for rollup strategy * add timezone, interval into SearchCapabilities * Add fields from capabilities * add timezone, interval into SearchCapabilities * fix default timezone * Merging of two Rollup Jobs was removed * move getFieldsForWildcard to searchStrategy * Fix TSVB search requests should have a timeout # Conflicts: # src/legacy/core_plugins/metrics/server/lib/vis_data/get_annotations.js # src/legacy/core_plugins/metrics/server/lib/vis_data/series/get_request_params.js * Add unit test * apply getEsShardTimeout for annorations/get_request_params, series/get_request_params * rename metrics -> tsvb * search_strategies_register refactoring: move 'add' method from class * Add merge rollup capabilities with fields * Add merge rollup capabilities with fields - small fixes * Add support of 'Everything' aggregation for Rollup Search * Return back metrics plugin * remove 'metrics' from the X-pack\rollup require * Fix test cases * fix broken test: fail: "apis InfraOps GraphQL Endpoints metrics should basically work" * rollup search - split by terms is not working * Add count metric * /get_bucket_size.js. Add support of 'auto' interval, Add support of gte intervals e.g.: >=1m * fix build_request_body test * [Rollup] [Phase 1] Error handling - rollup search errors should be more user friendly * [Rollup] [Phase 1] Table View - research the query to ES - sorting is not wokring * Merge #26006 into rollup # Conflicts: # src/legacy/core_plugins/metrics/server/lib/vis_data/annorations/build_request_body.js # src/legacy/core_plugins/metrics/server/lib/vis_data/get_annotations.js # src/legacy/core_plugins/metrics/server/lib/vis_data/get_series_data.js # src/legacy/core_plugins/metrics/server/lib/vis_data/get_table_data.js # src/legacy/core_plugins/metrics/server/lib/vis_data/request_processors/annotations/query.js # src/legacy/core_plugins/metrics/server/lib/vis_data/series/__tests__/build_request_body.js # src/legacy/core_plugins/metrics/server/lib/vis_data/series/build_request_body.js # src/legacy/core_plugins/metrics/server/lib/vis_data/series/get_request_params.js * Add table view support * fix broken build * fix broken build * [Rollup] [Phase 1] - write new tests (rollup_search_request, rollup_search_strategy) * [Rollup] [Phase 1] - write tests for rollup_search_capabilities * Add test on default_search_capabilities, abstract_search_strategy, search_strategies_register * Add test cases for search_requests folder * [Rollup] [Phase 1] - write tests for rollup_search_strategy * FIx broken build * remove todo * fix calculation of interval value for rollup search * add unit tests * Remove default exports * fix PR comments * fix calendar intervals
* Added a feature of rollup search on the UI side Signed-off-by: Alexey Antonov <alexwizp@gmail.com> * Rollup Feature - initial commit * Revert "Added a feature of rollup search on the UI side" This reverts commit 9568b09. # Conflicts: # src/legacy/core_plugins/metrics/public/components/index_pattern.js * Remove the 'label' property from the search strategies * Changed search by strategy from the last * add single search request * rollup_search_strategy add base implementation of isViable method * rollup_search_strategy add base implementation of isViable method -fix * Changed requests due to search request type * refactoring of import Base classes / remove '../../../../../../ * remove extra await * rollup_search_strategy. Refactoring of isRollupJobExists method * remove question * Add support of annotations and table data * skeleton for adding Search Strategy restrictions * Add rollup search capabilities * apply search strategy for annotations request * set fields capabilities for rollup strategy * add timezone, interval into SearchCapabilities * Add fields from capabilities * add timezone, interval into SearchCapabilities * fix default timezone * Merging of two Rollup Jobs was removed * move getFieldsForWildcard to searchStrategy * Fix TSVB search requests should have a timeout # Conflicts: # src/legacy/core_plugins/metrics/server/lib/vis_data/get_annotations.js # src/legacy/core_plugins/metrics/server/lib/vis_data/series/get_request_params.js * Add unit test * apply getEsShardTimeout for annorations/get_request_params, series/get_request_params * rename metrics -> tsvb * search_strategies_register refactoring: move 'add' method from class * Add merge rollup capabilities with fields * Add merge rollup capabilities with fields - small fixes * Add support of 'Everything' aggregation for Rollup Search * Return back metrics plugin * remove 'metrics' from the X-pack\rollup require * Fix test cases * fix broken test: fail: "apis InfraOps GraphQL Endpoints metrics should basically work" * rollup search - split by terms is not working * Add count metric * /get_bucket_size.js. Add support of 'auto' interval, Add support of gte intervals e.g.: >=1m * fix build_request_body test * [Rollup] [Phase 1] Error handling - rollup search errors should be more user friendly * [Rollup] [Phase 1] Table View - research the query to ES - sorting is not wokring * Merge elastic#26006 into rollup # Conflicts: # src/legacy/core_plugins/metrics/server/lib/vis_data/annorations/build_request_body.js # src/legacy/core_plugins/metrics/server/lib/vis_data/get_annotations.js # src/legacy/core_plugins/metrics/server/lib/vis_data/get_series_data.js # src/legacy/core_plugins/metrics/server/lib/vis_data/get_table_data.js # src/legacy/core_plugins/metrics/server/lib/vis_data/request_processors/annotations/query.js # src/legacy/core_plugins/metrics/server/lib/vis_data/series/__tests__/build_request_body.js # src/legacy/core_plugins/metrics/server/lib/vis_data/series/build_request_body.js # src/legacy/core_plugins/metrics/server/lib/vis_data/series/get_request_params.js * Add table view support * fix broken build * fix broken build * [Rollup] [Phase 1] - write new tests (rollup_search_request, rollup_search_strategy) * [Rollup] [Phase 1] - write tests for rollup_search_capabilities * Add test on default_search_capabilities, abstract_search_strategy, search_strategies_register * Add test cases for search_requests folder * [Rollup] [Phase 1] - write tests for rollup_search_strategy * FIx broken build * remove todo * fix calculation of interval value for rollup search * add unit tests * Remove default exports * fix PR comments * fix calendar intervals
* Added a feature of rollup search on the UI side Signed-off-by: Alexey Antonov <alexwizp@gmail.com> * Rollup Feature - initial commit * Revert "Added a feature of rollup search on the UI side" This reverts commit 9568b09. # Conflicts: # src/legacy/core_plugins/metrics/public/components/index_pattern.js * Remove the 'label' property from the search strategies * Changed search by strategy from the last * add single search request * rollup_search_strategy add base implementation of isViable method * rollup_search_strategy add base implementation of isViable method -fix * Changed requests due to search request type * refactoring of import Base classes / remove '../../../../../../ * remove extra await * rollup_search_strategy. Refactoring of isRollupJobExists method * remove question * Add support of annotations and table data * skeleton for adding Search Strategy restrictions * Add rollup search capabilities * apply search strategy for annotations request * set fields capabilities for rollup strategy * add timezone, interval into SearchCapabilities * Add fields from capabilities * add timezone, interval into SearchCapabilities * fix default timezone * Merging of two Rollup Jobs was removed * move getFieldsForWildcard to searchStrategy * Fix TSVB search requests should have a timeout # Conflicts: # src/legacy/core_plugins/metrics/server/lib/vis_data/get_annotations.js # src/legacy/core_plugins/metrics/server/lib/vis_data/series/get_request_params.js * Add unit test * apply getEsShardTimeout for annorations/get_request_params, series/get_request_params * rename metrics -> tsvb * search_strategies_register refactoring: move 'add' method from class * Add merge rollup capabilities with fields * Add merge rollup capabilities with fields - small fixes * Add support of 'Everything' aggregation for Rollup Search * Return back metrics plugin * remove 'metrics' from the X-pack\rollup require * Fix test cases * fix broken test: fail: "apis InfraOps GraphQL Endpoints metrics should basically work" * rollup search - split by terms is not working * Add count metric * /get_bucket_size.js. Add support of 'auto' interval, Add support of gte intervals e.g.: >=1m * fix build_request_body test * [Rollup] [Phase 1] Error handling - rollup search errors should be more user friendly * [Rollup] [Phase 1] Table View - research the query to ES - sorting is not wokring * Merge #26006 into rollup # Conflicts: # src/legacy/core_plugins/metrics/server/lib/vis_data/annorations/build_request_body.js # src/legacy/core_plugins/metrics/server/lib/vis_data/get_annotations.js # src/legacy/core_plugins/metrics/server/lib/vis_data/get_series_data.js # src/legacy/core_plugins/metrics/server/lib/vis_data/get_table_data.js # src/legacy/core_plugins/metrics/server/lib/vis_data/request_processors/annotations/query.js # src/legacy/core_plugins/metrics/server/lib/vis_data/series/__tests__/build_request_body.js # src/legacy/core_plugins/metrics/server/lib/vis_data/series/build_request_body.js # src/legacy/core_plugins/metrics/server/lib/vis_data/series/get_request_params.js * Add table view support * fix broken build * fix broken build * [Rollup] [Phase 1] - write new tests (rollup_search_request, rollup_search_strategy) * [Rollup] [Phase 1] - write tests for rollup_search_capabilities * Add test on default_search_capabilities, abstract_search_strategy, search_strategies_register * Add test cases for search_requests folder * [Rollup] [Phase 1] - write tests for rollup_search_strategy * FIx broken build * remove todo * fix calculation of interval value for rollup search * add unit tests * Remove default exports * fix PR comments * fix calendar intervals
Summary
This PR will enable the support of KQL in TSVB as in #17722.
Before reviewing this PR, two other PRs needs to be merged: #23345 and #25994This PR enable KQL support in the following case:
This file was removed because no longer used:
src/core_plugins/metrics/server/lib/vis_data/table/get_column_data.js
The CSS fix is used to display correctly the TSVB error subtitle.
A follow up PR can be created to display a warning message on a TSVB that doesn't use an index-pattern.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.[ ] This was checked for cross-browser compatibility, including a check against IE11[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support[ ] Documentation was added for features that require explanation or tutorials[ ] Unit or functional tests were updated or added to match the most common scenarios[ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
[ ] This was checked for breaking API changes and was labeled appropriately