-
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
Changes from 11 commits
ac3532e
7f1464b
9db9e9d
15125bd
4bf31a3
b4a570f
eb27329
f007742
6093879
ce46be8
a61214e
c3ea045
85b81fc
0c431f3
edc0342
5e78ee9
9384a2d
940aba6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
/* | ||
* Licensed to Elasticsearch B.V. under one or more contributor | ||
* license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright | ||
* ownership. Elasticsearch B.V. licenses this file to you under | ||
* the Apache License, Version 2.0 (the "License"); you may | ||
* not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
export async function getEsQueryConfig(req) { | ||
const uiSettings = req.getUiSettingsService(); | ||
const allowLeadingWildcards = await uiSettings.get('query:allowLeadingWildcards'); | ||
const queryStringOptions = await uiSettings.get('query:queryString:options'); | ||
return { | ||
allowLeadingWildcards, | ||
queryStringOptions: JSON.parse(queryStringOptions), | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,41 @@ | ||||||||||||||||||||||
/* | ||||||||||||||||||||||
* Licensed to Elasticsearch B.V. under one or more contributor | ||||||||||||||||||||||
* license agreements. See the NOTICE file distributed with | ||||||||||||||||||||||
* this work for additional information regarding copyright | ||||||||||||||||||||||
* ownership. Elasticsearch B.V. licenses this file to you under | ||||||||||||||||||||||
* the Apache License, Version 2.0 (the "License"); you may | ||||||||||||||||||||||
* not use this file except in compliance with the License. | ||||||||||||||||||||||
* You may obtain a copy of the License at | ||||||||||||||||||||||
* | ||||||||||||||||||||||
* http://www.apache.org/licenses/LICENSE-2.0 | ||||||||||||||||||||||
* | ||||||||||||||||||||||
* Unless required by applicable law or agreed to in writing, | ||||||||||||||||||||||
* software distributed under the License is distributed on an | ||||||||||||||||||||||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||||||||||||||||||||||
* KIND, either express or implied. See the License for the | ||||||||||||||||||||||
* specific language governing permissions and limitations | ||||||||||||||||||||||
* under the License. | ||||||||||||||||||||||
*/ | ||||||||||||||||||||||
|
||||||||||||||||||||||
export async function getIndexPatternObject(req, indexPatternString) { | ||||||||||||||||||||||
// getting the matching index pattern | ||||||||||||||||||||||
const savedObjectClient = req.getSavedObjectsClient(); | ||||||||||||||||||||||
const indexPatternObjects = await savedObjectClient.find({ | ||||||||||||||||||||||
type: 'index-pattern', | ||||||||||||||||||||||
fields: ['title', 'fields'], | ||||||||||||||||||||||
search: `"${indexPatternString}"`, | ||||||||||||||||||||||
search_fields: ['title'], | ||||||||||||||||||||||
}); | ||||||||||||||||||||||
|
||||||||||||||||||||||
// 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 commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||||||||||||||||||||||
const { title, fields } = indexPattern.attributes; | ||||||||||||||||||||||
return { | ||||||||||||||||||||||
title, | ||||||||||||||||||||||
fields: JSON.parse(fields), | ||||||||||||||||||||||
}; | ||||||||||||||||||||||
}); | ||||||||||||||||||||||
return indexPatterns.length === 1 ? indexPatterns[0] : null; | ||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -19,21 +19,18 @@ | |||||
|
||||||
import getBucketSize from '../../helpers/get_bucket_size'; | ||||||
import getTimerange from '../../helpers/get_timerange'; | ||||||
export default function query(req, panel, annotation) { | ||||||
import { buildEsQuery } from '@kbn/es-query'; | ||||||
|
||||||
export default function query(req, panel, annotation, esQueryConfig, indexPattern) { | ||||||
return next => doc => { | ||||||
const timeField = annotation.time_field; | ||||||
const { | ||||||
bucketSize | ||||||
} = getBucketSize(req, 'auto'); | ||||||
const { bucketSize } = getBucketSize(req, 'auto'); | ||||||
const { from, to } = getTimerange(req); | ||||||
|
||||||
doc.size = 0; | ||||||
doc.query = { | ||||||
bool: { | ||||||
must: [] | ||||||
} | ||||||
}; | ||||||
|
||||||
const queries = req.payload.query ? [req.payload.query] : []; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
@markov00 we're just getting back into reviewing the PR after fixing that bug in master that I mentioned in our zoom sync. Wanted to let you know that you'll need to fix it for TSVB in this PR as well since the relevant file changed. All you have to do is apply the suggestion above. Our PR is #27636 in case you want a little more info. |
||||||
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 commentThe 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 commentThe 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And what about the visualization, don't we need to add the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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:
The current label If this is ok for you, we can merge this PR and backport it to 6.7 |
||||||
const timerange = { | ||||||
range: { | ||||||
[timeField]: { | ||||||
|
@@ -54,11 +51,6 @@ export default function query(req, panel, annotation) { | |||||
}); | ||||||
} | ||||||
|
||||||
const globalFilters = req.payload.filters; | ||||||
if (!annotation.ignore_global_filters) { | ||||||
doc.query.bool.must = doc.query.bool.must.concat(globalFilters); | ||||||
} | ||||||
|
||||||
if (!annotation.ignore_panel_filters && panel.filter) { | ||||||
doc.query.bool.must.push({ | ||||||
query_string: { | ||||||
|
@@ -76,7 +68,5 @@ export default function query(req, panel, annotation) { | |||||
} | ||||||
|
||||||
return next(doc); | ||||||
|
||||||
}; | ||||||
} | ||||||
|
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.