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

Introduce EditorConfigProvider to customize vis editor #20519

Merged
merged 13 commits into from
Aug 10, 2018
83 changes: 44 additions & 39 deletions src/ui/public/agg_types/__tests__/agg_param_writer.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,55 +46,60 @@ export default function AggParamWriterHelper(Private) {
* @param {object} opts - describe the properties of this paramWriter
* @param {string} opts.aggType - the name of the aggType we want to test. ('histogram', 'filter', etc.)
*/
function AggParamWriter(opts) {
const self = this;
class AggParamWriter {

self.aggType = opts.aggType;
if (_.isString(self.aggType)) {
self.aggType = aggTypes.byName[self.aggType];
}
constructor(opts) {
const self = this;
Copy link
Member

Choose a reason for hiding this comment

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

get rid of self


// not configurable right now, but totally required
self.indexPattern = stubbedLogstashIndexPattern;
self.aggType = opts.aggType;
if (_.isString(self.aggType)) {
self.aggType = aggTypes.byName[self.aggType];
}

// the schema that the aggType satisfies
self.visAggSchema = null;
// not configurable right now, but totally required
self.indexPattern = stubbedLogstashIndexPattern;

self.vis = new Vis(self.indexPattern, {
type: 'histogram',
aggs: [{
id: 1,
type: self.aggType.name,
params: {}
}]
});
}
// the schema that the aggType satisfies
self.visAggSchema = null;

self.vis = new Vis(self.indexPattern, {
type: 'histogram',
aggs: [{
id: 1,
type: self.aggType.name,
params: {}
}]
});
}

AggParamWriter.prototype.write = function (paramValues) {
const self = this;
paramValues = _.clone(paramValues);
write(paramValues, modifyAggConfig = null) {
const self = this;
Copy link
Member

Choose a reason for hiding this comment

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

same as above

paramValues = _.clone(paramValues);

if (self.aggType.params.byName.field && !paramValues.field) {
// pick a field rather than force a field to be specified everywhere
if (self.aggType.type === 'metrics') {
paramValues.field = _.sample(self.indexPattern.fields.byType.number);
} else {
const type = self.aggType.params.byName.field.filterFieldTypes || 'string';
let field;
do {
field = _.sample(self.indexPattern.fields.byType[type]);
} while (!field.aggregatable);
paramValues.field = field.name;
if (self.aggType.params.byName.field && !paramValues.field) {
// pick a field rather than force a field to be specified everywhere
if (self.aggType.type === 'metrics') {
paramValues.field = _.sample(self.indexPattern.fields.byType.number);
} else {
const type = self.aggType.params.byName.field.filterFieldTypes || 'string';
let field;
do {
field = _.sample(self.indexPattern.fields.byType[type]);
} while (!field.aggregatable);
paramValues.field = field.name;
}
}
}

const aggConfig = self.vis.aggs[0];
aggConfig.setParams(paramValues);

const aggConfig = self.vis.aggs[0];
aggConfig.setParams(paramValues);
if (modifyAggConfig) {
modifyAggConfig(aggConfig);
}

return aggConfig.write(self.vis.aggs);
};
return aggConfig.write(self.vis.aggs);
}
}

return AggParamWriter;

}
61 changes: 61 additions & 0 deletions src/ui/public/agg_types/__tests__/buckets/_histogram.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,13 @@
*/

import expect from 'expect.js';
import sinon from 'sinon';
import ngMock from 'ng_mock';
import { aggTypes } from '../..';
import chrome from '../../../chrome';
import AggParamWriterProvider from '../agg_param_writer';

const config = chrome.getUiSettingsClient();
const histogram = aggTypes.byName.histogram;
describe('Histogram Agg', function () {

Expand All @@ -46,6 +49,13 @@ describe('Histogram Agg', function () {
paramWriter = new AggParamWriter({ aggType: 'histogram' });
}));

describe('intervalBase', () => {
it('should not be written to the DSL', () => {
const output = paramWriter.write({ intervalBase: 100 });
expect(output.params).not.to.have.property('intervalBase');
});
});

describe('interval', function () {
// reads aggConfig.params.interval, writes to dsl.interval

Expand Down Expand Up @@ -74,6 +84,57 @@ describe('Histogram Agg', function () {
const output = paramWriter.write({ interval: [] });
expect(isNaN(output.params.interval)).to.be.ok();
});

describe('interval scaling', () => {

beforeEach(() => {
sinon.stub(config, 'get');
});

it('will respect the histogram:maxBars setting', () => {
config.get.withArgs('histogram:maxBars').returns(5);
const output = paramWriter.write({ interval: 5 },
aggConfig => aggConfig.setAutoBounds({ min: 0, max: 10000 }));
expect(output.params).to.have.property('interval', 2000);
});

it('will return specified interval, if bars are below histogram:maxBars config', () => {
config.get.withArgs('histogram:maxBars').returns(10000);
const output = paramWriter.write({ interval: 5 },
aggConfig => aggConfig.setAutoBounds({ min: 0, max: 10000 }));
expect(output.params).to.have.property('interval', 5);
});

it('will set to intervalBase if interval is below base', () => {
const output = paramWriter.write({ interval: 3, intervalBase: 8 });
expect(output.params).to.have.property('interval', 8);
});

it('will round to nearest intervalBase multiple if interval is above base', () => {
const roundUp = paramWriter.write({ interval: 46, intervalBase: 10 });
expect(roundUp.params).to.have.property('interval', 50);
const roundDown = paramWriter.write({ interval: 43, intervalBase: 10 });
expect(roundDown.params).to.have.property('interval', 40);
});

it('will not change interval if it is a multiple of base', () => {
const output = paramWriter.write({ interval: 35, intervalBase: 5 });
expect(output.params).to.have.property('interval', 35);
});

it('will round to intervalBase after scaling histogram:maxBars', () => {
config.get.withArgs('histogram:maxBars').returns(100);
const output = paramWriter.write({ interval: 5, intervalBase: 6 },
aggConfig => aggConfig.setAutoBounds({ min: 0, max: 1000 }));
// 100 buckets in 0 to 1000 would result in an interval of 10, so we should
// round to the next multiple of 6 -> 12
expect(output.params).to.have.property('interval', 12);
});

afterEach(() => {
config.get.restore();
});
});
});

describe('min_doc_count', function () {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,17 @@
import _ from 'lodash';
import moment from 'moment';
import expect from 'expect.js';
import sinon from 'sinon';
import ngMock from 'ng_mock';
import AggParamWriterProvider from '../../agg_param_writer';
import FixturesStubbedLogstashIndexPatternProvider from 'fixtures/stubbed_logstash_index_pattern';
import chrome from '../../../../chrome';
import { aggTypes } from '../../..';
import { AggConfig } from '../../../../vis/agg_config';
import { timefilter } from 'ui/timefilter';

const config = chrome.getUiSettingsClient();

describe('params', function () {

let paramWriter;
Expand Down Expand Up @@ -132,6 +136,30 @@ describe('params', function () {
});
});

describe('time_zone', () => {
beforeEach(() => {
sinon.stub(config, 'get');
sinon.stub(config, 'isDefault');
});

it('should use the specified time_zone', () => {
const output = paramWriter.write({ time_zone: 'Europe/Kiev' });
expect(output.params).to.have.property('time_zone', 'Europe/Kiev');
});

it('should use the Kibana time_zone if no parameter specified', () => {
config.isDefault.withArgs('dateFormat:tz').returns(false);
config.get.withArgs('dateFormat:tz').returns('Europe/Riga');
const output = paramWriter.write({});
expect(output.params).to.have.property('time_zone', 'Europe/Riga');
});

afterEach(() => {
config.get.restore();
config.isDefault.restore();
});
});

describe('extended_bounds', function () {
it('should write a long value if a moment passed in', function () {
const then = moment(0);
Expand Down
16 changes: 7 additions & 9 deletions src/ui/public/agg_types/buckets/date_histogram.js
Original file line number Diff line number Diff line change
Expand Up @@ -128,13 +128,6 @@ export const dateHistogramBucketAgg = new BucketAggType({
output.bucketInterval = interval;
output.params.interval = interval.expression;

const isDefaultTimezone = config.isDefault('dateFormat:tz');
if (isDefaultTimezone) {
output.params.time_zone = detectedTimezone || tzOffset;
} else {
output.params.time_zone = config.get('dateFormat:tz');
}

const scaleMetrics = interval.scaled && interval.scale < 1;
if (scaleMetrics && aggs) {
const all = _.every(aggs.bySchemaGroup.metrics, function (agg) {
Expand All @@ -147,13 +140,18 @@ export const dateHistogramBucketAgg = new BucketAggType({
}
}
},

{
name: 'time_zone',
default: () => {
const isDefaultTimezone = config.isDefault('dateFormat:tz');
return isDefaultTimezone ? detectedTimezone || tzOffset : config.get('dateFormat:tz');
},
},
{
name: 'customInterval',
default: '2h',
write: _.noop
},

{
name: 'format'
},
Expand Down
21 changes: 20 additions & 1 deletion src/ui/public/agg_types/buckets/histogram.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,15 @@ export const histogramBucketAgg = new BucketAggType({
name: 'field',
filterFieldTypes: 'number'
},

{
/*
* This parameter can be set if you want the auto scaled interval to always
* be a multiple of a specific base.
*/
name: 'intervalBase',
default: null,
write: () => {},

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an explicit test for that

},
{
name: 'interval',
editor: intervalTemplate,
Expand Down Expand Up @@ -111,6 +119,17 @@ export const histogramBucketAgg = new BucketAggType({
}
}

const base = aggConfig.params.intervalBase;
if (base) {
if (interval < base) {
// In case the specified interval is below the base, just increase it to it's base
interval = base;
} else if (interval % base !== 0) {
// In case the interval is not a multiple of the base round it to the next base
interval = Math.round(interval / base) * base;
}
}

output.params.interval = interval;
}
},
Expand Down
12 changes: 11 additions & 1 deletion src/ui/public/agg_types/controls/number_interval.html
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,15 @@
position="'right'"
content="'Interval will be automatically scaled in the event that the provided value creates more buckets than specified by Advanced Setting\'s histogram:maxBars'"
></icon-tip>

<icon-tip
ng-if="editorConfig.interval.warning"
position="'right'"
content="editorConfig.interval.warning"
type="'alert'"
color="'warning'"
style="float: right"
></icon-tip>
</label>
<input
id="visEditorInterval{{agg.id}}"
Expand All @@ -14,7 +23,8 @@
type="number"
class="form-control"
name="interval"
min="0"
min="{{editorConfig.interval.base || 0}}"
step="{{editorConfig.interval.base}}"

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I have editor hints still on the list, but wanted to pull it into a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually still added it to this PR.

input-number
>
</div>
2 changes: 1 addition & 1 deletion src/ui/public/react_components.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,6 @@ app.directive('icon', reactDirective => reactDirective(EuiIcon));

app.directive('colorPicker', reactDirective => reactDirective(EuiColorPicker));

app.directive('iconTip', reactDirective => reactDirective(EuiIconTip, ['content', 'type', 'position', 'title']));
app.directive('iconTip', reactDirective => reactDirective(EuiIconTip, ['content', 'type', 'position', 'title', 'color']));

app.directive('callOut', reactDirective => reactDirective(EuiCallOut, ['title', 'color', 'size', 'iconType', 'children']));
Loading