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] Fix wrongly display stacked as percentage charts #62654

Merged

Conversation

markov00
Copy link
Member

@markov00 markov00 commented Apr 6, 2020

Summary

This PR fix an issue with stack as percentage charts in TSVB by upgrading the @elastic/charts library to 18.2.2 the patch version that solves this issue: elastic/elastic-charts#617

Fix #62419

In this fix, when rendering a stacked area chart in percentage mode, if we have all values as 0 in a bucket, the chart will render a 0% data point for all the series.
Only in case where the values are nulls it will completely disconnect the line/area points where we found the not defined value.
Screenshot 2020-04-09 at 16 36 08

@elastic/charts upgrade procedure

@markov00 markov00 added bug Fixes for quality problems that affect the customer experience Feature:TSVB TSVB (Time Series Visual Builder) regression Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 v7.7.0 labels Apr 6, 2020
@markov00 markov00 requested a review from a team April 6, 2020 17:15
@markov00 markov00 requested a review from a team as a code owner April 6, 2020 17:15
@elasticmachine
Copy link
Contributor

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

@timroes timroes added the v7.8.0 label Apr 7, 2020
@markov00 markov00 added the release_note:skip Skip the PR/issue when compiling release notes label Apr 7, 2020
@flash1293
Copy link
Contributor

ACK, reviewing now.

Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Tested on Chrome Linux with given sample data. Fixes the issue. Code LGTM

@flash1293
Copy link
Contributor

The problem seems fixed, but there is a difference in how gaps are treated in TSVB (compared to 7.6.1). Not sure whether that was intended

7.6.1:
Screenshot 2020-04-07 at 11 07 40

This PR:
Screenshot 2020-04-07 at 11 07 46

@timroes
Copy link
Contributor

timroes commented Apr 7, 2020

I think the new behavior looks more appropriate to me, and also should match what we're doing in visualize editor in percentage mode. If we want to stick more closely to the previous behavior I think we could use Fit.LINEAR instead on the chart? That would at least highlight that those parts are not real values actually but just interpolated.

@flash1293
Copy link
Contributor

Good suggestion @timroes - changing the interpolation mode for all TSVB visualizations seems like a confusing change for people used to the old behavior. Just a nit though, IMHO we can also address this later (and give the user control over interpolation while we are at it).

@markov00
Copy link
Member Author

markov00 commented Apr 7, 2020

@flash1293 @timroes I will push another change in the elastic-chart to fix this behavior.
I've checked in version 7.4 and, where we have 3 series stacked in percentage mode with all 3 values at 0, the resulting chart brings down to 0% the total percentage. In 7.6 the chart wrongly represents the data because we are tracing a line that interpolates between the previous and next point, misrepresenting the 0 value.
I will fix that in ECH to consider that a 0% bucket and push another commit here

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

Shared deps upgrade LGTM

@markov00 markov00 merged commit 93b3463 into elastic:master Apr 9, 2020
@markov00 markov00 deleted the 2020_04_06-update-charts-to-18.1.2 branch April 9, 2020 21:53
markov00 added a commit to markov00/kibana that referenced this pull request Apr 9, 2020
Update to  elastic-charts 18.2.2 with the valid 0% rendering
markov00 added a commit to markov00/kibana that referenced this pull request Apr 9, 2020
Update to  elastic-charts 18.2.2 with the valid 0% rendering
markov00 added a commit that referenced this pull request Apr 10, 2020
)

Update to  elastic-charts 18.2.2 with the valid 0% rendering
markov00 added a commit that referenced this pull request Apr 10, 2020
)

Update to  elastic-charts 18.2.2 with the valid 0% rendering
@flash1293
Copy link
Contributor

Approving retroactively, tested and the new behavior looks good to me 👍 Sorry for the delay

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:TSVB TSVB (Time Series Visual Builder) regression release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.7.0 v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TSVB] Wrongly display stacked as percentage charts
6 participants