Introduce EditorConfigProvider to customize vis editor #20519

timroes · 2018-07-06T14:51:52Z

This PR introduces the basic EditorConfigProvider which will be needed for roll-up support to limit down parameters to specific values. It introduces the base, fixedValue and hidden configurations.

It does not yet implement anything around date histograms and date interval multiples. Those will be in a separate PR to keep the size of the PRs reviewable.

Sample code on how to use this:

editorConfigProviders.register((aggType, indexPattern, aggConfig) => {
  if (aggConfig.params.field && aggConfig.params.field.name === 'bytes') {
    return {
      'intervalBase': {
        fixedValue: 50
      },
      'interval': {
        base: 50,
        warning: 'You can only specify multiples of 50, because of roll-up...',
      }
    };
  }
  if (aggConfig.type && aggConfig.type.name === 'date_histogram') {
    return {
      'time_zone': {
        fixedValue: 'UTC'
      }
    };
  }
  return {};
});

@jen-huang Could you please check again if this PR satisfies the need around base, fixed values and hiding. Also since we don't leverage any of those yet in the UI, we cannot provide functional tests with those. Please make sure that roll-up feature has a high coverage on functional tests, that the vis editor really behaves properly (hides, only allow multiples, etc.) according to the configuration.

elasticmachine · 2018-07-06T16:04:16Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-07-11T14:06:49Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-07-12T07:52:58Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-07-12T09:48:41Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-08-06T17:45:48Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cjcenizal

This looks great @timroes! Thanks for all of the comments and clear tests. I had some suggestions but nothing major.

cjcenizal · 2018-08-06T18:00:11Z

src/ui/public/vis/editors/config/editor_config_providers.test.ts

+      expect(config.base).toBe(40);
+    });
+
+    it('should throw on combinding fixedValue with base', () => {


Typo: "combinding" -> "combining"

cjcenizal · 2018-08-06T18:02:08Z

src/ui/public/vis/editors/config/editor_config_providers.ts

+      // If not we'll throw an error.
+      throw new Error(`Two EditorConfigProviders provided different fixed values for field ${paramName}:
+          ${merged.fixedValue} !== ${current.fixedValue}`);
+    } else if (


I think this can be just a regular if instead of else if, since the first condition is an early exit. This will make the code a bit easier to scan.

cjcenizal · 2018-08-06T18:02:13Z

src/ui/public/vis/editors/config/editor_config_providers.ts

+      // that are the multiple of the specific base value, but since there is no use-case for that
+      // right now, this isn't implemented.
+      throw new Error(`Tried to provide a fixedValue and a base for param ${paramName}.`);
+    } else if (this.isBaseParam(current) && this.isBaseParam(merged)) {


cjcenizal · 2018-08-06T18:02:29Z

src/ui/public/vis/editors/config/editor_config_providers.ts

+      return {
+        base: leastCommonMultiple(current.base, merged.base),
+      };
+    } else {


This doesn't need to be inside a condition at all, since the previous conditions all throw or return.

cjcenizal · 2018-08-06T18:02:43Z

src/ui/public/vis/editors/config/editor_config_providers.ts

+        return {
+          fixedValue: current.fixedValue,
+        };
+      } else if (this.isBaseParam(current)) {


This can just be an if.

cjcenizal · 2018-08-06T18:05:55Z

src/ui/public/vis/editors/default/agg_params.js

@@ -28,6 +28,29 @@ import { uiModules } from '../../../modules';
 import { documentationLinks } from '../../../documentation_links/documentation_links';
 import aggParamsTemplate from './agg_params.html';
 import { aggTypeFilters } from '../../../agg_types/filter';
+import { editorConfigProviders } from '../config/editor_config_providers';
+
+// TODO: This is just an example how to use this, and will be removed later.


Does this still need to be here? It does seem useful, so maybe we should remove the TODO annotation and just change this comment to be This is an example of how to use this.?

Wait why is this still there? Oh yeah I forgot to push my last commit from yesterday :D

cjcenizal · 2018-08-06T18:06:55Z

src/ui/public/vis/editors/default/agg_params.js

+            if (config.hasOwnProperty('fixedValue')) {
+              $scope.agg.params[param] = config.fixedValue;
+            }
+            // TODO: process more editor config?


What do you mean by this?

That should have been removed

cjcenizal · 2018-08-06T18:07:15Z

src/ui/public/vis/editors/default/agg_params.js

-              const isExtraParam = i > 0;
-              if (!hasIndexedFields && isExtraParam) { // don't draw the rest of the options if there are no indexed fields.
+          $scope.agg.type.params
+            .filter(param => !get($scope, ['editorConfig', param.name, 'hidden'], false))


To make this easier to scan, can we assign this to a new variable or add a comment explaining what the intention is?

const visibleParams = $scope.agg.type.params.filter(param => !get($scope, ['editorConfig', param.name, 'hidden'], false));

cjcenizal · 2018-08-06T18:09:19Z

src/ui/public/vis/editors/config/editor_config_providers.ts

+  aggConfig: AggConfig
+) => EditorConfig;
+
+function greatestCommonDivisor(a: number, b: number): number {


@jen-huang Weren't you looking for utilities like this?

These seem general enough to belong in their own math module in ui/public, with a couple unit tests. What do you think @timroes?

Yeap, I will move them

cjcenizal · 2018-08-06T18:12:03Z

src/ui/public/vis/editors/config/editor_config_providers.ts

+    return configs.reduce((output, conf) => {
+      Object.entries(conf).forEach(([paramName, paramConfig]) => {
+        if (!output[paramName]) {
+          output[paramName] = paramConfig;


I know we're not mutating paramConfig anywhere in this code but is it possible someone might add code that adds properties to it in the future (outside of this module)? If so, is it worth being defensive and just creating a shallow copy here? After all, we do that on line 122 so we may as well retain parity.

output[paramName] = { ... paramConfig };

elasticmachine · 2018-08-07T12:05:11Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

jen-huang

Great work! I pulled this down and tested by creating a configuration that hides otherBucket and multipleBucket params, as well as configuring interval for histograms and timezone for date histogram based on rollup capabilities.

Just have a small question and suggestion regarding intervals.

    return {
      'intervalBase': {
        fixedValue: 50
      },
      'interval': {
        base: 50
      }
    };

Why do we need both intervalBase and interval? Would there be a case where intervalBase.fixedValue and interval.base are different?

timroes · 2018-08-08T07:06:52Z

src/ui/public/agg_types/controls/number_interval.html

@@ -14,7 +14,8 @@
    type="number"
    class="form-control"
    name="interval"
-    min="0"
+    min="{{editorConfig.interval.base || 0}}"
+    step="{{editorConfig.interval.base}}"


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

I actually still added it to this PR.

timroes · 2018-08-08T08:35:31Z

@jen-huang The reason why there are two different parameters is in the way aggregations in visualizations work. Basically everything needed to calculate the DSL must be an argument to the aggregation configuration, like interval. Since the auto scaling is taking part when calculating the DSL, we'll need to have the base that we want to use available at that point in time, meaning we need to have it as an argument to the aggConfig. So that explains why we need an intervalBase parameter to the pipeline (in addition to the interval which is the targeted interval, but might be scaled).

The other part of that question is: why do you need to write it out separately in the editor config? We could indeed say, whenever we hit a base setting for a parameter (which by itself just configured the UI to have stepped sizes in there), we will also set this value to the <paramName>Base parameter on aggConfig. That way the editor config provider itself would only need to return one thing and we let the editor handle splitting this up into the options it needs (1. fix the UI to it, and 2. set the appropriate base parameter on the aggconfig). My concern why I didn't want to have it that was was more and more implicit behavior in the editor, that noone will understand in a couple of months/years anymore.

We already have a lot of implicit behavior in there, and that way we would need to add another one which potentially is also error prone, like how is the parameter to the aggConfig called, that needs that base value? We always assume <paramName>Base? What if you set base, but the editor won't find a <paramName>Base interval on that aggConfig?

For that reason I rather tend not to do stuff implicitly anymore, but rather make them explicit. You want to set a value for the base that the aggregation uses to calculate - set it explicitly. You want the editor to only allow to configure a specific parameter to a specific base - set it explicitly.

Technically setting those separate would work technically: you could "step" the UI without forcing the scaling to come up with a multiple of that. Or always force the interval it ends up with being a specific multiple, and still let the user configure whatever they want in the UI. Is there a real use-case for that? I don't think so.

elasticmachine · 2018-08-08T13:06:52Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

jen-huang

Thanks for that explanation and adding the warning. LGTM!

ppisljar

left some small comments i hope can still get it, apart from that LGTM, gave just a quick look to the code.

ppisljar · 2018-08-09T13:37:27Z

src/ui/public/agg_types/__tests__/agg_param_writer.js

-      self.aggType = aggTypes.byName[self.aggType];
-    }
+    constructor(opts) {
+      const self = this;


get rid of self

ppisljar · 2018-08-09T13:37:44Z

src/ui/public/agg_types/__tests__/agg_param_writer.js

-    const self = this;
-    paramValues = _.clone(paramValues);
+    write(paramValues, modifyAggConfig = null) {
+      const self = this;


same as above

ppisljar · 2018-08-09T13:41:40Z

src/ui/public/vis/editors/config/editor_config_providers.ts

+import { leastCommonMultiple } from '../../../utils/math';
+import { EditorConfig, EditorParamConfig, FixedParam, NumericIntervalParam } from './types';
+
+type EditorConfigProvider = (


could we use a different name for this ... Provider is currently used to indicate you will need to call it with Private to get the actual thing out of it.

I struggled with that too. The problem is I couldn't come up with a better name (Service would sound more like the central class, and couldn't find much better names). We also just had a discussion with CJ and Jen, and couldn't find any better name. So I'll use CJ's argument here, that we are trying to remove Private in the long run and it shouldn't keep the monopoly on that word, since it makes sense in other places too where it's more conventional.

elasticmachine · 2018-08-10T17:56:04Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Add possibility to specify base for histogram interval * Move AggParamsWriter to ES6 * Add param for time_zone in date_histogram * Prevent writing intervalBase to DSL * Add basic EditorConfig providers * Merge multiple configs together * Allow hiding parameters * Improve config merging and add tests * Remove TODO * Implement review feedback * Add warning to parameter * Remove unneeded self

timroes added WIP Work in progress Feature:Visualizations Generic visualization features (in case no more specific feature label is available) v7.0.0 labels Jul 6, 2018

timroes mentioned this pull request Jul 10, 2018

removing schema references from vis types #20489

Merged

timroes force-pushed the editor-config branch from a240d3b to bf50835 Compare July 11, 2018 12:29

timroes mentioned this pull request Jul 12, 2018

Add AggParamFilters to filter out params in vis editor #20686

Closed

timroes force-pushed the editor-config branch from bf50835 to 77f2cf8 Compare July 12, 2018 06:51

Tim Roes added 7 commits July 12, 2018 10:27

Add possibility to specify base for histogram interval

c14cfe8

Move AggParamsWriter to ES6

c8d2e30

Add param for time_zone in date_histogram

a54e820

Prevent writing intervalBase to DSL

6b45b70

Add basic EditorConfig providers

a3c4c8e

Merge multiple configs together

31bd9e3

Allow hiding parameters

5b3eeed

timroes force-pushed the editor-config branch from cfc647f to 5b3eeed Compare July 12, 2018 08:34

cjcenizal mentioned this pull request Jul 12, 2018

[META] Rollups #20004

Closed

52 tasks

jen-huang mentioned this pull request Jul 23, 2018

Add rollupSearchStrategy #20505

Merged

10 tasks

Tim Roes added 3 commits August 6, 2018 12:11

Merge branch 'master' into editor-config

84a66eb

Improve config merging and add tests

a44b1b5

Remove TODO

7302f58

timroes requested review from ppisljar, cjcenizal, markov00 and jen-huang August 6, 2018 16:30

timroes added v6.5.0 and removed WIP Work in progress labels Aug 6, 2018

timroes changed the title ~~[WIP] Editor config~~ Introduce EditorConfigProvider to customize vis editor Aug 6, 2018

cjcenizal approved these changes Aug 6, 2018

View reviewed changes

Implement review feedback

f235685

jen-huang reviewed Aug 7, 2018

View reviewed changes

Add warning to parameter

d8137b1

jen-huang approved these changes Aug 8, 2018

View reviewed changes

ppisljar approved these changes Aug 9, 2018

View reviewed changes

Remove unneeded self

0bef613

timroes merged commit 133d25a into elastic:master Aug 10, 2018

timroes deleted the editor-config branch August 10, 2018 18:10

timroes mentioned this pull request Aug 10, 2018

[6.x] Introduce EditorConfigProvider to customize vis editor (#20519) #21888

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce EditorConfigProvider to customize vis editor #20519

Introduce EditorConfigProvider to customize vis editor #20519

timroes commented Jul 6, 2018 •

edited

Loading

elasticmachine commented Jul 6, 2018

elasticmachine commented Jul 11, 2018

elasticmachine commented Jul 12, 2018

elasticmachine commented Jul 12, 2018

elasticmachine commented Aug 6, 2018

cjcenizal left a comment

cjcenizal Aug 6, 2018

cjcenizal Aug 6, 2018

cjcenizal Aug 6, 2018

cjcenizal Aug 6, 2018

cjcenizal Aug 6, 2018

cjcenizal Aug 6, 2018 •

edited

Loading

timroes Aug 7, 2018

cjcenizal Aug 6, 2018

timroes Aug 7, 2018

cjcenizal Aug 6, 2018

cjcenizal Aug 6, 2018

timroes Aug 7, 2018

cjcenizal Aug 6, 2018 •

edited

Loading

elasticmachine commented Aug 7, 2018

jen-huang left a comment

This comment was marked as resolved.

timroes Aug 8, 2018

timroes Aug 8, 2018

timroes commented Aug 8, 2018

elasticmachine commented Aug 8, 2018

jen-huang left a comment

ppisljar left a comment

ppisljar Aug 9, 2018

ppisljar Aug 9, 2018

ppisljar Aug 9, 2018

timroes Aug 10, 2018

elasticmachine commented Aug 10, 2018

Introduce EditorConfigProvider to customize vis editor #20519

Introduce EditorConfigProvider to customize vis editor #20519

Conversation

timroes commented Jul 6, 2018 • edited Loading

elasticmachine commented Jul 6, 2018

💚 Build Succeeded

elasticmachine commented Jul 11, 2018

💔 Build Failed

elasticmachine commented Jul 12, 2018

💔 Build Failed

elasticmachine commented Jul 12, 2018

💚 Build Succeeded

elasticmachine commented Aug 6, 2018

💚 Build Succeeded

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Aug 7, 2018

💚 Build Succeeded

jen-huang left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timroes commented Aug 8, 2018

elasticmachine commented Aug 8, 2018

💚 Build Succeeded

jen-huang left a comment

Choose a reason for hiding this comment

ppisljar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Aug 10, 2018

💚 Build Succeeded

timroes commented Jul 6, 2018 •

edited

Loading

cjcenizal Aug 6, 2018 •

edited

Loading

cjcenizal Aug 6, 2018 •

edited

Loading