Finalize eligibility check for augmenting visualizations by vis augmenter

[lezzago]

Description

Finalize eligibility check for augmenting visualizations by vis augmenter

Eligibility Requirements:

• Must be a vislib line chart type
• Must contain one Y-axis metric aggregation
• Must not have non Y-axis metric aggregation types
• Must use date histogram aggregation type for the X-axis bucket
• Must have X-axis on the bottom
• There can only be 1 X-axis aggregation bucket setup
• The dimensions must be valid as well, this is determined by having an X-axis to be setup

Issues Resolved

#3268

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 #3687 (6ceeb3d) into feature/feature-anywhere (03b393f) will increase coverage by 0.01%.
The diff coverage is 85.71%.

📣 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    #3687      +/-   ##
============================================================
+ Coverage                     66.50%   66.51%   +0.01%     
============================================================
  Files                          3244     3244              
  Lines                         62432    62448      +16     
  Branches                       9637     9642       +5     
============================================================
+ Hits                          41520    41539      +19     
+ Misses                        18604    18576      -28     
- Partials                       2308     2333      +25     

Flag	Coverage Δ
Linux	66.46% <85.71%> (+0.01%)	⬆️
Windows	66.46% <85.71%> (+0.01%)	⬆️

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% <ø> (ø)
...ugins/vis_type_vislib/public/line_to_expression.ts	6.66% <0.00%> (ø)
src/plugins/vis_augmenter/public/test_constants.ts	100.00% <100.00%> (ø)
src/plugins/vis_augmenter/public/utils/utils.ts	97.29% <100.00%> (+0.23%)	⬆️
...lugins/vis_type_vega/public/expressions/helpers.ts	95.00% <100.00%> (+0.76%)	⬆️

... and 10 files with indirect coverage changes

[ohltyler]

LGTM!

[abbyhu2000]

LGTM!

[abbyhu2000]

just a question here: can we just rename the ids so they do not include '.'?

[lezzago]

If we change it how it is in OpenSearchDashboardsDatatable, this can cause a lot of implications as we need to modify OpenSearchaggsExpressionFunction.

[joshuarrrr]

@lezzago Looks good - a couple questions and some minor style suggestions, which can be implemented in a follow-up PR if you'd prefer.

[joshuarrrr]

Style nit for test readability - I prefer to clone the existing default object so that it's easier to see exactly which changes take it from valid to invalid. Something like

Suggested change

	const invalidConfigStates = [
	{
	enabled: true,
	type: 'date_histogram',
	params: {},
	},
	{
	enabled: true,
	type: 'dot',
	params: {},
	schema: 'radius',
	},
	{
	enabled: true,
	type: 'metrics',
	params: {},
	schema: 'metrics',
	},
	];
	const invalidConfigStates = [
	...configStates,
	{
	enabled: true,
	type: 'dot',
	params: {},
	schema: 'radius',
	},
	];

(and I'd probably rename configStates to validConfigStates to make it even easier to mentally track.)

Or maybe even just define the two default config states separately so that you can compose them, for instance L103-114 could be written like:

      const invalidConfigStates = [
        validMetricConfigState,
        {
          ...validDateHistConfigState,
          type: 'histogram',
        },
      ];

Just some ideas to play around with.

[lezzago]

I made some updates for this to improve this, but more improvements can happen if we make these into data models. That can be done in a future PR.

[joshuarrrr]

interesting finding!

[joshuarrrr]

In the future, I still think there's a better way to set this up, so that visAugmenter can simply specify that vega-lite rendering should be used, and it wouldn't be necessary to import here. But that's something we an come back to.

[lezzago]

Yea that seems like a better way to structure this, but we would have to come back to that later.