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

New type of filter to get all the values for a field within the current context #17571

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/core_plugins/kibana/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { exportApi } from './server/routes/api/export';
import { homeApi } from './server/routes/api/home';
import scripts from './server/routes/api/scripts';
import { registerSuggestionsApi } from './server/routes/api/suggestions';
import { registerValuesApi } from './server/routes/api/values';
import { registerFieldFormats } from './server/field_formats/register';
import { registerTutorials } from './server/tutorials/register';
import * as systemApi from './server/lib/system_api';
Expand Down Expand Up @@ -151,6 +152,7 @@ export default function (kibana) {
exportApi(server);
homeApi(server);
registerSuggestionsApi(server);
registerValuesApi(server);
registerFieldFormats(server);
registerTutorials(server);
server.expose('systemApi', systemApi);
Expand Down
5 changes: 5 additions & 0 deletions src/core_plugins/kibana/server/routes/api/values/index.js
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) {
Copy link
Contributor

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:

server.route({
  path: '/api/kibana/values/{index}',
  method: ['POST'],
  config: {
    validate: {
      params: Joi.object().keys({
        index: Joi.string().required()
      }).default(),
      payload: Joi.object().keys({
        field: Joi.string().required(),
        query: Joi.object().required(),
      }).required()
    },
    async handler(req, reply) {
      // ...
    }
  }
})

PS, the query parameter supports a lot of stuff. @elastic/kibana-operations @elastic/kibana-platform does this concern any of you?

const { index } = req.params;
const { field } = req.payload;
Copy link
Contributor

Choose a reason for hiding this comment

The 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 = [];
Copy link
Contributor

Choose a reason for hiding this comment

The 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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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 Array -> Set -> Array if you just use one from the beginning:

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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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 values property instead.

} catch (error) {
reply(handleESError(error));
}
}
});
}

function getBody(field, { query }) {
const aggregationSize = 10000;
Copy link
Contributor

@spalger spalger Apr 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also now it is hardcoded to a 10k limit for the value, I would like to make this configurable maybe with an advanced setting or with an argument in the filter pop up.

Yeah, I think it would be idea to support this as a parameter and default to something smaller, like 100.

Copy link
Contributor

@spalger spalger Apr 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think the the name Top Terms or Top Values would be more appropriate for this filter, since it's always going to be limited by the size (even if we set it to something huge like 10k).

Copy link
Author

Choose a reason for hiding this comment

The 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.

What about having the default value in an advanced setting? It is easier to have a custom default value.

Copy link
Contributor

Choose a reason for hiding this comment

The 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: {
Copy link
Author

Choose a reason for hiding this comment

The 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
Expand Up @@ -26,6 +26,7 @@ function emptySearch() {
* @return {Promise.<string>}
*/
export function requestFetchParamsToBody(
$rootScope,
requestsFetchParams,
Promise,
timeFilter,
Expand Down Expand Up @@ -68,6 +69,10 @@ export function requestFetchParamsToBody(
index = indexList;
}


$rootScope.filterState = { query: body.query };
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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).

Copy link
Contributor

Choose a reason for hiding this comment

The 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,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import { requestFetchParamsToBody } from './request_fetch_params_to_body';

export function RequestFetchParamsToBodyProvider(Promise, timefilter, kbnIndex, sessionId) {
export function RequestFetchParamsToBodyProvider($rootScope, Promise, timefilter, kbnIndex, sessionId) {
return (requestsFetchParams) => (
requestFetchParamsToBody(
$rootScope,
requestsFetchParams,
Promise,
timefilter,
Expand Down
2 changes: 2 additions & 0 deletions src/ui/public/filter_bar/lib/map_filter.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { FilterBarLibMapMissingProvider } from './map_missing';
import { FilterBarLibMapQueryStringProvider } from './map_query_string';
import { FilterBarLibMapGeoBoundingBoxProvider } from './map_geo_bounding_box';
import { FilterBarLibMapGeoPolygonProvider } from './map_geo_polygon';
import { FilterBarLibMapValuesProvider } from './map_values';
import { FilterBarLibMapDefaultProvider } from './map_default';

export function FilterBarLibMapFilterProvider(Promise, Private) {
Expand Down Expand Up @@ -41,6 +42,7 @@ export function FilterBarLibMapFilterProvider(Promise, Private) {
Private(FilterBarLibMapQueryStringProvider),
Private(FilterBarLibMapGeoBoundingBoxProvider),
Private(FilterBarLibMapGeoPolygonProvider),
Private(FilterBarLibMapValuesProvider),
Private(FilterBarLibMapDefaultProvider),
];

Expand Down
10 changes: 10 additions & 0 deletions src/ui/public/filter_bar/lib/map_values.js
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 });
}
};
}
2 changes: 2 additions & 0 deletions src/ui/public/filter_editor/filter_editor.html
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@
params="filterEditor.params"
></filter-params-editor>
</div>


</div>

<!-- DSL editor -->
Expand Down
4 changes: 4 additions & 0 deletions src/ui/public/filter_editor/lib/filter_editor_utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ export function getParamsFromFilter(filter) {
params = filter.query ? filter.query.match[key].query : filter.script.script.params.value;
} else if (type === 'phrases') {
params = filter.meta.params;
} else if (type === 'values') {
params = filter.meta.params.values;
} else if (type === 'range') {
const range = filter.range ? filter.range[key] : filter.script.script.params;
const from = _.has(range, 'gte') ? range.gte : range.gt;
Expand Down Expand Up @@ -77,6 +79,8 @@ export function buildFilter({ indexPattern, field, operator, params, filterBuild
filter = filterBuilder.buildRangeFilter(field, { gte: params.range.from, lt: params.range.to }, indexPattern);
} else if (operator.type === 'exists') {
filter = filterBuilder.buildExistsFilter(field, indexPattern);
} else if (operator.type === 'values') {
filter = filterBuilder.buildValuesFilter(field, params, indexPattern);
}
filter.meta.negate = operator.negate;
return filter;
Expand Down
6 changes: 6 additions & 0 deletions src/ui/public/filter_editor/lib/filter_operators.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,12 @@ export const FILTER_OPERATORS = [
type: 'exists',
negate: true,
},
{
name: 'values',
type: 'values',
negate: false,
fieldTypes: ['string', 'number', 'date', 'ip', 'geo_point', 'geo_shape']
},
];

export const FILTER_OPERATOR_TYPES = _(FILTER_OPERATORS)
Expand Down
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
@@ -1,4 +1,5 @@
<ng-switch on="operator.type">

<filter-params-phrase-editor
ng-switch-when="phrase"
field="field"
Expand All @@ -16,4 +17,11 @@
field="field"
params="params"
></filter-params-range-editor>
</ng-switch>

<filter-params-values-editor
ng-switch-when="values"
field="field"
params="params"
></filter-params-values-editor>

</ng-switch>
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,23 @@ import { uiModules } from 'ui/modules';
import template from './filter_params_editor.html';
import './filter_params_phrase_editor';
import './filter_params_phrases_editor';
import './filter_params_values_editor';
import './filter_params_range_editor';
import { filterParamsController } from './filter_params_controller';

const module = uiModules.get('kibana');
module.directive('filterParamsEditor', function () {

return {
restrict: 'E',
template,
scope: {
field: '=',
operator: '=',
params: '='
}
},

controllerAs: 'filterParamsEditor',
controller: filterParamsController
};
});
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,4 @@
>
Accepted date formats
</a>
</small>
</small>
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Up @@ -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) {
Copy link
Contributor

@spalger spalger Apr 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure to pull in the Promise service here.

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is using a mix of angular promises from $http I think it should also return angular promises when it returns early. This is also an angular controller, so I think angular promises are appropriate.


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
Copy link
Contributor

@spalger spalger Apr 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than writing the filter state to $rootScope, maybe we can use a temporary SearchSource to determine the current filters? @elastic/kibana-discovery? Might work something like this:

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);
      });
  };

}

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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.
Instead we want dashboard to pass filters, time range and query explicitly down to it's embeddables.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could probably just pull in the queryFilter and timeFilter services, I'm mostly just worried about not doing things the same way they are done by the app itself that might cause the values retrieved by this filter to not match the values being rendered by the vis/discover/dashboard they are supposed to match.

Copy link
Contributor

Choose a reason for hiding this comment

The 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
};
});
1 change: 1 addition & 0 deletions src/ui/public/filter_manager/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@ export { buildPhraseFilter } from './phrase';
export { buildPhrasesFilter } from './phrases';
export { buildQueryFilter } from './query';
export { buildRangeFilter } from './range';
export { buildValuesFilter } from './values';
36 changes: 36 additions & 0 deletions src/ui/public/filter_manager/lib/values.js
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;
}