[XY] Usable reference lines for xyVis.

[Kuznietsov]

Summary

Closes part of the issue.
Added referenceLine expression, which is usable for users. It enables the possibility to provide static value for the reference line at the xyVis expression function.

Example

kibana
| selectFilter
| demodata
| tail 10
| xyVis seriesType="line" xAccessor="@timestamp" accessors="price" 
  referenceLines={referenceLine value=10 color="rgba(101, 232, 89, 0.5)" icon="asterisk" fill="below" } 
  referenceLines={referenceLine value=60 icon="bolt" color="blue" fill="above" }
| render

Usage

[elasticmachine]

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

[elasticmachine]

Pinging @elastic/kibana-presentation (Team:Presentation)

[mdefazio]

Just seeing a file rename. 👍

[flash1293]

High level feedback: It seems like this change is only relevant for being able to conveniently place reference lines via expression, but the change propagates through to the actual elastic-charts spec too, including some duplication of logic in reference_line vs reference_line_layer. It seems like we can avoid this additional complexity by absorbing the difference of "reference line layer" vs "reference line" on the expression level by creating the same context structure for both "types" of reference lines (which would involve creating a dummy table for the single value but that doesn't seem like a big problem). This will make it easier to keep the behavior of the reference lines in the charts identical (I'm concerned in the current structure a lot of changes would have to be done in two places but it's very easy to miss one of them) and also reduce the amount of redundant code, making it easier to understand what's going on.

[Kuznietsov]

High level feedback: It seems like this change is only relevant for being able to conveniently place reference lines via expression, but the change propagates through to the actual elastic-charts spec too, including some duplication of logic in reference_line vs reference_line_layer. It seems like we can avoid this additional complexity by absorbing the difference of "reference line layer" vs "reference line" on the expression level by creating the same context structure for both "types" of reference lines (which would involve creating a dummy table for the single value but that doesn't seem like a big problem). This will make it easier to keep the behavior of the reference lines in the charts identical (I'm concerned in the current structure a lot of changes would have to be done in two places but it's very easy to miss one of them) and also reduce the amount of redundant code, making it easier to understand what's going on.

@flash1293, on the other hand, I can't agree with such a point of view. Here are some arguments for the current solution and against merging those two logics:

1. We can simplify the understanding experience for developers by splitting the logic into different logical blocks with individual behavior. If the output of the expression is simple, and it is used directly at the component without any magic logic of looking for some value at some synthetic datatable. "Merging" logics of the components, which have different natures and structures is increasing the complexity. I'm talking mostly about this accessors -> yConfig.forAccessor logic and the other magic of this implementation.
2. I'm sure, there will be a point, at which those two logics will be separated and will go on their own roads. Len's one will be supporting a lot of Lens internal logic, which will be not reproducible from the Canvas (the question of a couple of versions). So, it is better to keep them separated from each other because they are not the same at all. The only common thing there is the elastic-charts components and their configuration.
3. For the purpose of avoiding missing some features, all the configuration of the elastic-charts was unified.
   WDYT?

[flash1293]

I'm sure, there will be a point, at which those two logics will be separated and will go on their own roads. Len's one will be supporting a lot of Lens internal logic, which will be not reproducible from the Canvas (the question of a couple of versions). So, it is better to keep them separated from each other because they are not the same at all

I don't agree with this view - the point of a unified renderer is to keep it aligned in terms of features and code. Branching out into different implementations should be avoided as much as possible and in the current state I'm seeing a bunch of opportunities to avoid branching. For example the files src/plugins/chart_expressions/expression_xy/public/components/reference_lines/reference_line_layer.tsx and src/plugins/chart_expressions/expression_xy/public/components/reference_lines/reference_line.tsx are very similar with the biggest difference being the way the data and config is passed in (table vs value, single y config vs multiple y configs) together with shared bits like calling helper functions in the exact same way and configuring the elastic-chart spec elements in the same way (with duplicated constants like styling) - it seems like one is a special case of the other.

[flash1293]

Breaks Lens:

In terms of inspector integration reference layers are similar to regular data layers and they should be reported in the same way.

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: 8846369
• Storybooks Preview
• Webpack Bundle Analyzer

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
expressionXY	94	98	+4

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
expressionXY	128	129	+1
lens	457	459	+2
total			+3

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
expressionXY	73.9KB	75.0KB	+1.1KB
lens	1.2MB	1.2MB	-8.0B
total			+1.1KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
expressionXY	13	14	+1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
expressionXY	30.0KB	31.6KB	+1.7KB

Unknown metric groups

API count

id	before	after	diff
expressionXY	138	139	+1
lens	532	534	+2
total			+3

History

• 💔 Build #46117 failed d8df746
• 💚 Build #45992 succeeded 0df67db
• 💔 Build #45988 failed 9b9a275
• 💔 Build #45801 failed fd72a67
• 💛 Build #45227 was flaky 7f06310

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

cc @Kunzetsov

[Kuznietsov]

Breaks Lens:

In terms of inspector integration reference layers are similar to regular data layers and they should be reported in the same way.

@flash1293, sure, sorry, that is a merge conflict resolving problem... Will fix it today

@flash1293, I fill like you should restart your kibana.

kibana/src/plugins/chart_expressions/expression_xy/common/expression_functions/reference_line_layer.ts

Line 39 in 8846369

layerId: {

[flash1293]

Tested around and from what I can tell this works really well. One thing I noticed - the standalone reference lines do not try to do the segmenting if they color into the same direction: https://github.com/elastic/kibana/pull/132192/files#diff-f588e9251dfe7febff5c66997c7f3acbcfbd16e41f0d123109cabeef96fb21bdR64

This results in overlapping colors:

What about adopting the same logic of the reference line layer for all standalone reference lines? It's a neat use case to be able to color the areas in the same direction to create "sections" in the chart.

[flash1293]

Approving to not block the PR

[Kuznietsov]

@flash1293, I'll add the feature you've mentioned at the follow-up PR. Thanks for the really good point. I've totally missed it 😃