adds metric_vis_renderer #57694

ppisljar · 2020-02-14T15:07:17Z

Summary

adds renderer for metric_vis (so it doesn't need to use visualization renderer anymore)

VisualizationContainer component was added to visualizations plugin which should be used to wrap any visualization.
visualizations should use VisualizationNoResults component when it wants to show a no-result message
metric vis toAST() function to produce expression out of vis configuration

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-02-14T15:54:20Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

lukeelmers · 2020-03-05T00:00:06Z

Finally got around to looking through this....

we could provide utility functions to make it easier for visualizations to implement the above behavior in consistent way.

error handling could be moved to render() function on expression service.

I think these ideas make the most sense for the unanswered items. One thing I'm not sure about is how/if to deal with non-React scenarios. We could provide helpers or HOCs to decorate React components, but that doesn't help folks using other rendering technologies.

I wonder if there'd be a way to consolidate all of this into some sort of render function wrapper -- where you could give us your visData/visConfig, domNode, and render function, and we conditionally render it based on the status.

visualizationRenderWrapper(
  domNode: HTMLElement;
  config: { visData, visConfig };
  renderFn: (domNode) => render(<MyApp />, domNode);
);

Then in visualizationRenderWrapper:

we call the provided render fn if everything seems to be okay
if there's an error/no-data/loading situation, we could skip calling the fn and hijack the domNode to render our own thing into it instead.
we could also potentially add the visualization class to the domNode itself, and then pass it back to the provided render function.

This is mostly a brain dump - I haven't thought too much about the details on this, so there's likely something I'm overlooking.

Edit: Come to think of it, I'm not sure the wrapper idea works entirely. Because we don't always know the shape of the data that a given visualization function provides to a renderer. hmm

ppisljar · 2020-07-30T13:59:43Z

@elasticmachine merge upstream

src/plugins/vis_type_metric/public/metric_vis_renderer.tsx

src/plugins/vis_type_metric/public/components/metric_vis_component.tsx

lukeelmers

Wondering if as a part of this we should also be adding a toExpression function on the metric vis and removing the metric vis function from build_pipeline? That would help validate that this works and demonstrate the whole workflow for creating a vis.

Edit: Also I just realized that build_pipeline is calling toExpression whereas the vis object has toAST defined... I'm guessing that's a bug we hadn't noticed yet?

lukeelmers · 2020-08-13T20:12:29Z

src/plugins/visualizations/public/vis_types/base_vis_type.ts

@@ -54,7 +54,7 @@ export class BaseVisType {
  stage: 'experimental' | 'beta' | 'production';
  isExperimental: boolean;
  options: Record<string, any>;
-  visualization: VisualizationControllerConstructor;
+  visualization: VisualizationControllerConstructor | undefined;


nit: Maybe you could do visualization?: VisualizationControllerConstructor; instead of adding undefined?

yeah i tried that, but it gets into some ts errors so i went with this

src/plugins/visualizations/public/components/visualization_container.tsx

src/plugins/vis_type_metric/public/metric_vis_renderer.tsx

ppisljar · 2020-08-15T12:22:12Z

@elasticmachine merge upstream

ppisljar · 2020-08-17T14:31:09Z

@elasticmachine merge upstream

lukeelmers

This LGTM now, added a few final comments which are mostly nits.

src/plugins/visualizations/public/legacy/build_pipeline.ts

lukeelmers · 2020-08-18T19:04:05Z

src/plugins/visualizations/public/legacy/build_pipeline.ts

-  // request handler
-  if (vis.type.requestHandler === 'courier') {
-    pipeline += `esaggs
+  if (vis.type.toAST) {


I always forget what naming convention we decided on for these. toAST? toExpression? toExpressionAst?

I think it's all over the place now, but might be good to choose something we are okay with applying everywhere before all of the vis renderers get written.

i agree, you are using toExpressionAst ?

aggs are using toExpressionAst

src/plugins/vis_type_metric/public/to_ast.ts

lukeelmers · 2020-08-18T19:10:20Z

src/plugins/vis_type_metric/public/to_ast.ts

+  }
+
+  // @ts-ignore
+  const metricVis = buildExpressionFunction<MetricVisExpressionFunctionDefinition>('metricVis', {


curious, what does TS not like about the usage with buildExpressionFunction here? wondering if it's something fixable in the function builder

we need to cleanup the types i think, we need a separation between argumnent types (arguments user is providing) and argument types (arguments as received inside fn function .... with defaults injected) which we currently don't have.

ah, yeah so right now this is a tradeoff between losing type safety when using the expression builder, or having a worse DX when writing the expression function.

you wouldn't need to ignore this if the typings for metric_vis_fn args reflected the actual function definition configuration (that is, args which are optional are typed as optional, args which are required are typed as required).

in that case this would work, but it would mean inside the function implementation itself you would need to assert that you know something exists:

colorRange: args.colorRange!, percentageMode: args.percentageMode!, ...etc

i think there are a couple ways to solve this which we could discuss -- for now i've pushed an update to this PR to switch out the ts-ignore with ts-expect-error so that we remember to remove it later when we fix this

src/plugins/vis_type_metric/public/to_ast.ts

src/plugins/vis_type_metric/public/metric_vis_renderer.tsx

src/plugins/expressions/common/ast/build_function.ts

lukeelmers

LGTM. We should sort out the issue with expression definition typings and how they are used in the function builder, but can discuss that separately as it's orthogonal to this effort.

ppisljar · 2020-08-19T15:19:48Z

@elasticmachine merge upstream

ppisljar · 2020-08-19T17:29:07Z

@elasticmachine merge upstream

ppisljar · 2020-08-19T18:49:41Z

@elasticmachine merge upstream

kibanamachine · 2020-08-19T19:16:07Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: e2b14b6

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
visTypeMetric	260	+2	258
visualizations	135	+1	134
total		+3

page load bundle size

id	value	diff	baseline
expressions	348.9KB	+65.0B	348.9KB
visTypeMetric	581.1KB	+4.8KB	576.3KB
visualizations	408.5KB	-541.0B	409.0KB
total		+4.3KB

History

💛 Build #69114 was flaky 2f3d7c1
💚 Build #69092 succeeded 3cc385f
💚 Build #69004 succeeded b22e91a
💔 Build #68985 failed f907fb5acc6563b45ff2e44049d0648fa54f13af
💔 Build #68971 failed 679240cb49df07b28619c70d3ad26f4068e7abac

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

ThomThomson

Tested locally on Chrome, for a quick sanity check of the functionality of metric vis.
Kibana App code LGTM!

ppisljar added release_note:skip Skip the PR/issue when compiling release notes v8.0.0 labels Feb 14, 2020

ppisljar force-pushed the metricRenderer branch from a02c601 to 9556e8d Compare February 14, 2020 15:54

ppisljar requested a review from a team as a code owner February 14, 2020 15:54

ppisljar added chore Team:AppArch labels Feb 14, 2020

ppisljar added the v7.7.0 label Feb 14, 2020

ppisljar requested a review from lukeelmers February 14, 2020 15:59

ppisljar force-pushed the metricRenderer branch from 9556e8d to b260b95 Compare February 14, 2020 23:44

ppisljar changed the title ~~[PoC] adds metric_vis_renderer~~ adds metric_vis_renderer Feb 15, 2020

ppisljar changed the title ~~adds metric_vis_renderer~~ [PoC] adds metric_vis_renderer Feb 15, 2020

spalger added v7.7.1 and removed v7.7.0 labels May 14, 2020

lukeelmers added the WIP Work in progress label Jul 7, 2020

ppisljar added v7.11.0 and removed v7.7.1 labels Jul 21, 2020

ppisljar force-pushed the metricRenderer branch from 7b3eb77 to 2caacc8 Compare July 29, 2020 14:04

ppisljar requested a review from a team July 29, 2020 14:04

ppisljar removed the WIP Work in progress label Jul 29, 2020

ppisljar changed the title ~~[PoC] adds metric_vis_renderer~~ adds metric_vis_renderer Jul 30, 2020

lukeelmers reviewed Aug 5, 2020

View reviewed changes

src/plugins/vis_type_metric/public/metric_vis_renderer.tsx Outdated Show resolved Hide resolved

src/plugins/vis_type_metric/public/components/metric_vis_component.tsx Outdated Show resolved Hide resolved

streamich mentioned this pull request Aug 11, 2020

Renderers for all visualizations #46801

Closed

13 tasks

ppisljar force-pushed the metricRenderer branch 4 times, most recently from 745b52d to 035cf70 Compare August 13, 2020 09:22

lukeelmers reviewed Aug 13, 2020

View reviewed changes

ppisljar force-pushed the metricRenderer branch from 941ccc6 to fc9b5a0 Compare August 17, 2020 13:39

botelastic bot added the Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) label Aug 17, 2020

ppisljar force-pushed the metricRenderer branch from 1523ca0 to acf2ed1 Compare August 17, 2020 15:03

lukeelmers reviewed Aug 18, 2020

View reviewed changes

adds metric_vis_renderer

3bef8d1

ppisljar force-pushed the metricRenderer branch 2 times, most recently from 679240c to f907fb5 Compare August 19, 2020 07:42

adding toAST function

b22e91a

ppisljar force-pushed the metricRenderer branch from f907fb5 to b22e91a Compare August 19, 2020 09:13

Use ts-expect-error

3cc385f

lukeelmers force-pushed the metricRenderer branch from ea762e1 to 3cc385f Compare August 19, 2020 14:32

lukeelmers approved these changes Aug 19, 2020

View reviewed changes

Merge branch 'master' into metricRenderer

2f3d7c1

Merge branch 'master' into metricRenderer

e2b14b6

ThomThomson approved these changes Aug 19, 2020

View reviewed changes

ppisljar merged commit 28df926 into elastic:master Aug 20, 2020

ppisljar mentioned this pull request Aug 20, 2020

[7.x] adds metric_vis_renderer (#57694) #75518

Merged

ppisljar added a commit to ppisljar/kibana that referenced this pull request Aug 20, 2020

adds metric_vis_renderer (elastic#57694)

1351b60

ppisljar added a commit that referenced this pull request Aug 20, 2020

adds metric_vis_renderer (#57694) (#75518)

3419d00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds metric_vis_renderer #57694

adds metric_vis_renderer #57694

ppisljar commented Feb 14, 2020 •

edited

Loading

elasticmachine commented Feb 14, 2020

lukeelmers commented Mar 5, 2020 •

edited

Loading

ppisljar commented Jul 30, 2020

lukeelmers left a comment •

edited

Loading

lukeelmers Aug 13, 2020

ppisljar Aug 14, 2020

ppisljar commented Aug 15, 2020

ppisljar commented Aug 17, 2020

lukeelmers left a comment

lukeelmers Aug 18, 2020

ppisljar Aug 19, 2020 •

edited

Loading

lukeelmers Aug 19, 2020

lukeelmers Aug 18, 2020

ppisljar Aug 19, 2020

lukeelmers Aug 19, 2020

lukeelmers left a comment

ppisljar commented Aug 19, 2020

ppisljar commented Aug 19, 2020

ppisljar commented Aug 19, 2020

kibanamachine commented Aug 19, 2020

ThomThomson left a comment

adds metric_vis_renderer #57694

adds metric_vis_renderer #57694

Conversation

ppisljar commented Feb 14, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Feb 14, 2020

lukeelmers commented Mar 5, 2020 • edited Loading

ppisljar commented Jul 30, 2020

lukeelmers left a comment • edited Loading

Choose a reason for hiding this comment

lukeelmers Aug 13, 2020

Choose a reason for hiding this comment

ppisljar Aug 14, 2020

Choose a reason for hiding this comment

ppisljar commented Aug 15, 2020

ppisljar commented Aug 17, 2020

lukeelmers left a comment

Choose a reason for hiding this comment

lukeelmers Aug 18, 2020

Choose a reason for hiding this comment

ppisljar Aug 19, 2020 • edited Loading

Choose a reason for hiding this comment

lukeelmers Aug 19, 2020

Choose a reason for hiding this comment

lukeelmers Aug 18, 2020

Choose a reason for hiding this comment

ppisljar Aug 19, 2020

Choose a reason for hiding this comment

lukeelmers Aug 19, 2020

Choose a reason for hiding this comment

lukeelmers left a comment

Choose a reason for hiding this comment

ppisljar commented Aug 19, 2020

ppisljar commented Aug 19, 2020

ppisljar commented Aug 19, 2020

kibanamachine commented Aug 19, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

page load bundle size

History

ThomThomson left a comment

Choose a reason for hiding this comment

ppisljar commented Feb 14, 2020 •

edited

Loading

lukeelmers commented Mar 5, 2020 •

edited

Loading

lukeelmers left a comment •

edited

Loading

ppisljar Aug 19, 2020 •

edited

Loading