Create switch to render line charts using vega-lite

[lezzago]

Description

Line charts can now be rendered through vega-lite instead of vislib by having a switch that is enabled whenever annotations need to appear on the visualization.

Issues Resolved

#2915

Check List

• All tests pass
  • yarn test:jest
  • yarn test:jest_integration
  • yarn test:ftr
• New functionality includes testing.
• New functionality has been documented.
• Update CHANGELOG.md
• Commits are signed per the DCO using --signoff

[lezzago]

Opened an issue to have integration tests for this: opensearch-project/opensearch-dashboards-functional-test#425

[ohltyler]

Vislib charts support brush-to-zoom and tooltips when hovering over specific datapoints. Have you looked at the possibility of adding these for vega-lite, or how we should call out such differences when rendering in vega-lite if we won't support them initially?

I think brush-to-zoom may be a bit complex since it will require function callbacks to re-fetch data when a user drags on the canvas and changes the time range. Vislib did that by having embedded functions in the implementation-level of the charts which doesn't map quite the same in this case.

Curious of your thoughts on this @joshuarrrr - do you see this as something Dashboards should take on later, as part of the overall migration to vega-lite, or as a blocker if not implemented & included for this initial release?

[lezzago]

Vislib charts support brush-to-zoom and tooltips when hovering over specific datapoints. Have you looked at the possibility of adding these for vega-lite, or how we should call out such differences when rendering in vega-lite if we won't support them initially?

I think brush-to-zoom may be a bit complex since it will require function callbacks to re-fetch data when a user drags on the canvas and changes the time range. Vislib did that by having embedded functions in the implementation-level of the charts which doesn't map quite the same in this case.

Curious of your thoughts on this @joshuarrrr - do you see this as something Dashboards should take on later, as part of the overall migration to vega-lite, or as a blocker if not implemented & included for this initial release?

I have supported tooltips, however, brush to zoom is more complex and has become out of scope for this as we will have it rendering using vega-lite whenever we need to do the annotation functionality. This way also, users won't be interacting with the vega lite render from where they create the line chart and only on the dashboard page.

[ohltyler]

Vislib charts support brush-to-zoom and tooltips when hovering over specific datapoints. Have you looked at the possibility of adding these for vega-lite, or how we should call out such differences when rendering in vega-lite if we won't support them initially?
I think brush-to-zoom may be a bit complex since it will require function callbacks to re-fetch data when a user drags on the canvas and changes the time range. Vislib did that by having embedded functions in the implementation-level of the charts which doesn't map quite the same in this case.
Curious of your thoughts on this @joshuarrrr - do you see this as something Dashboards should take on later, as part of the overall migration to vega-lite, or as a blocker if not implemented & included for this initial release?

I have supported tooltips, however, brush to zoom is more complex and has become out of scope for this as we will have it rendering using vega-lite whenever we need to do the annotation functionality. This way also, users won't be interacting with the vega lite render from where they create the line chart and only on the dashboard page.

Right, we'll need to make it clear in docs / release notes that there is this functional difference when annotations are rendered. Have you seen other functional differences, or obvious visual differences in your testing so far?

[joshuarrrr]

Great start! I think the most important structural recommendations I have are to

1. minimize the logic that goes in the conditional that determines the rendering pipeline (so that other expression callers can also have control over which to use)
2. encapsulate the chart-type-specific spec generation with a naming/directory location approach that makes it easy to add more such mappings in the future.
3. add unit tests to demonstrate which configurations and mappings we support so far.

[joshuarrrr]

@lezzago Should this be moved out of draft status?

[ohltyler]

For testing, could you add a bunch of (what we think will be valid/eligible) vis configs and throw them at the function to make sure they don't break / throw NPEs / etc.? This should help boost confidence that we won't have things crashing when running existing charts through and displaying w/ vega.

Eligibility checking tests can be more responsible for testing against a broad type of vis configs, including non-line-types, etc.

[ohltyler]

just to confirm - this won't shrink the chart bounds to fit such that there is no empty positions at the beginning and end of the chart, right? I see you had a comment referencing a different approach here #3106 (comment)

Also, I think we still need time buckets even if there is empty data from the charts (empty rows). This is so the events can still fit into such empty buckets if that is the closest bucket based on their timestamps. Typically this wouldn't be the case with associated plugin resources, but because we are allowing unrelated resources to be added to the charts, it's possible that we have events even with empty data shown for a particular vis

[ohltyler]

If you weren't able to find a good solution via agg params, I think we will need a helper fn and artificially add rows to fit the full bounds.

[ohltyler]

Discussed offline - this won't shrink the chart bounds. We can keep the domain as temporal.

As a follow up I'll need to create a helper fn to add empty rows for potential event datapoints

[lezzago]

I thought that the previous approach was needed since there were some local issues with using scale and domain. The line charts actually do something very similar and dont create extra buckets. We will need to create a helper fn to add rows as changing agg params can cause a lot of unknown side effects.

[ohltyler]

Yes, just adding rows after-the-fact is what we need, we should not touch the agg params

[joshuarrrr]

Looking really great - agree with @ohltyler on just making sure we're consistent with specifying line in all the naming. That's the only blocker from my perspective, although I left some other minor questions and comments.

There are a couple old unresolved suggestions from my last review in src/plugins/vis_type_vislib/public/line_to_expression.ts - up to you whether you want to take them or ignore them. We also want to validate error handling and rendering against UX mocks, but that can be scoped in a follow-up issue and PR.

I saw you added some unit tests while I was reviewing - thanks!

[joshuarrrr]

do we have error handling for the else condition?

[lezzago]

Essentially this is an empty chart which would be expected when they didn't define the x axis yet. There is a unit test for this

[joshuarrrr]

I think there's a more standard error-rendering embeddable pattern to follow - but not a blocker, I can make a follow-up issue.

[joshuarrrr]

why an empty string instead of undefined?

[lezzago]

Thanks for pointing this out. There was a bug here and changed this to visParams.grid.valueAxis as it would be true if its there and false if its not there.

[ohltyler]

not trying to block, but I feel we should have some more test cases for this. I get that a lot of the invalid specs will be filtered out at the eligibility layer, but having some tests for empty data, single data point, etc. could be helpful

Might be worth extracting the table generation in a helper fn. Then can call that to generate the table and inject to simplify and shorten the test files

[ohltyler]

if you feel more of this is covered in eligibility checks and integration tests, I'm fine with that too

[joshuarrrr]

I'd also prefer more tests - but I'm OK spinning that off into a follow-up issue and PR so we don't slow the progress.

[lezzago]

Added a bunch more and broke up the bigger function to make it easier to test different scenarios

[ohltyler]

No other blockers from me - looks good!