Skip to content

[Lens] include empty rows setting for date histogram#127453

Merged
flash1293 merged 17 commits intoelastic:mainfrom
flash1293:lens-date-histogram-defaults
Mar 22, 2022
Merged

[Lens] include empty rows setting for date histogram#127453
flash1293 merged 17 commits intoelastic:mainfrom
flash1293:lens-date-histogram-defaults

Conversation

@flash1293
Copy link
Copy Markdown
Contributor

@flash1293 flash1293 commented Mar 10, 2022 •
edited
Loading

Fixes #119474

Adding "Show empty rows" switch to date histograms which is set to true by default but can be turned off (existing date histograms are migrated). This gives the user control whether date buckets without any backing data should be filled in or not.

Screenshot 2022-03-10 at 13 58 12

Screenshot 2022-03-10 at 13 58 17

Screenshot 2022-03-10 at 13 58 49

Screenshot 2022-03-10 at 13 58 41

Agg configs

Added a new param extendToTimeRange which takes the current time range and applies it as extended bounds if set.

@flash1293 flash1293 added release_note:enhancement Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// Team:AppServicesSv Feature:Lens backport:skip This PR does not require backporting v8.2.0 labels Mar 10, 2022
@flash1293 flash1293 mentioned this pull request Mar 15, 2022
Merged
@flash1293 flash1293 marked this pull request as ready for review March 17, 2022 08:53
@flash1293 flash1293 requested review from a team as code owners March 17, 2022 08:53
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Mar 17, 2022

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

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Mar 17, 2022

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

@flash1293
Copy link
Copy Markdown
Contributor Author

flash1293 commented Mar 17, 2022

@elasticmachine merge upstream

@kertal
Copy link
Copy Markdown
Member

kertal commented Mar 17, 2022

@elasticmachine merge upstream

kertal
kertal approved these changes Mar 17, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

DataDiscovery.team code LGTM, just tests were changed, did a quick cloud test, looks like before

@stratoula
Copy link
Copy Markdown
Contributor

stratoula commented Mar 18, 2022

@elasticmachine merge upstream

stratoula
stratoula reviewed Mar 18, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@stratoula stratoula 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.

This looks great! - The new behavior makes perfect sense to me. Even on that case
image
I have seen users who don't like the no results page and would like something like that instead. Also heatmap looks much better now.

Just one comment from my side:

  • I have this case, (breakdown by intervals and the empty row on). The missing values setting doesn't do anything on this case.

image

I am not sure if this is a good idea but maybe we could improve the experience here? This tooltip doesn't make sense to me now

image

  • Make we could improve the text somehow? I am not sure if the include empty rows makes sense to all our users. Maybe a tooltip to explain it further?

@flash1293
Copy link
Copy Markdown
Contributor Author

flash1293 commented Mar 18, 2022

I have this case, (breakdown by intervals and the empty row on). The missing values setting doesn't do anything on this case.

It doesn't do anything because there are no "missing values" - count will always give you a zero which is not "missing" in the sense of xy missing value handling. This is what #127731 will take care of.

@flash1293
Copy link
Copy Markdown
Contributor Author

flash1293 commented Mar 18, 2022

I have seen users who don't like the no results page and would like something like that instead. Also heatmap looks much better now.

Huh, good point, I kind of forgot this would happen. @ghudgins are we OK with this change of behavior? Instead of a "no results found" chart icon we will get an empty chart for time series. It still seems sensible (and we planned to go into this direction anyway), but important to point out.

@stratoula
Copy link
Copy Markdown
Contributor

stratoula commented Mar 18, 2022

I have this case, (breakdown by intervals and the empty row on). The missing values setting doesn't do anything on this case.

It doesn't do anything because there are no "missing values" - count will always give you a zero which is not "missing" in the sense of xy missing value handling. This is what #127731 will take care of.

Oh nice, I missed the other PR.

@stratoula stratoula closed this Mar 18, 2022
@stratoula stratoula reopened this Mar 18, 2022
stratoula
stratoula approved these changes Mar 18, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@stratoula stratoula 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.

Code LGTM, as I said above the new behavior makes perfect sense to me. Even the no results page, as a user I prefer it :)
I think we could improve a bit the text on the switch (or add a tooltip to explain the behavior) but I don't want to block the PR for it. I leave it to you Joe.

@stratoula
Copy link
Copy Markdown
Contributor

stratoula commented Mar 18, 2022

@elasticmachine merge upstream

@flash1293
Copy link
Copy Markdown
Contributor Author

flash1293 commented Mar 21, 2022

@elasticmachine merge upstream

ppisljar
ppisljar approved these changes Mar 21, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

code LGTM

@mbondyra
Copy link
Copy Markdown
Contributor

mbondyra commented Mar 21, 2022

I've noticed some weird behavior, is this a bug or am I not understanding something?

Mar-21-2022.15-43-58.mp4

@flash1293
Copy link
Copy Markdown
Contributor Author

flash1293 commented Mar 21, 2022

Great catch @mbondyra - the issue is the way "drop partials" works, I'm going to fix it on this PR

@flash1293
Copy link
Copy Markdown
Contributor Author

flash1293 commented Mar 21, 2022

@mbondyra Should be fixed - I extended the "drop partials" logic to try to look up the used interval from the params before falling back to check the distanced between bucket starts.

@kibana-ci
Copy link
Copy Markdown

kibana-ci commented Mar 21, 2022

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
data 2833 2834 +1

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 +586.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 457.3KB 458.0KB +656.0B
Unknown metric groups

API count

id before after diff
data 3446 3447 +1

History

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

mbondyra
mbondyra approved these changes Mar 22, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

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

Rechecked and works well now 👌🏼

@flash1293 flash1293 merged commit 354cd01 into elastic:main Mar 22, 2022
@ghudgins
Copy link
Copy Markdown
Contributor

ghudgins commented Mar 26, 2022

[retroactive agreement]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kertal kertal kertal approved these changes

@mbondyra mbondyra mbondyra approved these changes

@ppisljar ppisljar ppisljar approved these changes

@stratoula stratoula stratoula approved these changes

Assignees

No one assigned

Labels

backport:skip This PR does not require backporting Feature:Lens release_note:enhancement Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v8.2.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Lens] Offer option to extend auto-extend histogram and date histogram operations

9 participants

@flash1293 @elasticmachine @kertal @stratoula @mbondyra @kibana-ci @ghudgins @ppisljar @kibanamachine