Skip to content

[TSVB] Allow string fields on value count aggregation#79267

Merged
stratoula merged 4 commits intoelastic:masterfrom
stratoula:tsvb-allow-strings-value-count
Oct 6, 2020
Merged

[TSVB] Allow string fields on value count aggregation#79267
stratoula merged 4 commits intoelastic:masterfrom
stratoula:tsvb-allow-strings-value-count

Conversation

@stratoula
Copy link
Copy Markdown
Contributor

@stratoula stratoula commented Oct 2, 2020

Summary

Fixes #67581
Seems that value count aggregation supports non-numeric fields but currently TSVB restricts it to only histogram and number type fields. So in this PR I also allow string fields for this aggregation.

Updated
Allow all field types for this aggregation.

Checklist

@stratoula stratoula changed the title Tsvb allow strings value count [TSVB] Allow string fields on value count aggregation Oct 2, 2020
@stratoula stratoula force-pushed the tsvb-allow-strings-value-count branch from dc01edc to 766cf62 Compare October 2, 2020 13:28
@stratoula stratoula requested a review from alexwizp October 2, 2020 14:22
@stratoula
Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

Copy link
Copy Markdown
Contributor

@alexwizp alexwizp left a comment

Choose a reason for hiding this comment

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

LGTM! Tested locally

@stratoula stratoula added release_note:fix Feature:TSVB TSVB (Time Series Visual Builder) v7.10.0 v8.0.0 Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// and removed release_note:fix labels Oct 5, 2020
@stratoula stratoula marked this pull request as ready for review October 5, 2020 10:20
@stratoula stratoula requested a review from a team October 5, 2020 10:20
@elasticmachine
Copy link
Copy Markdown
Contributor

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

@stratoula
Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

Copy link
Copy Markdown
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.

Tested and works fine, however I think we can allow all fields for this one. I'm fine with merging as is, but it seems like a quick win to allow this for boolean etc. as well

case METRIC_TYPES.CARDINALITY:
return Object.values(KBN_FIELD_TYPES).filter((t) => t !== KBN_FIELD_TYPES.HISTOGRAM);
case METRIC_TYPES.VALUE_COUNT:
return [KBN_FIELD_TYPES.NUMBER, KBN_FIELD_TYPES.HISTOGRAM, KBN_FIELD_TYPES.STRING];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems like value count works for all field types - I changed this to return KBN_FIELD_TYPES and got nice looking charts as well:
Screenshot 2020-10-05 at 15 28 11

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No if it supports them all then I will change it to support them all 🙂 Thanx for that

@kibanamachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
visTypeTimeseries 1.8MB 1.8MB +49.0B

History

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

Copy link
Copy Markdown
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.

LGTM, did not test again

@stratoula stratoula merged commit 40ef720 into elastic:master Oct 6, 2020
stratoula added a commit to stratoula/kibana that referenced this pull request Oct 6, 2020
* [TSVB] Enable string fields for value count aggregation

* fix test title

* Allow all field types for value count aggregation

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
stratoula added a commit that referenced this pull request Oct 6, 2020
* [TSVB] Enable string fields for value count aggregation

* fix test title

* Allow all field types for value count aggregation

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 6, 2020
* master: (85 commits)
  Refactor attribute service (elastic#78414)
  [APM] Add default message to alerts. (elastic#78930)
  [Discover] Modify columns and sort when switching index pattern (elastic#74336)
  Document ts project references setup (elastic#78586)
  build all ts refs in single kbn:bootstrap (elastic#79438)
  [TSVB] Allow string fields on value count aggregation (elastic#79267)
  [SECURITY SOLUTION] Investigate EQL signal in timeline (elastic#79049)
  [Fleet] Add loading spinner to page headers (elastic#79568)
  [Security Solution][Resolver] Resolver query panel load more (elastic#79160)
  Add type row to monitor detail page. (elastic#79556)
  Remove license refresh from setup (elastic#79518)
  [docker] add reporting fonts (elastic#74806)
  [SECURITY_SOLUTION][ENDPOINT] Add info about trusted apps to the page subtitle + create flyout (elastic#79558)
  Trim Hash value before validating it (elastic#79545)
  Warn users when security is not configured (elastic#78545)
  update copy styling (elastic#79313)
  Update dependency @elastic/charts to v23.1.1 (elastic#78459)
  Introduce geo-threshold alerts (elastic#76285)
  elastic#76920 Show base breadcrumb when there is an error booting the app (elastic#79571)
  remove react-intl from kibana and keep it inside only i18n package (elastic#78956)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:TSVB TSVB (Time Series Visual Builder) release_note:fix Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v7.10.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[TSVB] Inability to use value_count aggregation on non-numeric fields

5 participants