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

[Lens] Don't use auto interval to get interval for single bar #64502

Merged
merged 3 commits into from
Apr 29, 2020

Conversation

mbondyra
Copy link
Contributor

@mbondyra mbondyra commented Apr 27, 2020

Summary

This is a proposal of solution of the following problem:
Charts are not able to calculate an interval for a chart with a single bar to display proper width of it. Right now we're passing minInterval from aggsConfig interval param, but it'll be removed with the PR: #63874

What I am proposing is to calculate the approximate interval with a simplified method of what we do for auto interval and as it's an edgecase, I don't think we need a big precision.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@mbondyra mbondyra changed the title Don't use auto interval to get interval for single bar [Lens] Don't use auto interval to get interval for single bar Apr 27, 2020
@mbondyra mbondyra requested review from flash1293 and wylieconlon and removed request for flash1293 April 27, 2020 09:20
@mbondyra mbondyra force-pushed the lens/single_bar_interval branch from 80c7745 to 93b1289 Compare April 27, 2020 09:23
@mbondyra mbondyra force-pushed the lens/single_bar_interval branch from 93b1289 to bb8cd9a Compare April 27, 2020 11:46
@mbondyra mbondyra added Feature:Lens Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.8.0 v8.0.0 labels Apr 27, 2020
@elasticmachine
Copy link
Contributor

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

@mbondyra mbondyra added the release_note:skip Skip the PR/issue when compiling release notes label Apr 27, 2020
@mbondyra mbondyra marked this pull request as ready for review April 27, 2020 12:49
@mbondyra mbondyra requested a review from a team April 27, 2020 12:49
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.

I just tested a little and this fix doesn't work anymore when the user is selecting a custom interval.

For example, if the custom interval is larger than the configured time range, no bar is shown as well:
Screenshot 2020-04-27 at 16 24 04

Vislib vis types can deal with the situation
Screenshot 2020-04-27 at 16 29 15

I guess this happens because in this case the bucket starts before the min value of the x domain, and because the minInterval calculation doesn't come close to the actual value (because it's simulating the auto behavior) it renders the bar off-screen to the left.

I suggest we solve this now by doing a hybrid of the old behavior and the approach in this PR - if the meta object contains an actual interval, use it. Otherwise fall back to the logic with guessing the interval. I'm getting more and more convinced that in the long-term the data table should contain this information.

@mbondyra
Copy link
Contributor Author

mbondyra commented Apr 27, 2020

@flash1293 Thank you for cautiousness! I tested it on 7.6.1 and looks like we have the bug there too 😭 Nevertheless, I agree, this probably makes things even worse so I'll add a correction. I submitted an issue: #64544

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.

In this case I’m going to approve, we can solve the other bug separately. Maybe it even requires an elastic-charts fix, let’s see.

@wylieconlon
Copy link
Contributor

I agree with @flash1293 about this entire comment:

  • if the meta object contains an actual interval, use it. Otherwise fall back to the logic with guessing the interval. I'm getting more and more convinced that in the long-term the data table should contain this information.

I also agree @mbondyra that the bug with intervals larger than the time range should be dealt with separately.

@mbondyra
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@mbondyra mbondyra merged commit da4c33b into elastic:master Apr 29, 2020
mbondyra added a commit that referenced this pull request Apr 29, 2020
#64784)

* feat: don't use auto interval to get interval for single bar

* fix: fixing non auto intervals

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@mbondyra mbondyra deleted the lens/single_bar_interval branch April 30, 2020 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants