Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TSVB] Multi-field group by #126015

Merged
merged 26 commits into from
Mar 3, 2022
Merged

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Feb 18, 2022

Closes: #125063

Summary

The "group by" terms should allow multiple fields using multi field terms under the hood.

Screen

Screen.Recording.2022-02-28.at.12.31.19.PM.mov
Screen.Recording.2022-02-28.at.12.50.23.PM.mov

Todo list:

  • update the FieldSelect component to work with multi fields
  • update panel_model.ts. terms_field and pivot_id should support Array<string | null>
  • client: Activate new mode for Table -> Group By and Terms -> By
  • server: Add support for Table -> Group By
  • server: Add support for Terms -> By backend
  • update convert_series_to_datatable
  • update Navigate to Lens
  • update Tests

@alexwizp alexwizp force-pushed the multi_fields_support branch 3 times, most recently from fecf7f7 to 11c223f Compare February 24, 2022 12:12
@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@alexwizp alexwizp requested a review from a team as a code owner February 28, 2022 09:52
@alexwizp alexwizp self-assigned this Feb 28, 2022
@alexwizp alexwizp added Feature:TSVB TSVB (Time Series Visual Builder) release_note:feature Makes this part of the condensed release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Feb 28, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@@ -83,7 +83,7 @@ export class KibanaMetricsAdapter implements InfraMetricsAdapter {
series: panel.series.map((series) => {
return {
id: series.id,
label: series.label,
label: [series.label].flat()[0],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change intentional?
As far as I can understand this will have no effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is by design to avoid type errors on your end. When using splitting by one field, everything will continue to work as before

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, if you're making other changes it would be great if you could include that context in a code comment!

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alex thanx a lot for working on this, it loos great!

  • Can we also remove this question from our TSVB docs?

image

https://www.elastic.co/guide/en/kibana/current/tsvb.html (it suggests runtime fields but now we can support it natively so no need for this)

  • I navigate to Lens with two fields but the break down label is misleading

image

If you re-create it in Lens the label is "Top values of + 1 other". Can we align the behaviors? consider it as a nit though.

  • Creating filters with clicking on the timeseries chart doesn't work for multi fields terms

  • It is a bit confusing to understand what this plus button does. Can we add it a bit closer to the terms dropdown?

image

  • Can we respect the plural here? (2 others). Lens also does it

image

@alexwizp alexwizp added the backport:skip This commit does not require backporting label Mar 1, 2022
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On using "series aggs", the label seems to run through the multi terms formatting using the caret separator even though it got collapsed:
Screenshot 2022-03-01 at 12 37 47

It should just say "Series agg (sum)"

Otherwise this looks pretty good to me (also saw the points from Stratoulas list) 👍

@@ -24,7 +24,8 @@ export const dateHistogram: TableRequestProcessorsFunction =

const meta: TableSearchRequestMeta = {
timeField,
index: panel.use_kibana_indexes ? seriesIndex.indexPattern?.id : undefined,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: In src/plugins/vis_types/timeseries/server/lib/vis_data/request_processors/annotations/query.test.ts and src/plugins/vis_types/timeseries/server/lib/vis_data/request_processors/table/query.test.ts there are a few leftovers of the index property.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@flash1293 these indexes related to filter object. It's not related to that PR

image

@alexwizp
Copy link
Contributor Author

alexwizp commented Mar 2, 2022

@elasticmachine merge upstream

@alexwizp
Copy link
Contributor Author

alexwizp commented Mar 2, 2022

Creating filters with clicking on the timeseries chart doesn't work for multi fields terms

I see that feature is working well for limited cases. Reported issue for that here #126671 .
Not a blocker for that PR

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
visTypeTimeseries 298 302 +4

Async chunks

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

id before after diff
lens 1.1MB 1.1MB +159.0B
visTypeTimeseries 462.1KB 470.5KB +8.4KB
total +8.6KB

History

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

cc @alexwizp

@alexwizp alexwizp requested a review from stratoula March 2, 2022 14:39
Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works great Alex, thank you a lot! Let's fix all the filter events bugs on a separate PR. I am fine with that! LGTM!

@alexwizp alexwizp merged commit efcdbb6 into elastic:main Mar 3, 2022
lucasfcosta pushed a commit to lucasfcosta/kibana that referenced this pull request Mar 8, 2022
* fieldSelect

* activate multifield support for table

* update table>pivot request_processor

* fix some tests

* apply some changes

* fix JEST

* push initial logic for series request_processor

* fix some broken cases for Table tab

* update convert_series_to_datatable / convert_series_to_vars

* add some logic

* fix table/terms

* do some logic

* fix some issues

* push some logic

* navigation to Lens

* fix CI

* add excludedFieldFormatsIds param into excludedFieldFormatsIds

* fix ci

* fix translations

* fix some comments

* fix series_agg label

* update labels in lens

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
lucasfcosta pushed a commit to lucasfcosta/kibana that referenced this pull request Mar 8, 2022
* fieldSelect

* activate multifield support for table

* update table>pivot request_processor

* fix some tests

* apply some changes

* fix JEST

* push initial logic for series request_processor

* fix some broken cases for Table tab

* update convert_series_to_datatable / convert_series_to_vars

* add some logic

* fix table/terms

* do some logic

* fix some issues

* push some logic

* navigation to Lens

* fix CI

* add excludedFieldFormatsIds param into excludedFieldFormatsIds

* fix ci

* fix translations

* fix some comments

* fix series_agg label

* update labels in lens

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:TSVB TSVB (Time Series Visual Builder) release_note:feature Makes this part of the condensed release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TSVB] Multi-field group by
7 participants