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

spalger · 2016-10-18T19:57:02Z

The logic for getting the list of fields usable in an aggregations is currently implemented in two places, and both do things slightly different.

This combines the two implementations into aggConfig.getFieldOptions(), which will return either an IndexedArray of fields that can be used for the agg config's field param, or null if the aggConfig does not have a field param.

The purpose of these changes it to enable filtering the field list based on the properties of the agg (in this case the scriptable property) which fixes #8643

spalger · 2016-10-18T22:23:17Z

src/core_plugins/kibana/public/visualize/editor/agg_params.js

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


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

Bargs

Mostly confused by the test changes included in this PR

Bargs · 2016-10-20T21:59:05Z

src/ui/public/agg_response/tabify/__tests__/_response_writer.js

        { type: 'terms', schema: 'segment', params: { field: 'extension' } },
-        { type: 'avg', schema: 'metric', params: { field: '@timestamp' } }
+        { type: 'avg', schema: 'metric', params: { field: 'bytes' } }


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

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

kibana/src/fixtures/logstash_fields.js

Line 5 in 2aca107

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

?

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

Bargs · 2016-10-20T22:05:56Z

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

@@ -94,6 +92,7 @@ describe('AggType Class', function () {
          });

          let aggConfig = vis.aggs.byTypeName.date_histogram[0];
+          const aggType = aggConfig.type;

          expect(aggType.getFormat(aggConfig)).to.be(fieldFormat.getDefaultInstance('date'));


Not sure I understand the changes in this file, tests seem to pass without them?

This only causes tests to fail with the valid check from #8643

Bargs · 2016-10-20T22:08:36Z

src/ui/public/agg_types/param_types/field.js

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


This filter was inside the above conditional in the old code.

Yeah, I figured that wasn't intentional. Can't imagine why anyone would want the fields ordered differently based on the existence of agg.filterFieldTypes

I can't either, but I don't see a reason to risk unintentionally breaking something unless we know the old code was causing some other issue?

Bargs · 2016-10-20T22:11:42Z

src/ui/public/agg_types/param_types/field.js

@@ -56,7 +80,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');


is this just an unrelated improvement?

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

Fair enough

Bargs · 2016-10-20T22:13:56Z

src/ui/public/index_patterns/__tests__/_index_pattern.js

@@ -12,8 +12,11 @@ import IndexPatternsMapperProvider from 'ui/index_patterns/_mapper';
 import UtilsMappingSetupProvider from 'ui/utils/mapping_setup';


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

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

Bargs · 2016-10-20T22:14:48Z

src/ui/public/notify/__tests__/notifier.js

@@ -33,10 +33,13 @@ describe('Notifier', function () {

  beforeEach(function () {


same question as for the index pattern tests, how do these notifier tests relate to the agg changes?

See https://github.com/elastic/kibana/pull/8734/files/2dbb462bc17675d2089f9588aee3090e3e7e1563#r84581647

Bargs · 2016-10-20T22:16:06Z

src/ui/public/route_based_notifier/__tests__/index.js

@@ -10,12 +10,15 @@ describe('ui/route_based_notifier', function () {



Clearing the notifier before each test doesn't clean up the changes you've made. It needs to happen in the after each handler, right?

But what does that have to do with aggregation field options? I'm trying to understand the connection to the purpose of the PR so that I can review it better. If it's not connected, that makes the PR a lot harder to review.

Fair enough, it was useful for debugging the issues but can be done in it's own pr

Bargs · 2016-10-20T22:16:18Z

src/ui/public/test_harness/test_harness.js

@@ -6,6 +6,7 @@ import chrome from 'ui/chrome';
 import Nonsense from 'Nonsense';


Since the courier lives outside of angular, it's state is not reset in between tests. With the new valid check in #8723 calls to notify.error() were breaking the notifier tests, so I figured it would be a good idea to fail a test if it left notifications in the notifier (tests need to clean up after themselves).

Bargs · 2016-10-20T22:20:19Z

src/ui/public/vis/__tests__/_agg_config.js

          }
        ]
      });

-      let field = indexPattern.fields.byName.ssl;
+      let field = indexPattern.fields.byName.bytes;


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

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

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

spalger · 2016-10-22T17:39:46Z

Maybe it wasn't a good idea to pull these changes out of #8723. I don't want to loose the inline feedback by closing this issue but I didn't notice that much of the changes here are only necessary with the new valid check in agg deserialization.

Bargs · 2016-10-24T18:19:40Z

Since #8723 only has a couple comments, perhaps you could pull those last two commits from it into this PR, update this PR's title/description and close #8723? I can't really verify these test changes without the additional changes in that PR anyway.

spalger · 2016-10-25T00:27:48Z

@Bargs merged in #8723, extracted the notifier checks into #8822, and incorporated your feedback

Bargs · 2016-10-25T16:46:11Z

Code looks good, functionality works, one thought though. When I tried loading up a saved, invalid sig terms vis I got a double error at the top of the screen. It might be a little confusing for the user, what do you think?

spalger · 2016-10-25T22:15:03Z

It's less than ideal because they seem so similar, but at least it points the user and gives them a hint why their field parameter is no longer defined.

Bargs · 2016-10-25T22:26:25Z

Maybe we could just make the first one a little more descriptive? Something like ""field" parameter in saved Vis is invalid. Select a new field."

The double message would still be slightly redundant, but it might be a little more clear what's going on.

thomasneirynck

Not an expert on this topic, but LGTM.

Only minor comments.

thomasneirynck · 2016-10-27T04:06:57Z

src/fixtures/logstash_fields.js

+      lang,
+      scripted,
+    };
+  });


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

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

--------- **Commit 1:** [aggConfig] unify field option handling * Original sha: e49ba04 * Authored by spalger <[email protected]> on 2016-10-18T19:45:24Z **Commit 2:** [tests] fix tests that were improperly using fields * Original sha: a9c139a * Authored by spalger <[email protected]> on 2016-10-18T19:56:22Z **Commit 3:** [logstash_fields] remove sortable and filterable, which do nothing * Original sha: 0757c45 * Authored by spalger <[email protected]> on 2016-10-18T22:11:23Z **Commit 4:** [notifier] ensure that notifications are not left around * Original sha: 2dbb462 * Authored by spalger <[email protected]> on 2016-10-18T22:48:01Z **Commit 5:** [aggParams/field] support aggParamType#scriptable * Original sha: 95d704c * Authored by spalger <[email protected]> on 2016-10-17T22:40:04Z **Commit 6:** [aggConfig] add light validation for aggConfig field param * Original sha: 0e11c22 * Authored by spalger <[email protected]> on 2016-10-18T22:47:27Z **Commit 7:** [aggConfig] restore use of `@timestamp` * Original sha: e71b0f2 * Authored by spalger <[email protected]> on 2016-10-25T00:04:13Z **Commit 8:** [tests] revert changes to notifier clearing * Original sha: 6157275 * Authored by spalger <[email protected]> on 2016-10-25T00:07:31Z **Commit 9:** [aggType/test] remove unnecessary change * Original sha: fbf884a * Authored by spalger <[email protected]> on 2016-10-25T00:20:55Z **Commit 10:** [aggTypes/fieldParam] move orderby back into condition * Original sha: 0018786 * Authored by spalger <[email protected]> on 2016-10-25T00:22:46Z **Commit 11:** [aggParams/field] use a more descriptive warning * Original sha: 7205993 * Authored by spalger <[email protected]> on 2016-10-27T18:38:36Z

[backport] PR #8734 to 5.x - Unify getting fields for aggs, and filter scripted fields for significant terms agg

--------- **Commit 1:** [aggConfig] unify field option handling * Original sha: 8a0a6c9df8961b9dc910a2b405b6f7029e96421b [formerly e49ba04] * Authored by spalger <[email protected]> on 2016-10-18T19:45:24Z **Commit 2:** [tests] fix tests that were improperly using fields * Original sha: a7d47ac1d423ebe4181c3adb5283b54670497624 [formerly a9c139a] * Authored by spalger <[email protected]> on 2016-10-18T19:56:22Z **Commit 3:** [logstash_fields] remove sortable and filterable, which do nothing * Original sha: bc102c56db9e76b326597cac7feec5ff4cda200f [formerly 0757c45] * Authored by spalger <[email protected]> on 2016-10-18T22:11:23Z **Commit 4:** [notifier] ensure that notifications are not left around * Original sha: bd5ec0bb0bb23a420bbfd01c4c4a50ca91bb2b53 [formerly 2dbb462] * Authored by spalger <[email protected]> on 2016-10-18T22:48:01Z **Commit 5:** [aggParams/field] support aggParamType#scriptable * Original sha: b5f84211b0a37f711bf9515d4a8c4eff962e1994 [formerly 95d704c] * Authored by spalger <[email protected]> on 2016-10-17T22:40:04Z **Commit 6:** [aggConfig] add light validation for aggConfig field param * Original sha: dec5b418360f6c82a430d8719574c8a5b145c816 [formerly 0e11c22] * Authored by spalger <[email protected]> on 2016-10-18T22:47:27Z **Commit 7:** [aggConfig] restore use of `@timestamp` * Original sha: 0a77eead0688fa2fa82e937ff7079b36e66cfbbe [formerly e71b0f2] * Authored by spalger <[email protected]> on 2016-10-25T00:04:13Z **Commit 8:** [tests] revert changes to notifier clearing * Original sha: c81c25aa61be5afc85a8e03a44eb678a5bf9be97 [formerly 6157275] * Authored by spalger <[email protected]> on 2016-10-25T00:07:31Z **Commit 9:** [aggType/test] remove unnecessary change * Original sha: da013cda95081110696582836a6ac9fab21d8447 [formerly fbf884a] * Authored by spalger <[email protected]> on 2016-10-25T00:20:55Z **Commit 10:** [aggTypes/fieldParam] move orderby back into condition * Original sha: cf19934513af0dd96f0c87d7a835cd22d25588c7 [formerly 0018786] * Authored by spalger <[email protected]> on 2016-10-25T00:22:46Z **Commit 11:** [aggParams/field] use a more descriptive warning * Original sha: ffdd5c74d1055ca94864ba354c159b2b0defbf16 [formerly 7205993] * Authored by spalger <[email protected]> on 2016-10-27T18:38:36Z Former-commit-id: 01ca5dd

[backport] PR elastic#8734 to 5.x - Unify getting fields for aggs, and filter scripted fields for significant terms agg Former-commit-id: fa11c03

spalger added review v5.1.0 v6.0.0 labels Oct 18, 2016

spalger assigned Bargs Oct 18, 2016

spalger mentioned this pull request Oct 18, 2016

No scripted fields for significant_terms #8723

Closed

spalger force-pushed the implement/get-field-options branch 4 times, most recently from 6be3ce4 to e554402 Compare October 18, 2016 22:21

spalger added 3 commits October 18, 2016 15:22

[aggConfig] unify field option handling

e49ba04

[tests] fix tests that were improperly using fields

a9c139a

[logstash_fields] remove sortable and filterable, which do nothing

0757c45

spalger force-pushed the implement/get-field-options branch from e554402 to 0757c45 Compare October 18, 2016 22:22

spalger commented Oct 18, 2016

View reviewed changes

spalger added 3 commits October 18, 2016 15:48

[notifier] ensure that notifications are not left around

2dbb462

[aggParams/field] support aggParamType#scriptable

95d704c

[aggConfig] add light validation for aggConfig field param

0e11c22

spalger changed the title ~~Implement aggConfig.getFieldOptions()~~ Unify getting aggregation field options Oct 18, 2016

Bargs suggested changes Oct 20, 2016

View reviewed changes

Bargs removed the review label Oct 20, 2016

spalger added the review label Oct 24, 2016

spalger changed the title ~~Unify getting aggregation field options~~ Unify getting fields for aggs, and filter scripted fields for significant terms agg Oct 24, 2016

spalger added 2 commits October 24, 2016 17:04

[aggConfig] restore use of @timestamp

e71b0f2

[tests] revert changes to notifier clearing

6157275

spalger mentioned this pull request Oct 25, 2016

[tests] ensure that the notifier is emptied by each test #8822

Merged

spalger added 2 commits October 24, 2016 17:20

[aggType/test] remove unnecessary change

fbf884a

[aggTypes/fieldParam] move orderby back into condition

0018786

Bargs approved these changes Oct 25, 2016

View reviewed changes

spalger assigned thomasneirynck Oct 25, 2016

thomasneirynck reviewed Oct 27, 2016

View reviewed changes

[aggParams/field] use a more descriptive warning

7205993

spalger merged commit 7e4d42d into elastic:master Oct 27, 2016

elastic-jasper mentioned this pull request Oct 27, 2016

[backport] PR #8734 to 5.x - Unify getting fields for aggs, and filter scripted fields for significant terms agg #8856

Merged

spalger added a commit that referenced this pull request Oct 27, 2016

Merge pull request #8856 from elastic/jasper/backport/8734/5.x

fa11c03

[backport] PR #8734 to 5.x - Unify getting fields for aggs, and filter scripted fields for significant terms agg

Bargs mentioned this pull request Oct 31, 2016

add top_hit metric #7302

Merged

3 tasks

ppisljar added the backported label Dec 2, 2016

epixa added v5.1.1 and removed v5.1.0 labels Dec 8, 2016

spalger deleted the implement/get-field-options branch October 18, 2019 17:40

		@@ -12,8 +12,11 @@ import IndexPatternsMapperProvider from 'ui/index_patterns/_mapper';
		import UtilsMappingSetupProvider from 'ui/utils/mapping_setup';

		@@ -33,10 +33,13 @@ describe('Notifier', function () {

		beforeEach(function () {

		@@ -10,12 +10,15 @@ describe('ui/route_based_notifier', function () {

		@@ -6,6 +6,7 @@ import chrome from 'ui/chrome';
		import Nonsense from 'Nonsense';

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

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

Conversation

spalger commented Oct 18, 2016 • edited Loading

Choose a reason for hiding this comment

Bargs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger Oct 22, 2016 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger Oct 22, 2016 • 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

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

Choose a reason for hiding this comment

spalger commented Oct 22, 2016

Bargs commented Oct 24, 2016

spalger commented Oct 25, 2016 • edited Loading

Bargs commented Oct 25, 2016

spalger commented Oct 25, 2016

Bargs commented Oct 25, 2016

thomasneirynck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger commented Oct 18, 2016 •

edited

Loading

spalger Oct 22, 2016 •

edited

Loading

spalger Oct 22, 2016 •

edited

Loading

spalger commented Oct 25, 2016 •

edited

Loading