kibana
kibana copied to clipboard
[XY] Migrate vis type xy to new unified xy expression
Completes part of https://github.com/elastic/kibana/issues/127115
Summary
Migrate existing vis type xy implementation to the new unified xy expression. As part of this the following changes was made in unified xy expression:
- Moved curveType inside data layer expression functions as we can configure different curve types for different layers.
- Expanded constants of fitting functions
- Added supporting of vis dimension for Accessors from ExtendedDataLayer (as part of that the
table
arg was removed) - Added
splitColumnAccessor
andsplitRowAccessor
forlayeredXyVis
function - Added toggle legend button and legend color picker
- Added
enforce
argument in axis extent so that ignore extents validation - Updated logic for x-domain to use adjusted interval by using function ->
getAdjustedInterval
fromsrc/plugins/charts/public/static/components/endzones.tsx
- Fixed problem with formatting for splitted charts when we use range
- Fixed problem with scrolling of legend when we use new expression in vis editor
- Fixed problem with logging datatable for inspector (added missing accessors, log only one datatable for vis type xy)
- Fixed problem with direction of splitting by row and by column: before split by row uses
splitHorizontally
, now it was usessplitVertically
prop.
The delay agg test fails because of this:
This is how the table looks like (it's using a split series column which is always empty)
But in the new renderer, we do this (basically filtering out a table if the split series is not defined): https://github.com/elastic/kibana/blob/6d1ba64bc59e928393beb31510293846b41b821b/src/plugins/chart_expressions/expression_xy/public/helpers/layers.ts#L102
I think this behavior is fine and it's kind of bad luck this test breaks. The easiest way to work around it I found is to map the shard delay agg to a chart split instead of a split series (you just have to change the schema
to split
instead of group
for the shard delay agg in https://github.com/elastic/kibana/blob/6c93e3ded723fa38dff83bf09eb20123c1b33264/x-pack/test/functional/fixtures/kbn_archiver/dashboard_async/async_search.json#L53
Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)
@elasticmachine merge upstream
About the handleEmptyXAccessor
- I think we can solve this without introducing a new hyper-specific argument. As we know whether there is no x accessor during expression build time, let's do it like this instead:
If there is no x accessor during building the expression:
- Pass
all
as x accessor - Pass
All docs
as xAxisConfig.title - In the expression building logic, before the
layeredXyVis
function call add| staticColumn name="all" value="_all"
For the renderer this column should behave like a "real" column, so we don't need any extra logic in the renderer and no additional argument
About the
handleEmptyXAccessor
- I think we can solve this without introducing a new hyper-specific argument. As we know whether there is no x accessor during expression build time, let's do it like this instead: If there is no x accessor during building the expression:
- Pass
all
as x accessor- Pass
All docs
as xAxisConfig.title- In the expression building logic, before the
layeredXyVis
function call add| staticColumn name="all" value="_all"
For the renderer this column should behave like a "real" column, so we don't need any extra logic in the renderer and no additional argument
done.
The color picking on dashboard seems to work, but it does not pick up existing colors.
- Configure an agg based xy chart off this PR (either main or on a released version) and set a color on a dashboard
- Export the dashboard and import it into a instance running this PR
- Default color is shown instead of picked color
Fixed.
As discussed offline, the behavior of useAdjustedInterval
always make sense - we can remove the argument and always trigger this behavior
As discussed offline, the behavior of
useAdjustedInterval
always make sense - we can remove the argument and always trigger this behavior
done
Create new vertical bar chart, change chart type for the first metric to "Line" - crash:
New implementation throws on active time marker setting, without date histogram old implementation ignores it:
Dashed and dot-dashed styles look different:
Clicking a data point in a split chart ignores the split chart dimension. Like this chart:
Clicking a data point will just apply the time filter from the x axis while on main it will show the modal for the split chart filter
Other bucket on date field doesn't render properly:
In horizontal bar chart the labels are angled the other way ( the way it used to work seems better to me):
Show labels setting is ignored (on all axes):
"Vertical" alignment on axis labels is 180 degrees rotated:
Dot size ratio is not interpreted in the same way - the dots are much larger in the new version:
Axis extents are ignored:
@flash1293 I fixed all your remarks. Could you please review again?
@elasticmachine merge upstream
It looks like on existing visualizations the legend visibility is ignored:
Flights sample data (left is new, right is old)
Seems like it's never persisted in the new version
Scale to data bounds is still ignored for bar charts:
If there are multiple metrics, their names are not added to the legend entry anymore (left is PR, right is main):
Also, the color assignment is different now (it used to be identical to Lens before) (left is PR, right is main):
Also the tooltip is wrong if there are multiple metrics - it's always showing the name of the first:
PR:
Main:
This works as expected, I found some small bugs:
-
When the user selects the compatibility palette, the overwrite colors (that appear when the user clicks a legend item) should be from the compatibility palette and not from the default EUI. This is a screenshot with the expected colors
-
Percentiles are not colored correctly, neither the percentile ranks
-
The dot size information is not depicted on the detailed tooltip
-
hiding the second y layer breaks the chart
@elasticmachine merge upstream
@elasticmachine merge upstream
Thanx Vlad! I found another bug. The overwrite color doesnt work for multiple metrics
Moreover, splitting a percentiles metric works differently. (same for percentile ranks)
In main. (check the legend order in comparison with the screenshot below)
Same chart here
Also the split series order seems wrong (possibly related to the percentiles bug mentioned above)
![image](https://user-images.githubusercontent.com/17003240/183377446-c1254d4b-d10a-4034-bb88-9932d86782f3.png)