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.

Sign up for GitHub

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

Jump to bottom

[Lens] Support histogram mapping type for all numeric functions #90357

Merged
merged 12 commits into from
Feb 16, 2021
Merged

[Lens] Support histogram mapping type for all numeric functions #90357

merged 12 commits into from
Feb 16, 2021

Conversation

wylieconlon
Copy link
Contributor

@wylieconlon wylieconlon commented Feb 4, 2021 •
edited
Loading

This is supported in field stats too:

Screen Shot 2021-02-05 at 10 57 07 AM

Closes #74581

Checklist

@wylieconlon wylieconlon changed the title [Lens] Support histogram mapping type [Lens] Support histogram mapping type for all numeric functions Feb 5, 2021
@wylieconlon wylieconlon added Feature:Lens release_note:feature Makes this part of the condensed release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.12.0 v8.0.0 labels Feb 5, 2021
@wylieconlon wylieconlon marked this pull request as ready for review February 5, 2021 16:00
@wylieconlon wylieconlon requested a review from a team February 5, 2021 16:00
@wylieconlon wylieconlon requested review from a team as code owners February 5, 2021 16:00
@elasticmachine
Copy link
Contributor

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

wylieconlon
wylieconlon commented Feb 5, 2021
View reviewed changes
src/plugins/home/server/services/sample_data/data_sets/logs/field_mappings.ts Outdated
@@ -102,6 +102,9 @@ export const fieldMappings = {
bytes: {
type: 'long',
},
bytes_histogram: {
type: 'histogram',
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reviewers: You can test the histogram data in the sample logs, but I think this is probably not a good idea to merge. So I'll plan to remove these changes before merging.

@flash1293
Copy link
Contributor

@elasticmachine merge upstream

@flash1293
Copy link
Contributor

@elasticmachine merge upstream

flash1293
flash1293 approved these changes Feb 10, 2021
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I would be fine with solving this separately, but as there's no default formatter on the index pattern for histogram fields, we fall back to Number.toString which is not the best choice:
Screenshot 2021-02-10 at 14 08 37
Screenshot 2021-02-10 at 14 08 56

Three options I see here:

  • Set default number formatter for histogram fields on the index pattern (this would solve the issue for Visualize charts as well)
  • Have a special logic for histogram fields in Lens
  • Just ignore for now

Approving for the general feature, if we are going to merge without addressing the formatting, let's open a separate issue

x-pack/plugins/lens/server/routes/field_stats.ts Outdated
@@ -205,6 +209,81 @@ export async function getNumberHistogram(
};
}

export async function getNumberOnlyHistogram(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: It looks like this code is almost the same as getNumberHistogram (except for the top values part). Can we reduce the redundancy somehow in a nice way (maybe some conditional branches)? Not a blocker, just stumbled over it a little.

@wylieconlon
Copy link
Contributor Author

@flash1293 The number formatting for histograms is a little complicated because we need to apply different formatting for the raw value vs aggregated value. So in Discover, we use a string formatter, but in Lens we use a number formatter. My preference is to change the formatter at the aggConfigs level somehow.

I was having a hard time with the getNumberHistogram deduplication. I'll take another look now.

@flash1293
Copy link
Contributor

@elasticmachine merge upstream

flash1293
flash1293 approved these changes Feb 15, 2021
View reviewed changes
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.

Thanks for the refactoring, still works AFAICT.

I'm going to create a separate issue for the formatting problem

@kibanamachine
Copy link
Contributor

💚 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
lens 892.3KB 892.7KB +387.0B

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.7KB 798.7KB +72.0B

History

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

@wylieconlon
Copy link
Contributor Author

@flash1293 I already opened an issue for app-services here: #91163 but I plan on spending a little time to define the solution more.

mattkime
mattkime approved these changes Feb 16, 2021
View reviewed changes
Copy link
Contributor

@mattkime mattkime left a comment

Choose a reason for hiding this comment

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

Works well, nice work!

@wylieconlon wylieconlon merged commit 16d86b0 into elastic:master Feb 16, 2021
@wylieconlon wylieconlon deleted the lens/histogram-types branch February 16, 2021 16:51
wylieconlon pushed a commit to wylieconlon/kibana that referenced this pull request Feb 16, 2021
@kibanamachine @wylieconlon
[Lens] Support histogram mapping type for all numeric functions (elas…
d23f1d4 
…tic#90357)

* [Lens] Support histogram mapping type

* Fix field stats and allow max/min

* Fix types

* Revert to regular sample data

* Simplify server code

* Add test for edge case

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@wylieconlon wylieconlon mentioned this pull request Feb 16, 2021
Merged
wylieconlon pushed a commit that referenced this pull request Feb 16, 2021
@kibanamachine
[Lens] Support histogram mapping type for all numeric functions (#90357…
1fb387e 
…) (#91521)

* [Lens] Support histogram mapping type

* Fix field stats and allow max/min

* Fix types

* Revert to regular sample data

* Simplify server code

* Add test for edge case

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

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattkime mattkime mattkime approved these changes

@flash1293 flash1293 flash1293 approved these changes

@pgayvallet pgayvallet pgayvallet approved these changes

@dej611 dej611 Awaiting requested review from dej611

@mbondyra mbondyra Awaiting requested review from mbondyra

@ppisljar ppisljar Awaiting requested review from ppisljar

Assignees
No one assigned
Labels
Feature:Lens release_note:feature Makes this part of the condensed release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.12.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Lens] Support for Histogram data type with sum, average
6 participants
@wylieconlon @elasticmachine @flash1293 @kibanamachine @mattkime @pgayvallet