Skip to content

Fix agg datatable type#90574

Merged
flash1293 merged 4 commits intoelastic:masterfrom
flash1293:fix-agg-datatable-type
Feb 11, 2021
Merged

Fix agg datatable type#90574
flash1293 merged 4 commits intoelastic:masterfrom
flash1293:fix-agg-datatable-type

Conversation

@flash1293
Copy link
Contributor

The TabbedAggResponseWriter turning the response from Elasticsearch into a datatable is inferring the type of the datatable column using the type of the underlying field. While this is a good strategy for most aggregations, it doesn't work for cardinality - a string field will produce a number value as well for this aggregation.

This PR adds a way to specify the expected column type if it diverges from the field type.

Context of this is #89300 which is using this value to determine how to align text in a data table.

@flash1293 flash1293 added release_note:skip Skip the PR/issue when compiling release notes Team:AppServices v7.12.0 v8.0.0 labels Feb 8, 2021
@flash1293 flash1293 marked this pull request as ready for review February 8, 2021 10:38
@flash1293 flash1293 requested a review from a team as a code owner February 8, 2021 10:38
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServices)

Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

Did not run the change, but looks good.

},
source: 'esaggs',
sourceParams: {
appliedTimeRange: undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this is added because count is handled in a unique way, but most metrics have this?

@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@streamich streamich left a comment

Choose a reason for hiding this comment

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

Code changes LGTM.

/cc @ppisljar as he knows more about agg types.

@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
data 798.6KB 798.7KB +125.0B

History

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

@flash1293 flash1293 merged commit 16eb41b into elastic:master Feb 11, 2021
flash1293 added a commit to flash1293/kibana that referenced this pull request Feb 11, 2021
flash1293 added a commit that referenced this pull request Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants