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 date histogram time base configuration to EditorConfig #22344

Merged
merged 10 commits into from
Sep 7, 2018
18 changes: 15 additions & 3 deletions packages/kbn-datemath/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,20 @@

import moment from 'moment';

const units = ['y', 'M', 'w', 'd', 'h', 'm', 's', 'ms'];
const unitsDesc = units;
const unitsAsc = [...unitsDesc].reverse();
const unitsMap = {
ms: { weight: 1, type: 'fixed', base: 1 },
s: { weight: 2, type: 'fixed', base: 1000 },
m: { weight: 3, type: 'mixed', base: 1000 * 60 },
h: { weight: 4, type: 'mixed', base: 1000 * 60 * 60 },
d: { weight: 5, type: 'mixed', base: 1000 * 60 * 60 * 24 },
w: { weight: 6, type: 'calendar' },
M: { weight: 7, type: 'calendar' },
// q: { weight: 8, type: 'calendar' }, // TODO: moment duration does not support quarter
Copy link
Contributor Author

Choose a reason for hiding this comment

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

adding q to this list introduces all sorts of problems later down the stack due to moment duration's lack of support for this unit

y: { weight: 9, type: 'calendar' },
};
const units = Object.keys(unitsMap).sort((a, b) => unitsMap[a].weight - unitsMap[b].weight);
const unitsDesc = [...units].reverse();
const unitsAsc = [...units];

const isDate = d => Object.prototype.toString.call(d) === '[object Date]';

Expand Down Expand Up @@ -142,6 +153,7 @@ function parseDateMath(mathString, time, roundUp) {

export default {
parse: parse,
unitsMap: Object.freeze(unitsMap),
units: Object.freeze(units),
unitsAsc: Object.freeze(unitsAsc),
unitsDesc: Object.freeze(unitsDesc),
Expand Down
16 changes: 7 additions & 9 deletions src/ui/public/agg_types/controls/number_interval.html
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,6 @@
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
Copy link
Member

Choose a reason for hiding this comment

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

why are we changing the number_interval template ? its used by histogram not date_histogram ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ppisljar I changed the previous warning config to help instead to reflect the purpose better (to help users understand limitations). histogram supported warning so its template has been updated to use help here and will display the text below the input field.

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 @@ -27,4 +18,11 @@
step="{{editorConfig.interval.base}}"
input-number
>
<div
ng-if="editorConfig.interval.help"
class="kuiSubText kuiSubduedText kuiVerticalRhythmSmall"
style="margin-top: 5px"
>
<span>{{editorConfig.interval.help}}</span>
</div>
</div>
10 changes: 9 additions & 1 deletion src/ui/public/agg_types/controls/time_interval.html
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
></icon-tip>
</label>
<select
ng-if="!editorConfig.customInterval.timeBase"
id="visEditorInterval{{agg.id}}"
ng-model="agg.params.interval"
ng-change="agg.write()"
Expand All @@ -23,9 +24,16 @@
type="text"
name="customInterval"
ng-model="agg.params.customInterval"
validate-date-interval
validate-date-interval="{{editorConfig.customInterval.timeBase}}"
ng-change="aggForm.customInterval.$valid && agg.write()"
ng-if="agg.params.interval.val == 'custom'"
class="form-control"
required />
<div
ng-if="editorConfig.customInterval.help"
class="kuiSubText kuiSubduedText kuiVerticalRhythmSmall"
style="margin-top: 5px"
>
<span>{{editorConfig.customInterval.help}}</span>
</div>
</div>
57 changes: 57 additions & 0 deletions src/ui/public/utils/__tests__/parse_es_interval.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/*
* 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.
*/

import { parseEsInterval } from '../parse_es_interval';

This comment was marked as resolved.

import expect from 'expect.js';

describe('parseEsInterval', function () {
it('should correctly parse an interval containing unit and single value', function () {
expect(parseEsInterval('1ms')).to.eql({ value: 1, unit: 'ms', type: 'fixed' });
expect(parseEsInterval('1s')).to.eql({ value: 1, unit: 's', type: 'fixed' });
expect(parseEsInterval('1m')).to.eql({ value: 1, unit: 'm', type: 'calendar' });
expect(parseEsInterval('1h')).to.eql({ value: 1, unit: 'h', type: 'calendar' });
expect(parseEsInterval('1d')).to.eql({ value: 1, unit: 'd', type: 'calendar' });
expect(parseEsInterval('1w')).to.eql({ value: 1, unit: 'w', type: 'calendar' });
expect(parseEsInterval('1M')).to.eql({ value: 1, unit: 'M', type: 'calendar' });
expect(parseEsInterval('1y')).to.eql({ value: 1, unit: 'y', type: 'calendar' });
});

it('should correctly parse an interval containing unit and multiple value', function () {
expect(parseEsInterval('250ms')).to.eql({ value: 250, unit: 'ms', type: 'fixed' });
expect(parseEsInterval('90s')).to.eql({ value: 90, unit: 's', type: 'fixed' });
expect(parseEsInterval('60m')).to.eql({ value: 60, unit: 'm', type: 'fixed' });
expect(parseEsInterval('12h')).to.eql({ value: 12, unit: 'h', type: 'fixed' });
expect(parseEsInterval('7d')).to.eql({ value: 7, unit: 'd', type: 'fixed' });
});

it('should throw an error for intervals containing calendar unit and multiple value', function () {
expect(parseEsInterval).withArgs('4w').to.throwError();
expect(parseEsInterval).withArgs('12M').to.throwError();
expect(parseEsInterval).withArgs('10y').to.throwError();
});

it('should throw an error for invalid interval formats', function () {
expect(parseEsInterval).withArgs('1').to.throwError();
expect(parseEsInterval).withArgs('h').to.throwError();
expect(parseEsInterval).withArgs('0m').to.throwError();
expect(parseEsInterval).withArgs('0.5h').to.throwError();
expect(parseEsInterval).withArgs().to.throwError();
expect(parseEsInterval).withArgs({}).to.throwError();
});
});
51 changes: 51 additions & 0 deletions src/ui/public/utils/parse_es_interval.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/*
* 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.
*/
import dateMath from '@kbn/datemath';

const ES_INTERVAL_STRING_RE = new RegExp('^([1-9][0-9]*)\\s*(' + dateMath.units.join('|') + ')$');

This comment was marked as resolved.


/**
* Strict version of parseInterval(), enforces ES interval format, and disallows fractional values

This comment was marked as resolved.

*
* @param interval
* @returns {{amount: (RegExpMatchArray | null) | number, unit: (RegExpMatchArray | null) | string, type: string}}

This comment was marked as resolved.

*/
export function parseEsInterval(interval = '') {

This comment was marked as resolved.

const matches = String(interval)
.trim()
.match(ES_INTERVAL_STRING_RE);

if (!matches) {
throw Error(`Invalid interval format: ${interval}`);
}

const value = matches && parseFloat(matches[1]);
const unit = matches && matches[2];
const type = unit && dateMath.unitsMap[unit].type;

if (type === 'calendar' && value !== 1) {
throw Error(`Invalid calendar interval: ${interval}, value must be 1`);
}

return {
value,
unit,
type: (type === 'mixed' && value === 1) || type === 'calendar' ? 'calendar' : 'fixed',
};
}
20 changes: 19 additions & 1 deletion src/ui/public/validate_date_interval.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import { parseInterval } from './utils/parse_interval';
import { uiModules } from './modules';
import { leastCommonInterval } from './vis/lib/least_common_interval';

uiModules
.get('kibana')
Expand All @@ -27,14 +28,31 @@ uiModules
restrict: 'A',
require: 'ngModel',
link: function ($scope, $el, attrs, ngModelCntrl) {
const baseInterval = attrs.validateDateInterval || null;
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we are trying to get anyway rid of Angular, but could we use an @ scope binding for validateDateInterval instead of accessing it via attrs here. That way we also immediately have the scope of this directive isolated.


ngModelCntrl.$parsers.push(check);
ngModelCntrl.$formatters.push(check);

function check(value) {
ngModelCntrl.$setValidity('dateInterval', parseInterval(value) != null);
if(baseInterval) {
ngModelCntrl.$setValidity('dateInterval', parseWithBase(value) === true);
} else {
ngModelCntrl.$setValidity('dateInterval', parseInterval(value) != null);
}
return value;
}

// When base interval is set, check for least common interval and allow
// input the value is the same. This means that the input interval is a
// multiple of the base interval.
function parseWithBase(value) {
try {
const lci = leastCommonInterval(baseInterval, value);
return lci === value.replace(/\s/g, '');

This comment was marked as resolved.

} catch(e) {
return false;
}
}
}
};
});
Expand Down
67 changes: 61 additions & 6 deletions src/ui/public/vis/editors/config/editor_config_providers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
*/

import { EditorConfigProviderRegistry } from './editor_config_providers';
import { EditorParamConfig, FixedParam, NumericIntervalParam } from './types';
import { EditorParamConfig, FixedParam, NumericIntervalParam, TimeIntervalParam } from './types';

describe('EditorConfigProvider', () => {
let registry: EditorConfigProviderRegistry;
Expand Down Expand Up @@ -111,6 +111,49 @@ describe('EditorConfigProvider', () => {
}).toThrowError();
});

it('should allow same timeBase values', () => {
registry.register(singleConfig({ timeBase: '2h', default: '2h' }));
registry.register(singleConfig({ timeBase: '2h', default: '2h' }));
const config = getOutputConfig(registry) as TimeIntervalParam;
expect(config).toHaveProperty('timeBase');
expect(config).toHaveProperty('default');
expect(config.timeBase).toBe('2h');
expect(config.default).toBe('2h');
});

it('should merge multiple compatible timeBase values, using least common interval', () => {
registry.register(singleConfig({ timeBase: '2h', default: '2h' }));
registry.register(singleConfig({ timeBase: '3h', default: '3h' }));
registry.register(singleConfig({ timeBase: '4h', default: '4h' }));
const config = getOutputConfig(registry) as TimeIntervalParam;
expect(config).toHaveProperty('timeBase');
expect(config).toHaveProperty('default');
expect(config.timeBase).toBe('12h');
expect(config.default).toBe('12h');
});

it('should throw on combining incompatible timeBase values', () => {
registry.register(singleConfig({ timeBase: '2h', default: '2h' }));
registry.register(singleConfig({ timeBase: '1d', default: '1d' }));
expect(() => {
getOutputConfig(registry);
}).toThrowError();
});

it('should throw on invalid timeBase values', () => {
registry.register(singleConfig({ timeBase: '2w', default: '2w' }));
expect(() => {
getOutputConfig(registry);
}).toThrowError();
});

it('should throw if timeBase and default are different', () => {
registry.register(singleConfig({ timeBase: '1h', default: '2h' }));
expect(() => {
getOutputConfig(registry);
}).toThrowError();
});

it('should merge hidden together with fixedValue', () => {
registry.register(singleConfig({ fixedValue: 'foo', hidden: true }));
registry.register(singleConfig({ fixedValue: 'foo', hidden: false }));
Expand All @@ -131,12 +174,24 @@ describe('EditorConfigProvider', () => {
expect(config.hidden).toBe(false);
});

it('should merge warnings together into one string', () => {
registry.register(singleConfig({ warning: 'Warning' }));
registry.register(singleConfig({ warning: 'Another warning' }));
it('should merge hidden together with timeBase', () => {
registry.register(singleConfig({ timeBase: '2h', default: '2h', hidden: false }));
registry.register(singleConfig({ timeBase: '4h', default: '4h', hidden: false }));
const config = getOutputConfig(registry) as TimeIntervalParam;
expect(config).toHaveProperty('timeBase');
expect(config).toHaveProperty('default');
expect(config).toHaveProperty('hidden');
expect(config.timeBase).toBe('4h');
expect(config.default).toBe('4h');
expect(config.hidden).toBe(false);
});

it('should merge helps together into one string', () => {
registry.register(singleConfig({ help: 'Warning' }));
registry.register(singleConfig({ help: 'Another help' }));
const config = getOutputConfig(registry);
expect(config).toHaveProperty('warning');
expect(config.warning).toBe('Warning\n\nAnother warning');
expect(config).toHaveProperty('help');
expect(config.help).toBe('Warning\n\nAnother help');
});
});
});
Loading