Dynamically update vega spec to show VisLayers

[ohltyler]

Signed-off-by: Tyler Ohlsen [email protected]

Description

This PR adds the logic in the expressions fn to process any existing VisLayers and augment the vega spec as needed. This is done through 2 helper fns which are added in line_vega_spec_fn:

1. addPointInTimeEventsLayersToTable(): dynamically updating the source datatable with additional column(s) and data representing a particular PointInTimeEventsLayer
2. addPointInTimeEventsLayersToSpec(): dynamically update the vega spec by: (1) adding a rule layer to be visible when hovering over an annotation , (2) adding circle layers for each PointInTimeEventsLayer, and (3) adding parameters to handle user interaction, including expanding the circle size and making the corresponding rule visible on the chart.

Refactoring changes

The existing functions that were in line_vega_spec_fn have moved to expressions/helpers. This is to clean up the function file itself, and to be consistent with the helper functions that are now also being imported from vis_augmenter. The functions themselves are the same except for a minor change to createSpecFromDatatable() to omit adding layers for columns that are VisLayers, since that will now be handled by the new addPointInTimeEventsLayersToSpec() fn mentioned above.

A note on manual creation of empty buckets

By default, the queries being made to fetch the source visualization data do not include extra parameters like extended_bounds or hard_bounds to return empty buckets. Instead, only buckets with results are returned. The current vislib library has standalone logic for extending the chart bounds to match the min/max time range as the given context (the dashboards time range, or vis time range if in edit view). To overlay events on a visualization, we may need to populate time buckets that were previously not included in the source table. Consider the following: given a 30 day time range with 1 day time buckets, and no vis data is present from days 0-5, but there was an event on day 2. In this case, we need to create and populate a bucket for day 2, and add the event there. Currently there is no easy way to inject logic in the source queries to include empty buckets via extra *_bounds params. For example, extended_bounds does not filter, and would require the histogram agg to be nested within a filter time range. Also, for date_histogram, it is not even offered as a setting via the vis editor - only histogram offers for extended_bounds and that is all. So, rather than trying to complicate the underlying queries to accommodate VisLayer events by covering all scenarios for how to add any extra parameters to the histogram query parameters, we can post-process the source datatable by manually adding rows if there are any missing. Given the interval and min/max bounds, this is not too difficult. It also guarantees that we are matching the same chart bounds that would be achieved by the vislib library.

A note on tooltips

Custom tooltips, including how to potentially add icons / other VisLayer metadata, will be added as part of #3317. This PR doesn't add any configuration for tooltips on the VisLayer event datapoints.

A note on chart autosizing

Vega-lite has a known limitation where concatenated views cannot use the default fit autosize configuration to make the height/width fill in the available space in the parent container. Because of the way we augment the spec with VisLayer data via vconcat to have multiple views (one for the base vis and one for the event vis), we need to implement a workaround to still autosize correctly. This is done by adding the following:

• a showEvents field in the VegaParser to determine when we want to render this special version of a vega chart
• an autosize field of fit-x to automatically determine the width (this works because we are doing vconcat instead of a horizontal-concatenated hconcat spec)
• a helper function updateBaseVisHeight() to augment the compiled vega spec's signal on the base vis height, and manually set it as a function of the container height. So, whenever the container dimensions change, this will be triggered, and a new height will be determined for the base vis chart. Note we hardcode the event vis height since we have control of the data within it, so we don't need to have any auto-size logic for that chart.

For more details on how these layers and interactions will look, see the mockups in #2880

Issues Resolved

Closes #3130

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

[codecov-commenter]

Codecov Report

Merging #3145 (3f4e1e1) into feature/feature-anywhere (c8af939) will decrease coverage by 0.02%.
The diff coverage is 92.41%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@                     Coverage Diff                      @@
##           feature/feature-anywhere    #3145      +/-   ##
============================================================
- Coverage                     66.47%   66.45%   -0.02%     
============================================================
  Files                          3222     3225       +3     
  Lines                         61818    61980     +162     
  Branches                       9529     9542      +13     
============================================================
+ Hits                          41092    41189      +97     
- Misses                        18445    18499      +54     
- Partials                       2281     2292      +11     

Flag	Coverage Δ
Linux	66.45% <92.41%> (+0.03%)	⬆️
Windows	?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/plugins/vis_augmenter/public/index.ts	0.00% <ø> (ø)
...s/vis_type_vega/public/vega_view/vega_base_view.js	50.29% <0.00%> (-5.80%)	⬇️
...lugins/vis_type_vega/public/vega_view/vega_view.js	0.00% <0.00%> (-100.00%)	⬇️
...ugins/vis_type_vislib/public/line_to_expression.ts	6.66% <ø> (ø)
src/plugins/visualizations/public/plugin.ts	0.00% <0.00%> (ø)
...ins/vis_type_vega/public/data_model/vega_parser.ts	77.86% <71.42%> (-2.81%)	⬇️
...lugins/vis_type_vega/public/expressions/helpers.ts	94.23% <94.23%> (ø)
src/plugins/vis_augmenter/public/vega/helpers.ts	97.97% <97.97%> (ø)
src/plugins/vis_augmenter/public/constants.ts	100.00% <100.00%> (ø)
src/plugins/vis_augmenter/public/test_constants.ts	100.00% <100.00%> (ø)

... and 10 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[AMoo-Miki]

Any reason why this should not show up in the changelog?

[ohltyler]

Any reason why this should not show up in the changelog?

It's going to a feature branch, and will be added as one item in the changelog when merged to main/2.x

[AMoo-Miki]

It's going to a feature branch

The feature branches on this repo are held to the same high standards as the main branch; we enforce PRs and all the checks on them.

[ohltyler]

It's going to a feature branch

The feature branches on this repo are held to the same high standards as the main branch; we enforce PRs and all the checks on them.

I was told the changelog checks can be ignored since it won't make logical sense to add them here and would cause conflicts when eventually merging to the respective branches, and that they will be added when finally merging later on.

cc @joshuarrrr

[ohltyler]

Coming from #3106 (comment), we may be able to refactor some/all of the config into vis_augmenter. It's also worth exploring moving these helper fns into there and maintaining.

When we are able to rebase this, I'll look into how that may look.

[ashwin-pc]

Really good PR @ohltyler. The detailed PR description with context was really helpful in reviewing this. Most of my comments are nits which you can choose to make or not. The only real blocker for me was the showEvents flag. Its concerning because it in the Vega view itself when its usefulness seems to be limited to time series charts and even in there its applicable only to the event chart that you are augmenting. Can we either find a way to avoid that? or if not at least make it more general purpose?

[ashwin-pc]

nit: can you add a TODO: here so that its easy to identify in future?

[ashwin-pc]

Also a little more context about this would help. I know you mention in the PR description that this is to fit the events vis, but how does that scale for other plugins that may want to augment the data? what if the chart isnt a time series chart? If i were someone who wanted to augment the data on a pie chart or heat map chart and ran into a similar limitation there, would this be useful for me? or can this be modified to support a more general usecase?

[ohltyler]

The only real blocker for me was the showEvents flag. Its concerning because it in the Vega view itself when its usefulness seems to be limited to time series charts and even in there its applicable only to the event chart that you are augmenting. Can we either find a way to avoid that? or if not at least make it more general purpose?

Currently showEvents is just part of the vega parser, which is responsible for maintaining parameters of other similar scope, such as useMap which has downstream effects including spec changes and autosize changes. showEvents is used in the parser for changing autosize as well (see showEventsAutosize in vega_parser).

I chose the name showEvents because I thought it was a good balance between specific and generic; right now it's only being used for line charts, but may be expanded to other time series charts and potentially even other chart types. It may grow in responsibility, or potentially refactored and broken up if the flag becomes not fine-grained enough to be reasonable. Lmk what you think.

[joshuarrrr]

@ohltyler This looks great! A couple minor comments and questions.

[ohltyler]

The only real blocker for me was the showEvents flag. Its concerning because it in the Vega view itself when its usefulness seems to be limited to time series charts and even in there its applicable only to the event chart that you are augmenting. Can we either find a way to avoid that? or if not at least make it more general purpose?

Currently showEvents is just part of the vega parser, which is responsible for maintaining parameters of other similar scope, such as useMap which has downstream effects including spec changes and autosize changes. showEvents is used in the parser for changing autosize as well (see showEventsAutosize in vega_parser).

I chose the name showEvents because I thought it was a good balance between specific and generic; right now it's only being used for line charts, but may be expanded to other time series charts and potentially even other chart types. It may grow in responsibility, or potentially refactored and broken up if the flag becomes not fine-grained enough to be reasonable. Lmk what you think.

@ashwin-pc I've updated showEvents to be broken down into a boolean map for each VisLayer type in the latest commit: feb1d03

This should make it a bit more clear and intentional

[ashwin-pc]

Thanks @ohltyler. i like this implimentation better with the additional showEvents flag. We can generalize in future when we have other layer types that we support, but for now this works fine :)

[joshuarrrr]

Thanks @ohltyler for the refactoring - in particular, the tests are much more readable now. I'm still curious to see if we can get most of the automation passing, so I'll keep an eye on it - regardless I'll aim to merge by EOD.