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] Allow string fields on value count aggregation #79267

Merged
merged 4 commits into from
Oct 6, 2020

Conversation

stratoula
Copy link
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
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
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 Visualization editors, elastic-charts and infrastructure 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
Contributor

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

@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

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.

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

@@ -25,6 +25,7 @@ export function getSupportedFieldsByMetricType(type) {
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
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
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
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
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 Visualization editors, elastic-charts and infrastructure 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