-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
New type of filter to get all the values for a field within the current context #17571
Changes from all commits
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,5 @@ | ||
import { registerValues } from './register_values'; | ||
|
||
export function registerValuesApi(server) { | ||
registerValues(server); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
import handleESError from '../../../lib/handle_es_error'; | ||
|
||
export function registerValues(server) { | ||
server.route({ | ||
path: '/api/kibana/values/{index}', | ||
method: ['POST'], | ||
handler: async function (req, reply) { | ||
const { index } = req.params; | ||
const { field } = req.payload; | ||
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'd merge this and the next line |
||
const { query } = req.payload; | ||
|
||
const { callWithRequest } = server.plugins.elasticsearch.getCluster('data'); | ||
const body = getBody(field, query); | ||
let fieldList = []; | ||
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. This collects the list of values, not a list of fields, right? |
||
try { | ||
|
||
const response = await callWithRequest(req, 'search', { index, body }); | ||
const buckets = response.aggregations.aggResults.buckets; | ||
for (let i = 0; i < buckets.length; i < i++) { | ||
const currentField = buckets[i].key; | ||
if (typeof (currentField) === 'object') { | ||
// its an array | ||
fieldList = fieldList.concat(currentField); | ||
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 had no idea that bucket keys could be arrays. Also, you could merge the handling of array/non-array bucket keys and avoid going from const response = await callWithRequest(req, 'search', { index, body });
const values = new Set();
for (const bucket of response.aggregations.aggResults) {
for (const value of [].concat(bucket.key)) {
values.add(value)
}
}
reply(Array.from(values)) |
||
} else { | ||
fieldList.push(currentField); | ||
} | ||
} | ||
const uniqueValues = Array.from(new Set(fieldList)); | ||
reply(uniqueValues); | ||
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. JSON array responses can have weird security implications, I suggest responding with an object that has a |
||
} catch (error) { | ||
reply(handleESError(error)); | ||
} | ||
} | ||
}); | ||
} | ||
|
||
function getBody(field, { query }) { | ||
const aggregationSize = 10000; | ||
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.
Yeah, I think it would be idea to support this as a parameter and default to something smaller, like 100. 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 also think the the name 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.
What about having the default value in an advanced setting? It is easier to have a custom default value. 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. Works for me, the default can be read from the route handler with: const defaultSize = await request.getUiSettingsService().get('...') |
||
return { | ||
size: 0, | ||
timeout: '60s', | ||
aggs: { | ||
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. Is there a more efficient way to get this values back? Terms aggregation are pretty expensive especially when you set up a very high aggregation size. In my use case, I am planning to have this number 50k. |
||
aggResults: { | ||
terms: { | ||
field: field, | ||
size: aggregationSize | ||
} | ||
} | ||
}, | ||
query: query | ||
}; | ||
} | ||
|
||
// function getEscapedQuery(query = '') { | ||
// // https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-regexp-query.html#_standard_operators | ||
// return query.replace(/[.?+*|{}[\]()"\\#@&<>~]/g, (match) => `\\${match}`); | ||
// } |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,7 @@ function emptySearch() { | |
* @return {Promise.<string>} | ||
*/ | ||
export function requestFetchParamsToBody( | ||
$rootScope, | ||
requestsFetchParams, | ||
Promise, | ||
timeFilter, | ||
|
@@ -68,6 +69,10 @@ export function requestFetchParamsToBody( | |
index = indexList; | ||
} | ||
|
||
|
||
$rootScope.filterState = { query: body.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. This function is called for each request on the dashboard, which means it gets the specific filters for each panel. We need to find a better way to communicate this information to the values filter. 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 really don't want to introduce more reliance on rootScope in courier. I've been trying hard to de-angularize it. Is there perhaps another way we can achieve the same result? (I ask this without actually looking at the code beyond spying this one tidbit). 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. That's the intention behind the discussion in #17571 (comment) |
||
|
||
|
||
return JSON.stringify({ | ||
index, | ||
type: fetchParams.type, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
export function FilterBarLibMapValuesProvider(Promise) { | ||
return function (filter) { | ||
const { type, key, value, params } = filter.meta; | ||
if (type !== 'values') { | ||
return Promise.reject(filter); | ||
} else { | ||
return Promise.resolve({ type, key, value, params }); | ||
} | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,6 +68,8 @@ | |
params="filterEditor.params" | ||
></filter-params-editor> | ||
</div> | ||
|
||
|
||
</div> | ||
|
||
<!-- DSL editor --> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
|
||
export function filterParamsController($http, $scope) { | ||
$scope.$on('value_event', function (event, data) { | ||
$scope.params = { values: data }; | ||
}); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,4 +42,4 @@ | |
> | ||
Accepted date formats | ||
</a> | ||
</small> | ||
</small> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,4 +25,4 @@ | |
> | ||
<div ng-bind-html="value | highlight: $select.search"></div> | ||
</ui-select-choices> | ||
</ui-select> | ||
</ui-select> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,4 +27,4 @@ | |
> | ||
Accepted date formats | ||
</a> | ||
</small> | ||
</small> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
|
||
|
||
import _ from 'lodash'; | ||
import chrome from 'ui/chrome'; | ||
|
||
const baseUrl = chrome.addBasePath('/api/kibana/values'); | ||
|
||
export function filterParamsValuesController($http, $scope, $rootScope) { | ||
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. Make sure to pull in 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. But we are trying to remove reliance on the angular promise service, and the filter editor should be moving to react eventually. I don't actually think we want to use the angular Promise class here. #13855 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. Since this is using a mix of angular promises from |
||
|
||
this.getAllFieldValues = getAllFieldValues; | ||
|
||
|
||
function getAllFieldValues(field) { | ||
|
||
field = $scope.field; | ||
if (!_.get(field, 'aggregatable') || field.type !== 'string') { | ||
return Promise.resolve([]); | ||
} | ||
|
||
const postParams = { | ||
field: field.name, | ||
query: $rootScope.filterState | ||
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. Rather than writing the filter state to import chrome from 'ui/chrome';
import { FilterBarQueryFilterProvider } from 'ui/filter_bar/query_filter';
export function filterParamsValuesController($http, $scope, courier, Private, Promise) {
const baseUrl = chrome.addBasePath('/api/kibana/values');
const queryFilter = Private(FilterBarQueryFilterProvider);
function getRootSearchSource(searchSource) {
const parent = searchSource.getParent();
return parent ? getRootSearchSource(parent) : searchSource;
}
this.getAllFieldValues = function (field) {
field = $scope.field;
if (!field || !field.aggregatable || field.type !== 'string') {
return Promise.resolve([]);
}
const searchSource = new courier.SearchSource();
return searchSource
.set('index', field.indexPattern)
.set('filter', queryFilter.getFilters())
// this is necessary because apps like dashboard modify the "root search source" in
// the courier to ensure that all search sources inherit from the dashboard saved
// object. Since not every app does this we override this by inheriting from the true
// root by walking up until we find a searchSource that has no parents.
.inherits(getRootSearchSource(searchSource))
.getSearchRequestBody()
.then(({ query }) => (
$http.post(`${baseUrl}/${field.indexPattern.title}`, {
field: field.name,
query
})
))
.then(response => {
$scope.$emit('value_event', response.data);
});
};
} 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. btw, these are just ideas, not tested or sure it works well in all situations :) @stacey-gammon do you know why we don't set the search source in apps like Discover to be the root search source? 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. Ah, no, but I don't even like that dashboard does it. It's created some strange bugs. See #17123 and #16641 - we are workin towards getting rid of the search source inheritance all together. I haven't looked really at any of this PR but I'd be worried that any reliance on searchSource inheritance will break in the future. 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. This could probably just pull in 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. Haven't had much time to look into this either, but just want to second Stacey here. We will remove search source inheritance rather sooner than later (hopefully within one of the next minor versions). I really think we shouldn't build anything new that relies on that anymore. And of course also second, not introducing any new Angular code here, since we are really hardly trying to get rid of Angular. |
||
}; | ||
|
||
|
||
// TODO pass the response to the scope so that the build function can pick it up | ||
$http.post(`${baseUrl}/${field.indexPattern.title}`, postParams) | ||
.then(response => { | ||
$scope.$emit('value_event', response.data); | ||
}); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
<button | ||
class="kuiButton kuiButton--primary" | ||
ng-click="filterParamsValuesEditor.getAllFieldValues()"> | ||
Fetch | ||
</button> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
|
||
import { uiModules } from 'ui/modules'; | ||
import template from './filter_params_values_editor.html'; | ||
import { filterParamsValuesController } from './filter_params_values_controller'; | ||
import './filter_params_input_type'; | ||
|
||
const module = uiModules.get('kibana'); | ||
module.directive('filterParamsValuesEditor', function () { | ||
return { | ||
restrict: 'E', | ||
template, | ||
scope: { | ||
field: '=', | ||
params: '=' | ||
}, | ||
controllerAs: 'filterParamsValuesEditor', | ||
controller: filterParamsValuesController | ||
}; | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
// import { getPhraseScript } from './values'; | ||
|
||
export function buildValuesFilter(field, params, indexPattern) { | ||
const index = indexPattern.id; | ||
const type = 'values'; | ||
const key = field.name; | ||
const values = params.values; | ||
|
||
const filter = { | ||
meta: { index, type, key, values, params } | ||
}; | ||
|
||
// if (field.scripted) { | ||
// should = params.map((value) => ({ | ||
// script: getPhraseScript(field, value) | ||
// })); | ||
// } else { | ||
// should = params.map((value) => ({ | ||
// match_phrase: { | ||
// [field.name]: value | ||
// } | ||
// })); | ||
// } | ||
|
||
filter.query = { | ||
bool: { | ||
filter: { | ||
terms: { | ||
[field.name]: values | ||
} | ||
} | ||
} | ||
}; | ||
|
||
return filter; | ||
} |
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.
Please define validation rules for the supported params and payload parameters. I think it should look something like:
PS, the
query
parameter supports a lot of stuff. @elastic/kibana-operations @elastic/kibana-platform does this concern any of you?