-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Preserve x-axis ordering in split series vis. #27723
Conversation
Pinging @elastic/kibana-app |
💔 Build Failed |
💔 Build Failed |
384a7a8
to
37d145a
Compare
💚 Build Succeeded |
This resolves an issue where the original sort order sent back by ES was lost for point series / vislib visualizations with split series. This was due to the way the point series agg response handler generated series data, only filling in series values as it encountered them bucket-by-bucket, rather than first looking at all x-values and ordering them consistently within each series. With this change, when a series is first created in the `agg_response`, it will first look at all results, preserving the x-value sort order. Then when creating new series, it will instantiate a zero-filled array with the correctly ordered x axis values, filling it in with the real values as it encounters them. This duplicates some of the work done in the vislib `zero_injection` component, which can likely be cleaned up further, or possibly removed entirely.
4ed8394
to
9e613b3
Compare
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, tested it in chrome linux
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me block this PR so we can discuss this a bit:
Are we fixing only the first level of ordering (fixing only the order of the main columns on the x axis)?
Because the issue #17532 is not only related to the ordering of the bars on the x axis, but is related also to the ordering of the splitted series for each bar (as some linked issues report that.
You can easily see that if you use the following:
- first x axis by extensions terms
- split series by machine os.
Now try to change the ordering of the split series aggregation (ascending and descending) and check the tooltips values: seems that the series are ordered by series and they are not respecting the ordering coming from ES.
On the inspector table you can easily see the right results: but the visualization just insert points
based on series orders not on data order.
After further investigation with @markov00, we confirmed that this PR does indeed only solve part of the problem: While the x-axis will be ordered correctly, subbuckets will still be sorted based on the results of the first agg. TL;DR: I recommend we merge this PR as it still solves one use case, and open a new PR for the second. To reiterate Marco's point, take the following example using kibana_sample_data_logs. Here's the aggregation config:
Here is an excerpt of the ES response:
This PR will ensure the order is correct for the first bucket:
However, the subbuckets will all be ordered based on the first result only:
This issue is described in deeper detail in the comments on the original issue, and in some of the duplicate issues. I was focused on solving for the x-axes and missed the second use case. Solving for the subbucket ordering is more complex as it requires reworking our fundamental structure for passing around series data; currently the data is passed to vislib like this:
As you can see, there is no concept of ordering series items within each x-axis bucket, as they only exist once at the outer level. Solving for the ordering of subbuckets would require a few things:
Since this PR still solves one valid use case and is separate from the subbucket ordering issue, I recommend we merge this and open a new PR for the second use case. |
This comment has been minimized.
This comment has been minimized.
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This solution (probably) has another issue. We are zero filling all the series, which means we are actually changing the results. It will not have any effect on the bar chart, but area and line charts will look different. I don't think there is an easy way around it, as we have no way to differentiate between zero filled values and actual zero values.
A quick overview of where things go wrong:
- we correctly convert table (
tabify
) to preserve all the orders - we correctly create the series: series are ordered by the order they appeared in the response, and their values are ordered by the order they appeared in the response.
But when the chart tries to draw this, it would render one series a time, in the order they appeared and always render all values for each series. This is what produces wrong x-axis.
One way to fix above, would be to give information about the x axis, in the form of ordered array of x-axis values. Chart can then use that to make sure x-axis order is preserved.
However this still doesn't solve the issue @markov00 is mentioning, but i would argue that is not really an issue. Our charts were designed to behave that way. In most scenarios it makes sense:
- you do any chart with non stacked split series ... when the series are not stacked you might expect them to always show in the same order. For example charts like this:
order of series is always the same, no matter their value. it would be confusing if the red bar would be jumping left and right.
- you do stacked area or line. You will always want the order of series to stay the same, no matter the values ... you don't want the zig-zag lines just because the order of series changed between data points.
so the only use case where this order doesn't make that much sense (it still might in some scenarios) is in a stacked bar chart.
I suggest leaving this out of this PR, opening a feature request for it and referencing it in original issue.
@ppisljar the zigzag thing only depends on how you order the splitted series. Since we provide the user the ability to change the
that's the order we maintain throughout the visualization. when we find So in conclusion: yes mine is an issue: we are not taking in consideration the split series order by. |
💔 Build Failed |
@markov00 and we never were. But not really relevant, it shouldn't be part of this PR, its gonna be quite a big undertaking. as discussed yesterday over zoom, the problem is that the data structure we use to respresent chart data (series) doesn't hold the information about the ordering of points. |
I'm closing this in favor of #31533, which will address the issue as follows:
@ppisljar Just a note that I think we can check for the presence of an |
Closed in favor of #31533
Resolves part of #17532
Summary
This resolves an issue where the original sort order sent back by ES was
lost for point series / vislib visualizations with split series. This
was due to the way the point series agg response handler generated
series data, only filling in series values as it encountered them
bucket-by-bucket, rather than first looking at all x-values and ordering
them consistently within each series.
With this change, when a series is first created in the
agg_response
, itwill first look at all results, preserving the x-value sort order. Then
when creating new series, it will instantiate a zero-filled array with
the correctly ordered x axis values, filling it in with the real values
as it encounters them.
This duplicates some of the work done in the vislib
zero_injection
component, which can likely be cleaned up further, or possibly removed
entirely.
To Do
- [ ] Determine if(Edit: I think we should look at this in a separate PR to keep things smaller and simpler... plus I want to take additional time for testing should we remove this).vislib/components/zero_injection
can be removedChecklist
- [ ] This was checked for cross-browser compatibility, including a check against IE11- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support- [ ] Documentation was added for features that require explanation or tutorials- [ ] This was checked for keyboard-only and screenreader accessibility