Skip to content

Unify getting fields for aggs, and filter scripted fields for significant terms agg #8734

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Oct 27, 2016
Merged

Unify getting fields for aggs, and filter scripted fields for significant terms agg #8734

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e49ba04
[aggConfig] unify field option handling
Oct 18, 2016
a9c139a
[tests] fix tests that were improperly using fields
Oct 18, 2016
0757c45
[logstash_fields] remove sortable and filterable, which do nothing
Oct 18, 2016
2dbb462
[notifier] ensure that notifications are not left around
Oct 18, 2016
95d704c
[aggParams/field] support aggParamType#scriptable
Oct 17, 2016
0e11c22
[aggConfig] add light validation for aggConfig field param
Oct 18, 2016
e71b0f2
[aggConfig] restore use of `@timestamp`
Oct 25, 2016
6157275
[tests] revert changes to notifier clearing
Oct 25, 2016
fbf884a
[aggType/test] remove unnecessary change
Oct 25, 2016
0018786
[aggTypes/fieldParam] move orderby back into condition
Oct 25, 2016
7205993
[aggParams/field] use a more descriptive warning
Oct 27, 2016
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
3 changes: 2 additions & 1 deletion src/core_plugins/kibana/public/visualize/editor/__tests__/agg.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ describe('Vis-Editor-Agg plugin directive', function () {
$parentScope.agg = {
id: 1,
params: {},
schema: makeConfig()
schema: makeConfig(),
getFieldOptions: () => null
};
$parentScope.groupName = 'metrics';
$parentScope.group = [{
Expand Down
29 changes: 1 addition & 28 deletions src/core_plugins/kibana/public/visualize/editor/agg_params.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ uiModules

// create child scope, used in the editors
$aggParamEditorsScope = $scope.$new();
$aggParamEditorsScope.indexedFields = $scope.agg.getFieldOptions();

const agg = $scope.agg;
if (!agg) return;
Expand All @@ -81,10 +82,6 @@ uiModules
// build collection of agg params html
type.params.forEach(function (param, i) {
let aggParam;
// if field param exists, compute allowed fields
if (param.name === 'field') {
$aggParamEditorsScope.indexedFields = getIndexedFields(param);
}

if ($aggParamEditorsScope.indexedFields) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously this required access to the field param, but now the aggConfig knows how to get it's own field options with aggConfig.getFieldOptions()

const hasIndexedFields = $aggParamEditorsScope.indexedFields.length > 0;
Expand Down Expand Up @@ -136,30 +133,6 @@ uiModules
.append(param.editor)
.get(0);
}

function getIndexedFields(param) {
let fields = _.filter($scope.agg.vis.indexPattern.fields.raw, 'aggregatable');
const fieldTypes = param.filterFieldTypes;

if (fieldTypes) {
fields = $filter('fieldType')(fields, fieldTypes);
fields = $filter('orderBy')(fields, ['type', 'name']);
}

return new IndexedArray({

/**
* @type {Array}
*/
index: ['name'],

/**
* [group description]
* @type {Array}
*/
initialSet: fields
});
}
}
};
});
91 changes: 60 additions & 31 deletions src/fixtures/logstash_fields.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,36 +1,65 @@
function stubbedLogstashFields() {
let sourceData = [
{ name: 'bytes', type: 'number', indexed: true, analyzed: true, sortable: true, filterable: true, count: 10 },
{ name: 'ssl', type: 'boolean', indexed: true, analyzed: true, sortable: true, filterable: true, count: 20 },
{ name: '@timestamp', type: 'date', indexed: true, analyzed: true, sortable: true, filterable: true, count: 30 },
{ name: 'time', type: 'date', indexed: true, analyzed: true, sortable: true, filterable: true, count: 30 },
{ name: '@tags', type: 'string', indexed: true, analyzed: true, sortable: true, filterable: true },
{ name: 'utc_time', type: 'date', indexed: true, analyzed: true, sortable: true, filterable: true },
{ name: 'phpmemory', type: 'number', indexed: true, analyzed: true, sortable: true, filterable: true },
{ name: 'ip', type: 'ip', indexed: true, analyzed: true, sortable: true, filterable: true },
{ name: 'request_body', type: 'attachment', indexed: true, analyzed: true, sortable: false, filterable: true },
{ name: 'point', type: 'geo_point', indexed: true, analyzed: true, sortable: false, filterable: false },
{ name: 'area', type: 'geo_shape', indexed: true, analyzed: true, sortable: true, filterable: false },
{ name: 'hashed', type: 'murmur3', indexed: true, analyzed: true, sortable: false, filterable: false },
{ name: 'geo.coordinates', type: 'geo_point', indexed: true, analyzed: true, sortable: false, filterable: true },
{ name: 'extension', type: 'string', indexed: true, analyzed: true, sortable: true, filterable: true },
{ name: 'machine.os', type: 'string', indexed: true, analyzed: true, sortable: true, filterable: true },
{ name: 'geo.src', type: 'string', indexed: true, analyzed: true, sortable: true, filterable: true },
{ name: '_type', type: 'string', indexed: false, analyzed: true, sortable: true, filterable: true },
{ name: '_id', type: 'string', indexed: false, analyzed: false, sortable: false, filterable: true},
{ name: '_source', type: 'string', indexed: false, analyzed: false, sortable: false, filterable: false},
{ name: 'custom_user_field', type: 'conflict', indexed: false, analyzed: false, sortable: false, filterable: true },
{ name: 'script string', type: 'string', scripted: true, script: '\'i am a string\'', lang: 'expression' },
{ name: 'script number', type: 'number', scripted: true, script: '1234', lang: 'expression' },
{ name: 'script date', type: 'date', scripted: true, script: '1234', lang: 'painless' },
{ name: 'script murmur3', type: 'murmur3', scripted: true, script: '1234', lang: 'expression'},
].map(function (field) {
field.count = field.count || 0;
field.scripted = field.scripted || false;
return field;
});
return [
// |indexed
// | |analyzed
// | | |aggregatable
// | | | |searchable
// name type | | | | |metadata
['bytes', 'number', true, true, true, true, { count: 10 } ],
['ssl', 'boolean', true, true, true, true, { count: 20 } ],
['@timestamp', 'date', true, true, true, true, { count: 30 } ],
['time', 'date', true, true, true, true, { count: 30 } ],
['@tags', 'string', true, true, true, true ],
['utc_time', 'date', true, true, true, true ],
['phpmemory', 'number', true, true, true, true ],
['ip', 'ip', true, true, true, true ],
['request_body', 'attachment', true, true, true, true ],
['point', 'geo_point', true, true, true, true ],
['area', 'geo_shape', true, true, true, true ],
['hashed', 'murmur3', true, true, false, true ],
['geo.coordinates', 'geo_point', true, true, true, true ],
['extension', 'string', true, true, true, true ],
['machine.os', 'string', true, true, true, true ],
['geo.src', 'string', true, true, true, true ],
['_id', 'string', false, false, true, true ],
['_type', 'string', false, false, true, true ],
['_source', 'string', false, false, true, true ],
['custom_user_field', 'conflict', false, false, true, true ],
['script string', 'string', false, false, true, false, { script: '\'i am a string\'' } ],
['script number', 'number', false, false, true, false, { script: '1234' } ],
['script date', 'date', false, false, true, false, { script: '1234', lang: 'painless' } ],
['script murmur3', 'murmur3', false, false, true, false, { script: '1234' } ],
].map(function (row) {
const [
name,
type,
indexed,
analyzed,
aggregatable,
searchable,
metadata = {}
] = row;

const {
count = 0,
script,
lang = script ? 'expression' : undefined,
scripted = !!script,
} = metadata;

return sourceData;
return {
name,
type,
indexed,
analyzed,
aggregatable,
searchable,
count,
script,
lang,
scripted,
};
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I am missing what changed here. Is this a rewrite of the original? I guess 'expression' is now no longer hardcoded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The primary driver was to add aggregatable and searchable, which overflowed the rows when added in the previous style.

}

export default stubbedLogstashFields;
2 changes: 1 addition & 1 deletion src/ui/public/agg_response/tabify/__tests__/_response_writer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ describe('ResponseWriter class', function () {
let aggs = [
{ type: 'date_histogram', schema: 'segment', params: { field: '@timestamp' } },
{ type: 'terms', schema: 'segment', params: { field: 'extension' } },
{ type: 'avg', schema: 'metric', params: { field: '@timestamp' } }
{ type: 'avg', schema: 'metric', params: { field: 'bytes' } }
Copy link
Contributor

Choose a reason for hiding this comment

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

Were these tests suddenly blowing up for some reason? Why @timestamp -> time

Copy link
Contributor Author

@spalger spalger Oct 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@timestamp isn't a field in the stubbed logstash index pattern, so this fails the new valid check in #8723

Copy link
Contributor

Choose a reason for hiding this comment

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

kibana/src/fixtures/logstash_fields.js

Line 5 in 2aca107

{ name: '@timestamp', type: 'date', indexed: true, analyzed: true, sortable: true, filterable: true, count: 30 },
?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ugh, I guess I miscalculated this and it was just the type that was an issue (dates in the avg agg)

];

getColumns.returns(aggs.map(function (agg) {
Expand Down
1 change: 1 addition & 0 deletions src/ui/public/agg_types/buckets/significant_terms.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export default function SignificantTermsAggDefinition(Private) {
params: [
{
name: 'field',
scriptable: false,
filterFieldTypes: 'string'
},
{
Expand Down
1 change: 0 additions & 1 deletion src/ui/public/agg_types/buckets/terms.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ export default function TermsAggDefinition(Private) {
params: [
{
name: 'field',
scriptable: true,
filterFieldTypes: ['number', 'boolean', 'date', 'ip', 'string']
},
{
Expand Down
45 changes: 40 additions & 5 deletions src/ui/public/agg_types/param_types/field.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,21 @@ import { SavedObjectNotFound } from 'ui/errors';
import _ from 'lodash';
import editorHtml from 'ui/agg_types/controls/field.html';
import AggTypesParamTypesBaseProvider from 'ui/agg_types/param_types/base';
export default function FieldAggParamFactory(Private) {
import 'ui/filters/field_type';
import IndexedArray from 'ui/indexed_array';
import Notifier from 'ui/notify/notifier';

export default function FieldAggParamFactory(Private, $filter) {
let BaseAggParam = Private(AggTypesParamTypesBaseProvider);
const notifier = new Notifier();

_.class(FieldAggParam).inherits(BaseAggParam);
function FieldAggParam(config) {
FieldAggParam.Super.call(this, config);
}

FieldAggParam.prototype.editor = editorHtml;
FieldAggParam.prototype.scriptable = false;
FieldAggParam.prototype.scriptable = true;
FieldAggParam.prototype.filterFieldTypes = '*';

/**
Expand All @@ -25,6 +29,32 @@ export default function FieldAggParamFactory(Private) {
return field.name;
};

/**
* Get the options for this field from the indexPattern
*/
FieldAggParam.prototype.getFieldOptions = function (aggConfig) {
const indexPattern = aggConfig.getIndexPattern();
let fields = indexPattern.fields.raw;

fields = fields.filter(f => f.aggregatable);

if (!this.scriptable) {
fields = fields.filter(field => !field.scripted);
}

if (this.filterFieldTypes) {
fields = $filter('fieldType')(fields, this.filterFieldTypes);
fields = $filter('orderBy')(fields, ['type', 'name']);
}


return new IndexedArray({
index: ['name'],
group: ['type'],
initialSet: fields
});
};

/**
* Called to read values from a database record into the
* aggConfig object
Expand All @@ -33,13 +63,18 @@ export default function FieldAggParamFactory(Private) {
* @return {field}
*/
FieldAggParam.prototype.deserialize = function (fieldName, aggConfig) {
let field = aggConfig.vis.indexPattern.fields.byName[fieldName];
const field = aggConfig.getIndexPattern().fields.byName[fieldName];

if (!field) {
throw new SavedObjectNotFound('index-pattern-field', fieldName);
}

return field;
const validField = this.getFieldOptions(aggConfig).byName[fieldName];
if (!validField) {
notifier.error(`Saved "field" parameter is now invalid. Please select a new field.`);
}

return validField;
};

/**
Expand All @@ -56,7 +91,7 @@ export default function FieldAggParamFactory(Private) {
let field = aggConfig.getField();

if (!field) {
throw new Error(`"${aggConfig.makeLabel()}" requires a field`);
throw new TypeError('"field" is a required parameter');
Copy link
Contributor

Choose a reason for hiding this comment

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

is this just an unrelated improvement?

Copy link
Contributor Author

@spalger spalger Oct 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Somewhat, in a few situations I saw this cause call-stack overflows because certain aggs call write() on all of their params in their makeLabel() function

_Edit:_ This is more likely to happen now that this branch has the field validation logic too. Since we are changing the field options of the significant terms agg, if someone had a saved visualization with a scripted field for a signification terms agg this would have caused a call stack overflow

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough

}

if (field.scripted) {
Expand Down
19 changes: 6 additions & 13 deletions src/ui/public/index_patterns/__tests__/_index_pattern.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,11 @@ import IndexPatternsMapperProvider from 'ui/index_patterns/_mapper';
import UtilsMappingSetupProvider from 'ui/utils/mapping_setup';
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain the reason for the changes in this file? I don't understand how they connect to the agg changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that the stubbed logstash index pattern fields have aggregatable and searchable properties, the call to refresh the fields is not happening, which means that the stubbed version of mapper.getFieldsForIndexPattern() is not being used and the strategic points where bluebird promises were injected are no longer correct. Rather than with the fragile practice of only replacing the promises we need, the no_disgest_promises test until just replaces our Promise angular module with Bluebird promises, so that they resolve like normal and don't require calls to $rootScope.$apply().

import IndexPatternsIntervalsProvider from 'ui/index_patterns/_intervals';
import IndexPatternsIndexPatternProvider from 'ui/index_patterns/_index_pattern';
import NoDigestPromises from 'test_utils/no_digest_promises';

describe('index pattern', function () {
NoDigestPromises.activateForSuite();

let IndexPattern;
let mapper;
let mappingSetup;
Expand Down Expand Up @@ -55,7 +58,7 @@ describe('index pattern', function () {

// stub calculateIndices
calculateIndices = sinon.spy(function () {
return $injector.get('Promise').resolve([
return Promise.resolve([
{ index: 'foo', max: Infinity, min: -Infinity },
{ index: 'bar', max: Infinity, min: -Infinity }
]);
Expand Down Expand Up @@ -150,7 +153,6 @@ describe('index pattern', function () {

describe('refresh fields', function () {
// override the default indexPattern, with a truncated field list
require('test_utils/no_digest_promises').activateForSuite();
const indexPatternId = 'test-pattern';
let indexPattern;
let fieldLength;
Expand Down Expand Up @@ -321,7 +323,6 @@ describe('index pattern', function () {
});

describe('#toDetailedIndexList', function () {
require('test_utils/no_digest_promises').activateForSuite();
context('when index pattern is an interval', function () {
let interval;
beforeEach(function () {
Expand Down Expand Up @@ -400,7 +401,6 @@ describe('index pattern', function () {

describe('#toIndexList', function () {
context('when index pattern is an interval', function () {
require('test_utils/no_digest_promises').activateForSuite();

let interval;
beforeEach(function () {
Expand Down Expand Up @@ -431,7 +431,6 @@ describe('index pattern', function () {
});

context('when index pattern is a time-base wildcard', function () {
require('test_utils/no_digest_promises').activateForSuite();
beforeEach(function () {
sinon.stub(indexPattern, 'getInterval').returns(false);
sinon.stub(indexPattern, 'hasTimeField').returns(true);
Expand All @@ -453,7 +452,6 @@ describe('index pattern', function () {
});

context('when index pattern is a time-base wildcard that is configured not to expand', function () {
require('test_utils/no_digest_promises').activateForSuite();
beforeEach(function () {
sinon.stub(indexPattern, 'getInterval').returns(false);
sinon.stub(indexPattern, 'hasTimeField').returns(true);
Expand All @@ -472,13 +470,8 @@ describe('index pattern', function () {
sinon.stub(indexPattern, 'getInterval').returns(false);
});

it('is fulfilled by id', function () {
let indexList;
indexPattern.toIndexList().then(function (val) {
indexList = val;
});
$rootScope.$apply();

it('is fulfilled by id', async function () {
let indexList = await indexPattern.toIndexList();
expect(indexList).to.equal(indexPattern.id);
});
});
Expand Down
4 changes: 2 additions & 2 deletions src/ui/public/vis/__tests__/_agg_config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,12 +481,12 @@ describe('AggConfig', function () {
{
type: 'avg',
schema: 'metric',
params: { field: 'ssl' }
params: { field: 'bytes' }
}
]
});

let field = indexPattern.fields.byName.ssl;
let field = indexPattern.fields.byName.bytes;
Copy link
Contributor

Choose a reason for hiding this comment

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

ssl -> bytes makes sense, but why timestamp -> time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@timestamp isn't a field in the stubbed logstash index pattern, so this fails the new valid check in #8723

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was incorrect, I misread the field list and miscategorized the issue. Removed the changes.

expect(vis.aggs[0].fieldFormatter('html')).to.be(field.format.getConverterFor('html'));
});
});
Expand Down
22 changes: 17 additions & 5 deletions src/ui/public/vis/agg_config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,13 +148,11 @@ export default function AggConfigFactory(Private, fieldTypeFilter) {
* @return {object} the new params object
*/
AggConfig.prototype.resetParams = function () {
let fieldParam = this.type && this.type.params.byName.field;
let field;
const fieldOptions = this.getFieldOptions();

if (fieldParam) {
let prevField = this.params.field;
let fieldOpts = fieldTypeFilter(this.vis.indexPattern.fields, fieldParam.filterFieldTypes);
field = _.contains(fieldOpts, prevField) ? prevField : null;
if (fieldOptions) {
field = fieldOptions.byName[this.fieldName()] || null;
}

return this.fillDefaults({ row: this.params.row, field: field });
Expand Down Expand Up @@ -286,6 +284,20 @@ export default function AggConfigFactory(Private, fieldTypeFilter) {
return pre += this.type.makeLabel(this);
};

AggConfig.prototype.getIndexPattern = function () {
return this.vis.indexPattern;
};

AggConfig.prototype.getFieldOptions = function () {
const fieldParamType = this.type && this.type.params.byName.field;

if (!fieldParamType || !fieldParamType.getFieldOptions) {
return null;
}

return fieldParamType.getFieldOptions(this);
};

AggConfig.prototype.fieldFormatter = function (contentType, defaultFormat) {
let format = this.type && this.type.getFormat(this);
if (format) return format.getConverterFor(contentType);
Expand Down