[Transform] Support for the `top_metrics` aggregation #101152

darnautov · 2021-06-02T12:19:38Z

Summary

Resolves #98127

Adds UI support for the top_metrics aggregation.

Related Elasticsearch PR.

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

…op-metrics

elasticmachine · 2021-06-02T12:19:41Z

Pinging @elastic/ml-ui (:ml)

…op-metrics

qn895 · 2021-06-02T13:56:28Z

x-pack/plugins/transform/public/app/common/pivot_aggs.test.ts

+
+describe('isSpecialSortField', () => {
+  test('detects special sort field', () => {
+    expect(isSpecialSortField('_score')).toBe(true);


nit: would be good to add one or more test cases here to assert they will return false

Done in a2ae489

qn895 · 2021-06-02T14:02:19Z

..._transform/components/step_define/common/top_metrics_agg/components/top_metrics_agg_form.tsx

+
+  const sortDirectionOptions = Object.values(SORT_DIRECTION).map((v) => ({
+    id: v,
+    label: v,


Should the labels in these options be internationalized? 🤔

I don't recall seeing asc and desc to be translated in Kibana 🤔

qn895 · 2021-06-02T14:05:23Z

..._transform/components/step_define/common/top_metrics_agg/components/top_metrics_agg_form.tsx

+                >
+                  <EuiSelect
+                    options={NUMERIC_TYPES_OPTIONS[sortFieldType as KbnNumericType].map((v) => ({
+                      text: v,


Like the options above, should these be internationalized?

I believe we should not translate type names

qn895 · 2021-06-02T14:13:13Z

...public/app/sections/create_transform/components/step_define/common/top_metrics_agg/config.ts

+        sortDirection = sortDefinition;
+      }
+
+      if (typeof sortDefinition === 'object') {


nit: I think we can use isPopulatedObject here

Updated in 24b7c6f

qn895 · 2021-06-02T14:31:06Z

Gave this a test and functionally it works great. One suggestion is maybe we can either indicate that these sorting fields are required or disable the apply button if these options are not picked?

For example, if I pick the following empty first option and hit Apply I will get an error that's not so easy to understand

Likewise for the sort direction and sort mode

Another small consideration that maybe the design team can look at is the pop-up for the settings is getting longer and can sometimes get cut off from the screen. Not a big issue just something we should investigate since we are adding more and more options to the transform configurations in the future.

szabosteve

Left one small remark. UI text LGTM, thanks!

..._transform/components/step_define/common/top_metrics_agg/components/top_metrics_agg_form.tsx

…op-metrics

darnautov · 2021-06-04T08:15:36Z

thanks for testing it @qn895! 🙏 I improved the validation so it shouldn't allow applying invalid changes.
Regarding the popover redesign, we decided to keep it as is, because for an advanced configuration with loads of settings users tend to use a JSON editor. In the future, we may consider using a flyout instead, but it should be consistent for all aggregation types.

jgowdyelastic

Added a comment, but generally code LGTM

jgowdyelastic · 2021-06-04T09:42:04Z

..._transform/components/step_define/common/top_metrics_agg/components/top_metrics_agg_form.tsx

+    label: v,
+  }));
+
+  const sortFieldType = fields.find((f) => f.name === aggConfig.sortField)?.type;


should sortFieldOptions, sortDirectionOptions, sortModeOptions and sortFieldType all be wrapped in some useMemo?

Sorry, merged the PR from my phone and missed this comment. Definitely no harm in useMemo, but I don't expect any performance issues with the current code even with thousands of fields available. Also, this component is only rendered with the popover form is visible.

kibanamachine · 2021-06-04T09:59:10Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`transform`	225	227	+2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`transform`	918.5KB	928.0KB	+9.5KB

History

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

cc @darnautov

* [ML] init top_metrics agg * [ML] support sort * [ML] support _score sorting * [ML] support sort mode * [ML] support numeric type sorting * [ML] update field label, hide additional sorting controls * [ML] preserve advanced config * [ML] update agg fields after runtime fields edit * [ML] fix TS issue with EuiButtonGroup * [ML] fix Field label * [ML] refactor setUiConfig * [ML] update unit tests * [ML] wrap advanced sorting settings with accordion * [ML] config validation with tests * [ML] fix preserving of the unsupported config * [ML] update translation message * [ML] fix level of the custom config * [ML] preserve unsupported config for sorting

kibanamachine · 2021-06-04T14:07:47Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

) * [ML] init top_metrics agg * [ML] support sort * [ML] support _score sorting * [ML] support sort mode * [ML] support numeric type sorting * [ML] update field label, hide additional sorting controls * [ML] preserve advanced config * [ML] update agg fields after runtime fields edit * [ML] fix TS issue with EuiButtonGroup * [ML] fix Field label * [ML] refactor setUiConfig * [ML] update unit tests * [ML] wrap advanced sorting settings with accordion * [ML] config validation with tests * [ML] fix preserving of the unsupported config * [ML] update translation message * [ML] fix level of the custom config * [ML] preserve unsupported config for sorting Co-authored-by: Dima Arnautov <[email protected]>

darnautov added 8 commits May 27, 2021 10:27

[ML] init top_metrics agg

aa4cbdb

Merge remote-tracking branch 'upstream/master' into transform-98127-t…

4afee51

…op-metrics

[ML] support sort

74ab4c6

[ML] support _score sorting

7b14304

[ML] support sort mode

abcfc4e

[ML] support numeric type sorting

3b9746e

[ML] update field label, hide additional sorting controls

1a4732a

[ML] preserve advanced config

f210486

darnautov added release_note:enhancement :ml v8.0.0 Feature:Transforms ML transforms v7.14.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Jun 2, 2021

darnautov requested review from walterra and qn895 June 2, 2021 12:19

darnautov self-assigned this Jun 2, 2021

darnautov requested a review from a team as a code owner June 2, 2021 12:19

darnautov added 4 commits June 2, 2021 15:00

Merge remote-tracking branch 'upstream/master' into transform-98127-t…

0c60c10

…op-metrics

[ML] update agg fields after runtime fields edit

6187b06

[ML] fix TS issue with EuiButtonGroup

9261d65

[ML] fix Field label

6c274c4

elastic deleted a comment from kibanamachine Jun 2, 2021

qn895 reviewed Jun 2, 2021

View reviewed changes

darnautov requested a review from szabosteve June 3, 2021 12:08

darnautov added 2 commits June 3, 2021 14:42

[ML] refactor setUiConfig

24b7c6f

[ML] update unit tests

5b86c79

szabosteve approved these changes Jun 3, 2021

View reviewed changes

..._transform/components/step_define/common/top_metrics_agg/components/top_metrics_agg_form.tsx Outdated Show resolved Hide resolved

darnautov requested a review from jgowdyelastic June 3, 2021 13:34

darnautov added 7 commits June 3, 2021 17:02

[ML] wrap advanced sorting settings with accordion

d5c2bd3

[ML] config validation with tests

a2ae489

[ML] fix preserving of the unsupported config

0b5f57d

[ML] update translation message

a928ca4

[ML] fix level of the custom config

fb06878

[ML] preserve unsupported config for sorting

96e4214

Merge remote-tracking branch 'upstream/master' into transform-98127-t…

35322ab

…op-metrics

darnautov requested review from qn895 and alvarezmelissa87 and removed request for walterra June 4, 2021 08:15

jgowdyelastic approved these changes Jun 4, 2021

View reviewed changes

darnautov merged commit 9810a72 into elastic:master Jun 4, 2021

darnautov deleted the transform-98127-top-metrics branch June 4, 2021 14:05

kibanamachine mentioned this pull request Jun 4, 2021

[7.x] [Transform] Support for the top_metrics aggregation (#101152) #101390

Merged

lcawl added release_note:feature Makes this part of the condensed release notes and removed release_note:enhancement labels Jul 6, 2021

walterra mentioned this pull request Jan 20, 2022

[ML] Transforms: Config UI Support Meta Issue #123459

Open

29 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Transform] Support for the `top_metrics` aggregation #101152

[Transform] Support for the `top_metrics` aggregation #101152

darnautov commented Jun 2, 2021 •

edited

Loading

elasticmachine commented Jun 2, 2021

qn895 Jun 2, 2021 •

edited

Loading

darnautov Jun 4, 2021

qn895 Jun 2, 2021

darnautov Jun 3, 2021

qn895 Jun 2, 2021

darnautov Jun 3, 2021

qn895 Jun 2, 2021

darnautov Jun 3, 2021

qn895 commented Jun 2, 2021

szabosteve left a comment

darnautov commented Jun 4, 2021

jgowdyelastic left a comment

jgowdyelastic Jun 4, 2021

darnautov Jun 4, 2021

kibanamachine commented Jun 4, 2021

kibanamachine commented Jun 4, 2021

[Transform] Support for the top_metrics aggregation #101152

[Transform] Support for the top_metrics aggregation #101152

Conversation

darnautov commented Jun 2, 2021 • edited Loading

Summary

Checklist

elasticmachine commented Jun 2, 2021

qn895 Jun 2, 2021 • 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

qn895 commented Jun 2, 2021

szabosteve left a comment

Choose a reason for hiding this comment

darnautov commented Jun 4, 2021

jgowdyelastic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Jun 4, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

kibanamachine commented Jun 4, 2021

💚 Backport successful

[Transform] Support for the `top_metrics` aggregation #101152

[Transform] Support for the `top_metrics` aggregation #101152

darnautov commented Jun 2, 2021 •

edited

Loading

qn895 Jun 2, 2021 •

edited

Loading