[TSVB] Top_hit supports runtime fields #103401

DianaDerevyankina · 2021-06-25T13:51:45Z

Closes #101705

Summary

Replaced top_hits search request _source option with the fields option to retrieve the fields.
Fix unnecessary symbol escaping for table cells values.

Before:

After:

Checklist

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

For maintainers

This was checked for breaking API changes and was labeled appropriately

…ource

…and markdown

alexwizp · 2021-06-28T07:59:52Z

@elasticmachine merge upstream

alexwizp · 2021-06-28T11:36:01Z

src/plugins/vis_type_timeseries/public/application/components/aggs/top_hit.js

@@ -110,7 +110,7 @@ const TopHitAggUi = (props) => {
    PANEL_TYPES.METRIC,
    PANEL_TYPES.MARKDOWN,
  ].includes(panel.type)
-    ? [KBN_FIELD_TYPES.NUMBER, KBN_FIELD_TYPES.STRING]
+    ? [KBN_FIELD_TYPES.NUMBER, KBN_FIELD_TYPES.STRING, KBN_FIELD_TYPES.DATE]


You are right here.

For TimeSeries/Gauge we support only KBN_FIELD_TYPES.NUMBER field type.

For Table/Metric/Markdown - KBN_FIELD_TYPES.NUMBER, KBN_FIELD_TYPES.STRING, KBN_FIELD_TYPES.DATE

The main problem in your PR and code which was implemented before your changes that there are no correct Aggregation function for String and Date.

Currenly it's undefined but I prefer to do it like in classical visualizations with Concatenate option

Screen if I select Date field

alexwizp

Needs some changes

alexwizp · 2021-07-03T18:54:30Z

src/plugins/vis_type_timeseries/server/lib/vis_data/helpers/get_agg_value.js

@@ -45,7 +45,7 @@ export const getAggValue = (row, metric) => {
      }

      const hits = get(row, [metric.id, 'docs', 'hits', 'hits'], []);
-      const values = hits.map((doc) => get(doc, `_source.${metric.field}`));
+      const [values] = hits.map((doc) => doc.fields[metric.field]);


should be double checked. Why you take only first value?

alexwizp · 2021-07-05T14:13:11Z

src/plugins/vis_type_timeseries/public/application/components/aggs/top_hit.js

@@ -110,7 +110,7 @@ const TopHitAggUi = (props) => {
    PANEL_TYPES.METRIC,
    PANEL_TYPES.MARKDOWN,
  ].includes(panel.type)
-    ? [KBN_FIELD_TYPES.NUMBER, KBN_FIELD_TYPES.STRING]
+    ? [KBN_FIELD_TYPES.NUMBER, KBN_FIELD_TYPES.STRING, KBN_FIELD_TYPES.DATE]
    : [KBN_FIELD_TYPES.NUMBER];



@dziyanadzeraviankina @stratoula I see after switching from numbers to strings (and now to dates) our model keep wrong value (value which was selected to numbers). On UI we see Select... . This is issue should be fixed.

Please think about adding UI effect to set first option from available list

useEffect(() => { const defaultFn = aggWithOptions?.[0].value; const aggWith = model[AGG_WITH_KEY]; if (aggWith && defaultFn && aggWith !== defaultFn && !selectedAggWithOption) { handleChange({ AGG_WITH_KEY: defaultFn, }); } }, [model, selectedAggWithOption, aggWithOptions, handleChange]);

or fix it using different way, e.g. set agg_with to noop. I don't have strong opinion here

alexwizp · 2021-07-09T07:40:49Z

src/plugins/vis_type_timeseries/public/application/components/aggs/top_hit.js

-    order: 'desc',
-  };
-  const model = { ...defaults, ...props.model };
+  const defaults = useMemo(


in this case it looks like additional optimization. In fact, this will lead to a slight degradation in performance, since each time the React caching mechanism will be used to get the value.

If you want you can move it to one level up and create getDefaults() method or move it directly in line 108 with removing defaults at all

alexwizp · 2021-07-09T07:46:28Z

Please run https://kibana-ci.elastic.co/job/kibana+flaky-test-suite-runner/ to be sure that your new tests are not flucky

DianaDerevyankina · 2021-07-09T10:03:37Z

@alexwizp

Please run https://kibana-ci.elastic.co/job/kibana+flaky-test-suite-runner/ to be sure that your new tests are not flucky

Sure, no failures

VladLasitsa

LGTM, tested locally

elasticmachine · 2021-07-09T12:37:15Z

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

alexwizp

LGTM! Thank you

DianaDerevyankina · 2021-07-12T07:15:59Z

@elasticmachine merge upstream

kibanamachine · 2021-07-12T09:24:32Z

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id	before	after	diff
`visTypeTimeseries`	1001.6KB	1002.3KB	+639.0B

History

💚 Build #137517 succeeded 7fb795a
💚 Build #137152 succeeded 2ca4c62
💔 Build #137126 failed 1101eff
💚 Build #134480 succeeded d2d7069
💚 Build #134352 succeeded d22791d

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

cc @dziyanadzeraviankina

stratoula

Code LGTM,

I tested it with runtime fields and "normal" ones. It works fine. Thanx for all the tests added ❤️

* [TSVB] Refactor top-hit aggregation to work with fields instead of _source * Allow select date strings for top_hit aggregation in table, metric, and markdown * Fix agg_with handling for top_hit and add some tests * Refactor get_agg_value and fix type check for _tsvb_chart * Refactor top_hit.js Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* [TSVB] Refactor top-hit aggregation to work with fields instead of _source * Allow select date strings for top_hit aggregation in table, metric, and markdown * Fix agg_with handling for top_hit and add some tests * Refactor get_agg_value and fix type check for _tsvb_chart * Refactor top_hit.js Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

[TSVB] Refactor top-hit aggregation to work with fields instead of _s…

ebeaab4

…ource

DianaDerevyankina added Feature:TSVB TSVB (Time Series Visual Builder) Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.15.0 labels Jun 25, 2021

DianaDerevyankina requested a review from alexwizp June 25, 2021 13:51

DianaDerevyankina self-assigned this Jun 25, 2021

Allow select date strings for top_hit aggregation in table, metric, …

d22791d

…and markdown

Merge branch 'master' into issues/101705

d2d7069

alexwizp reviewed Jun 28, 2021

View reviewed changes

alexwizp requested changes Jun 28, 2021

View reviewed changes

alexwizp reviewed Jul 3, 2021

View reviewed changes

alexwizp reviewed Jul 5, 2021

View reviewed changes

DianaDerevyankina added 3 commits July 6, 2021 11:00

Merge branch 'master' into issues/101705

b875ab9

Fix agg_with handling for top_hit and add some tests

1101eff

Refactor get_agg_value and fix type check for _tsvb_chart

2ca4c62

alexwizp requested a review from VladLasitsa July 8, 2021 10:43

alexwizp reviewed Jul 9, 2021

View reviewed changes

VladLasitsa approved these changes Jul 9, 2021

View reviewed changes

DianaDerevyankina added 2 commits July 9, 2021 14:52

Merge branch 'master' into issues/101705

6adad74

Refactor top_hit.js

7fb795a

alexwizp requested a review from stratoula July 9, 2021 12:36

alexwizp marked this pull request as ready for review July 9, 2021 12:37

alexwizp requested a review from a team July 9, 2021 12:37

alexwizp approved these changes Jul 9, 2021

View reviewed changes

Merge branch 'master' into issues/101705

610fba9

stratoula added release_note:fix and removed release_note:skip Skip the PR/issue when compiling release notes labels Jul 12, 2021

stratoula requested review from wylieconlon and dej611 July 12, 2021 08:32

stratoula changed the title ~~[TSVB] Refactor top-hit aggregation to work with fields instead of _source~~ [TSVB] Top_hit supports runtime fields Jul 12, 2021

stratoula approved these changes Jul 12, 2021

View reviewed changes

DianaDerevyankina merged commit 87066e0 into elastic:master Jul 12, 2021

DianaDerevyankina mentioned this pull request Jul 12, 2021

[7.x] [TSVB] Top_hit supports runtime fields (#103401) #105226

Merged

DianaDerevyankina mentioned this pull request Jul 13, 2021

[TSVB] Add more functional tests #102606

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TSVB] Top_hit supports runtime fields #103401

[TSVB] Top_hit supports runtime fields #103401

DianaDerevyankina commented Jun 25, 2021 •

edited

Loading

alexwizp commented Jun 28, 2021

alexwizp Jun 28, 2021 •

edited

Loading

alexwizp left a comment

alexwizp Jul 3, 2021

alexwizp Jul 5, 2021

alexwizp Jul 9, 2021

alexwizp commented Jul 9, 2021

DianaDerevyankina commented Jul 9, 2021

VladLasitsa left a comment

elasticmachine commented Jul 9, 2021

alexwizp left a comment

DianaDerevyankina commented Jul 12, 2021

kibanamachine commented Jul 12, 2021

stratoula left a comment

[TSVB] Top_hit supports runtime fields #103401

[TSVB] Top_hit supports runtime fields #103401

Conversation

DianaDerevyankina commented Jun 25, 2021 • edited Loading

Summary

Checklist

For maintainers

alexwizp commented Jun 28, 2021

alexwizp Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

alexwizp left a comment

Choose a reason for hiding this comment

alexwizp Jul 3, 2021

Choose a reason for hiding this comment

alexwizp Jul 5, 2021

Choose a reason for hiding this comment

alexwizp Jul 9, 2021

Choose a reason for hiding this comment

alexwizp commented Jul 9, 2021

DianaDerevyankina commented Jul 9, 2021

VladLasitsa left a comment

Choose a reason for hiding this comment

elasticmachine commented Jul 9, 2021

alexwizp left a comment

Choose a reason for hiding this comment

DianaDerevyankina commented Jul 12, 2021

kibanamachine commented Jul 12, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

stratoula left a comment

Choose a reason for hiding this comment

DianaDerevyankina commented Jun 25, 2021 •

edited

Loading

alexwizp Jun 28, 2021 •

edited

Loading