Hides advanced json for count metric #74636

stratoula · 2020-08-10T08:53:53Z

Summary

Closes #74241
We want to hide the advanced json input when the user selects the Count aggregation as it can't affect this specific aggregation.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2020-08-11T09:36:03Z

Pinging @elastic/kibana-app (Team:KibanaApp)

stratoula · 2020-08-12T07:31:08Z

@elasticmachine merge upstream

stratoula · 2020-08-14T07:33:41Z

@elasticmachine merge upstream

stratoula · 2020-08-17T06:44:26Z

@elasticmachine merge upstream

stratoula · 2020-08-19T06:48:11Z

@elasticmachine merge upstream

alexwizp · 2020-08-19T10:41:19Z

src/plugins/vis_default_editor/public/components/agg_params_helper.ts

@@ -115,6 +115,7 @@ function getAggParamsToRender({
    }

    if (!paramEditor) {
+      if (agg.type.name === 'count' && param.type === 'json') return;


Your solutions was tested locally and it works fine. But could you please use METRIC_TYPES.COUNT instead of hardcoded value.

Also from the design perspective I don't like idea to add some if's into high level component. From my point of view all of that edge cases should be handled in src/plugins/vis_default_editor/public/components/agg_params_map.ts

But looks like agg_params_map also requires some refactoring to exclude some fields from that map =( but I prefer to go this way

@sulemanof what do you think?

In its simplest form, it looks like

agg_params_map.ts:

agg_params_helper.ts:

IMHO,
the restriction of an any param should come from an agg type.
The source of building any aggregation on UI is the agg.type.params, which is based on agg config.
In that case, we should restrict the count metric have the json param in its config.
I could suggest smth like that:

src/plugins/data/common/search/aggs/metrics/count.ts

and than in src/plugins/data/common/search/aggs/agg_type.ts

The same is happening now to customLabel param:

Probably yes, /plugins/data/common/search/aggs/metrics/count_fn.ts should be also updated

Than you both ❤️ Will look into this

lukeelmers

Overall the changes to the aggs service make sense. In the long run I'd like to completely remove the "magic" addition of the json param onto every agg type, and instead have each agg explicitly declare it so that you don't need something like a json: false setting. But that's pretty far outside the scope of this PR, so in the interim I'm comfortable with this approach.

lukeelmers · 2020-08-20T16:45:47Z

src/plugins/data/common/search/aggs/metrics/count_fn.ts

-          ...rest,
-          json: getParsedValue(args, 'json'),
-        },
+        params: rest,


We should remove the json argument from the function definition above as well, since it's no longer being used.

Note that this might cause a TS error because the function definition is using AggExpressionFunctionArgs which is pulling the relevant type from the AggParamsMapping. For count, the value used is BaseAggParams which includes json:

kibana/src/plugins/data/common/search/aggs/types.ts

Line 138 in bf0f6fa

[METRIC_TYPES.COUNT]: BaseAggParams;

If TS gives you trouble, you may need to define an AggParamsMetricCount in the count.ts file without extending BaseAggParams as others agg types do.

Thanx @lukeelmers , I missed that, TS looks happy btw :)

stratoula · 2020-08-21T07:15:59Z

@elasticmachine merge upstream

stratoula · 2020-08-21T08:02:25Z

@elasticmachine merge upstream

sulemanof

LGTM 💟

lukeelmers

App arch code updates LGTM!

stratoula · 2020-08-23T21:14:22Z

@elasticmachine merge upstream

stratoula · 2020-08-31T06:02:04Z

@elasticmachine merge upstream

kibanamachine · 2020-08-31T07:51:28Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 250e208

Build metrics

page load bundle size

id	value	diff	baseline
data	1.4MB	-1.3KB	1.4MB

History

💚 Build #69895 succeeded 31bfadc
💚 Build #69615 succeeded 73747e3
💔 Build #69602 failed 54ac39e
💔 Build #69598 failed d3a52a9
💚 Build #69306 succeeded bf0f6fa

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

kertal

Code LGTM, tested locally in Chrome, Firefox & Safari, works 👍

Just one request regarding the PR's summary, could you provide a bit more details? many thx!

* remove advanced json for count agg * Remove only advanced json from count agg * use Constant from data plugin * add the logic to data plugin * remove json arg from function definition * remove unecessary translations Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…s-for-710 * 'master' of github.com:elastic/kibana: (43 commits) [APM] Chart units don't update when toggling the chart legends (elastic#74931) [ILM] Add support for frozen phase in UI (elastic#75968) Hides advanced json for count metric (elastic#74636) add client-side feature usage API (elastic#75486) [Maps] add drilldown support map embeddable (elastic#75598) [Enterprise Search] Request handler refactors/enhancements + update existing routes to use shared handler (elastic#76106) [Resolver] model `location.search` in redux (elastic#76140) [APM] Prevent imports of public in server code (elastic#75979) fix eslint issue skip flaky suite (elastic#76223) [APM] Transaction duration anomaly alerting integration (elastic#75719) [Transforms] Avoid using "Are you sure" (elastic#75932) [Security Solution][Exceptions] - Fix bug of alerts not updating after closure from exceptions modal (elastic#76145) [plugin-helpers] improve 3rd party KP plugin support (elastic#75019) [docs/getting-started] link to yarn v1 specifically (elastic#76169) [Security_Solution][Resolver] Resolver loading and error state (elastic#75600) Fixes App Search documentation links (elastic#76133) Fix alerts unable to create / update when the name has trailing whitepace(s) (elastic#76079) [Resolver] Fix useSelector usage (elastic#76129) [Enterprise Search] Migrate util and components from ent-search (elastic#76051) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/data_tier_allocation/node_allocation.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/edit_policy.tsx # x-pack/plugins/index_lifecycle_management/public/application/services/policies/types.ts # x-pack/plugins/index_lifecycle_management/public/application/services/policies/warm_phase.ts

* remove advanced json for count agg * Remove only advanced json from count agg * use Constant from data plugin * add the logic to data plugin * remove json arg from function definition * remove unecessary translations Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

stratoula added 2 commits August 10, 2020 11:52

remove advanced json for count agg

6a32f32

Remove only advanced json from count agg

3bb8102

stratoula added Feature:Aggregations Aggregation infrastructure (AggConfig, esaggs, ...) Feature:Vis Editor Visualization editor issues labels Aug 10, 2020

stratoula changed the title ~~remove advanced json for count agg~~ Hides advanced json for count metric Aug 10, 2020

stratoula requested a review from sulemanof August 10, 2020 11:49

stratoula marked this pull request as ready for review August 10, 2020 12:39

stratoula requested a review from a team August 10, 2020 12:39

stratoula added release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Aug 10, 2020

Merge branch 'master' into count-agg-remove-advanced-json

e6e12f1

Merge branch 'master' into count-agg-remove-advanced-json

370d624

Merge branch 'master' into count-agg-remove-advanced-json

2e92418

stratoula requested a review from alexwizp August 18, 2020 07:55

Merge branch 'master' into count-agg-remove-advanced-json

dc495c3

alexwizp reviewed Aug 19, 2020

View reviewed changes

stratoula added 2 commits August 19, 2020 14:13

use Constant from data plugin

49c4bb3

add the logic to data plugin

bf0f6fa

stratoula requested a review from a team as a code owner August 20, 2020 08:33

lukeelmers self-requested a review August 20, 2020 16:38

lukeelmers requested changes Aug 20, 2020

View reviewed changes

remove json arg from function definition

d3a52a9

stratoula added 2 commits August 21, 2020 10:30

Merge branch 'master' into count-agg-remove-advanced-json

4424f0d

remove unecessary translations

54ac39e

Merge branch 'master' into count-agg-remove-advanced-json

73747e3

sulemanof approved these changes Aug 21, 2020

View reviewed changes

lukeelmers approved these changes Aug 22, 2020

View reviewed changes

Merge branch 'master' into count-agg-remove-advanced-json

31bfadc

Merge branch 'master' into count-agg-remove-advanced-json

250e208

alexwizp approved these changes Aug 31, 2020

View reviewed changes

kertal approved these changes Aug 31, 2020

View reviewed changes

stratoula merged commit 9ddd49a into elastic:master Aug 31, 2020

stratoula mentioned this pull request Aug 31, 2020

[7.x] Hides advanced json for count metric (#74636) #76266

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hides advanced json for count metric #74636

Hides advanced json for count metric #74636

stratoula commented Aug 10, 2020 •

edited

Loading

elasticmachine commented Aug 11, 2020

stratoula commented Aug 12, 2020

stratoula commented Aug 14, 2020

stratoula commented Aug 17, 2020

stratoula commented Aug 19, 2020

alexwizp Aug 19, 2020

alexwizp Aug 19, 2020

sulemanof Aug 19, 2020

alexwizp Aug 19, 2020

stratoula Aug 19, 2020

lukeelmers left a comment

lukeelmers Aug 20, 2020

stratoula Aug 21, 2020

stratoula commented Aug 21, 2020

stratoula commented Aug 21, 2020

sulemanof left a comment

lukeelmers left a comment

stratoula commented Aug 23, 2020

stratoula commented Aug 31, 2020

kibanamachine commented Aug 31, 2020

kertal left a comment •

edited

Loading

Hides advanced json for count metric #74636

Hides advanced json for count metric #74636

Conversation

stratoula commented Aug 10, 2020 • edited Loading

Summary

Checklist

elasticmachine commented Aug 11, 2020

stratoula commented Aug 12, 2020

stratoula commented Aug 14, 2020

stratoula commented Aug 17, 2020

stratoula commented Aug 19, 2020

alexwizp Aug 19, 2020

Choose a reason for hiding this comment

alexwizp Aug 19, 2020

Choose a reason for hiding this comment

sulemanof Aug 19, 2020

Choose a reason for hiding this comment

alexwizp Aug 19, 2020

Choose a reason for hiding this comment

stratoula Aug 19, 2020

Choose a reason for hiding this comment

lukeelmers left a comment

Choose a reason for hiding this comment

lukeelmers Aug 20, 2020

Choose a reason for hiding this comment

stratoula Aug 21, 2020

Choose a reason for hiding this comment

stratoula commented Aug 21, 2020

stratoula commented Aug 21, 2020

sulemanof left a comment

Choose a reason for hiding this comment

lukeelmers left a comment

Choose a reason for hiding this comment

stratoula commented Aug 23, 2020

stratoula commented Aug 31, 2020

kibanamachine commented Aug 31, 2020

💚 Build Succeeded

Build metrics

page load bundle size

History

kertal left a comment • edited Loading

Choose a reason for hiding this comment

stratoula commented Aug 10, 2020 •

edited

Loading

kertal left a comment •

edited

Loading