updating metricvis interpreter func arguments #34532

ppisljar · 2019-04-04T11:11:45Z

Summary

updating metricvis interpreter func arguments:

properties were extracted out of visConfig json to actual arguments
dimensions are using visdimension helper function

for example:
metricvis visConfig='"dimensions": {"metric": { "accessor": 1 }, "bucket": { "accessor": 0, "format": { "type": "ip" }}}, "metric": { "labels": {"show": true}, "percentage": true }}'
becomes:
metricvis metric={vis_dimension 1} bucket={vis_dimension 0 format='ip'} showLabels=true percentage=true

using multiple metric dimensions:
metricvis metric={vis_dimension 1} metric={vis_dimension 0} metric={vis_dimension 1} showLabels=true

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
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~~

For maintainers

~~[ ] This was checked for breaking API changes and was labeled appropriately~~
~~[ ] This includes a feature addition or change that requires a release note and was labeled appropriately~~

elasticmachine · 2019-04-04T11:30:17Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-04T14:44:00Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

lukeelmers

Makes sense to me - had a few nits around the help text for some of these args, otherwise no major concerns.

This also reinforces how it'll be helpful to refactor the build_pipeline utility to clean up some of this string concatenation. Still something on my near-to-midterm list

lukeelmers · 2019-04-05T21:32:38Z

src/legacy/core_plugins/metric_vis/public/metric_vis_fn.js

+      types: ['boolean'],
+      default: false,
+      help: i18n.translate('metricVis.function.percentage.help', {
+        defaultMessage: 'Shows metric in percentage mode. Dont forget to set colorRange.'


Dont -> Don't

Or would it be better to say Requires colorRange to be set?

lukeelmers · 2019-04-05T21:33:12Z

src/legacy/core_plugins/metric_vis/public/metric_vis_fn.js

+      types: ['string'],
+      default: '"Green to Red"',
+      help: i18n.translate('metricVis.function.colorSchema.help', {
+        defaultMessage: 'Color schema to use'


Is there any way to describe this besides "Color schema"? It feels like the type of thing that wouldn't be immediately intuitive to a user

Also (I don't know the answer to this): should colorSchema have a list of options? e.g.

options: ['Green to Red', 'Blah to blahblah'],

nit: "Color scheme" (not schema) seems like the right name here too, at least as an alias.

src/legacy/core_plugins/metric_vis/public/metric_vis_fn.js

lukeelmers · 2019-04-05T21:41:44Z

src/legacy/core_plugins/metric_vis/public/metric_vis_fn.js

+            style: {
+              bgFill: args.bgFill,
+              bgColor: args.colorMode === 'Background',
+              labelColor: args.colorMode === 'Label',


Nice, I like how colorMode simplifies these

lukeelmers · 2019-04-05T21:44:18Z

src/legacy/core_plugins/metric_vis/public/metric_vis_fn.js

@@ -21,7 +21,7 @@ import { functionsRegistry } from 'plugins/interpreter/registries';
 import { i18n } from '@kbn/i18n';

 export const metric = () => ({
-  name: 'kibana_metric',
+  name: 'metricvis',


nit: should this be camelCase if we are avoiding underscores? metricVis

lukeelmers · 2019-04-05T21:45:48Z

src/legacy/core_plugins/metric_vis/public/metric_vis_fn.js

+      types: ['string'],
+      default: '"[{ from: 0, to: 10000 }]"',
+      help: i18n.translate('metricVis.function.colorRange.help', {
+        defaultMessage: 'Color ranges'


I think more specific help text would be useful here (TBH i'm not sure how colorRange even works)

well its just array of objects with from and to property .... we probably want to create another helper function here to generate this objects, and maybe use a multi agg then ? @w33ble ?

Yeah, making users type JSON is pretty much always the wrong thing to do. You could split the args into "from" and "to", or make a new function. I also don't know what this does so it's hard for me to decide which, or offer much more input.

That help text also isn't very helpful 😛

...I wonder if it makes sense to put colorSchema (or colorScheme), colorMode, and even invertColors into its own function too. Are these concepts shared with other visualizations? If so, even partially, a new function (and type) probably makes sense.

There's a lot of "color" concepts to deal with as it is now, and I don't know what is unique to this vis.

elasticmachine · 2019-04-08T10:47:42Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-08T11:50:56Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-08T12:50:24Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

w33ble

Some change requests, nit ones would be nice but are not required.

w33ble · 2019-04-08T20:54:00Z

src/legacy/core_plugins/metric_vis/public/metric_vis_fn.js

+            useRanges: args.useRanges,
+            colorSchema: args.colorSchema,
+            metricColorMode: args.colorMode,
+            colorsRange: JSON.parse(args.colorRange),


Making users type JSON is pretty much always the wrong thing to do.

src/legacy/core_plugins/metric_vis/public/metric_vis_fn.js

w33ble · 2019-04-08T20:57:44Z

src/legacy/core_plugins/metric_vis/public/metric_vis_fn.js

+    },
+    fontSize: {
+      types: ['number'],
+      default: 60,


This should have a help property.

src/legacy/core_plugins/metric_vis/public/metric_vis_fn.js

w33ble · 2019-04-08T21:02:06Z

src/legacy/core_plugins/metric_vis/public/metric_vis_fn.js

+      types: ['string'],
+      default: `'[{ "from": 0, "to": 10000 }]'`,
+      help: i18n.translate('metricVis.function.colorRange.help', {
+        defaultMessage: 'Color ranges: array of objects with from and to property.'


nit: you're one of maybe 6 people that understands the vis object. The rest of us need more guidance than this... what do these numbers mean/do?

creates color range. for example passing in an array:

[ { from: 0, to: 100 }, { from: 100, to: 200 }, ]

will color metric in the "first color" (in scheme) if its value is between 0-100 and in second color if value is between 100 and 200

That's more helpful than "array of objects with from and to property", and that makes it sound like this should both be part of a new function and be a multi arg (multi: true).

@ppisljar I think for clarity we should update this help text since the user isn't expected to provide an array -- they just use the arg multiple times and the parser handles it for them. Maybe something like:

Suggested change

defaultMessage: 'Color ranges: array of objects with from and to property.'

defaultMessage: 'A range object specifying groups of values to which different colors should be applied.'

src/legacy/core_plugins/metric_vis/public/metric_vis_fn.js

w33ble · 2019-04-08T21:21:25Z

I don't fully understand all the args here, but some thoughts:

fontSize could instead be controlled with the existing font function
Since percentage and colorsRange are coupled, a new function might make sense
It looks like colorMode allows customizing the label or the background, but not both; putting all 3 associated args (colorSchema, colorMode, and I think maybe invertColors, but that might be wrong) behind a function may make sense; and then maybe both can be controlled with other enhancements too

ppisljar · 2019-04-09T06:08:10Z

@w33ble it feels like we'll end up with quite some helper functions, that can only be used with 1 or maybe 2 other functions. Whats canvas approach here ? Should we worry about having too many functions that can't be used independently or is that completely ok ?

feels also like we should do some additional work on visualization itself ...
for example if we would to use font function, maybe it makes sense if we actually allow users to change the font, not just fontsize, with it. (but all the options font can handle ?)

w33ble · 2019-04-09T17:27:27Z

Should we worry about having too many functions that can't be used independently or is that completely ok ?

That's OK, we have a bit of that in canvas. There is a line of course, but a single-use function here or there is fine. It's better if the thing the function does can be re-used of couse, which is why a lot of my feedback was trying to group concepts.

for example if we would to use font function, maybe it makes sense if we actually allow users to change the font, not just fontsize, with it.

Yes it is, but that's also something you can do later. The font function is general and encapsulates a concept. Even if you don't use all the parts it can provide, it's still useful, and can work the same way even if the user switches to a different function. And you can make upstream changes to support more font properties and the expression does not change, it just starts doing more than it previously did.

The same could be true for controlling colors, like via a "colorConfig" object, which could be plugged into any argument that controls a color in nearly any function. Maybe... that's the idea I was trying to get across, but I don't know how all these vis objects work so I can't provide more than abstract ideas.

elasticmachine · 2019-05-07T10:14:03Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-08T07:47:19Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

ppisljar · 2019-05-08T08:04:13Z

@w33ble using font function requires it to be moved to open source. I took a look on what that would take and i think this is it:

move font interpreter function
move lib/fonts.js
move some types over

are you ok with going down this route ?

elasticmachine · 2019-05-09T11:12:35Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

w33ble · 2019-05-20T18:49:14Z

@ppisljar I don't see an issue with that off-hand. Are there side effects I'm not aware of? Nothing's really special about that function, there's no reason it can't be in OSS.

…ckage

…plugin/filter-manager-1

…ependency on rootScope

…unction)

elasticmachine · 2019-06-27T12:55:10Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-27T14:01:37Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

lukeelmers

LGTM!

Just wanted to double check this question I had to be sure the help text is okay: #34532 (comment)

This reverts commit 444bafb.

ppisljar added the WIP Work in progress label Apr 4, 2019

ppisljar requested review from w33ble and lukeelmers April 4, 2019 11:11

ppisljar force-pushed the interpreter/semantics/metric branch from bd2f784 to 08156e5 Compare April 4, 2019 14:21

lukeelmers reviewed Apr 5, 2019

View reviewed changes

ppisljar force-pushed the interpreter/semantics/metric branch from 08156e5 to bab5600 Compare April 8, 2019 10:25

ppisljar added :AppArch review v7.2.0 v8.0.0 chore labels Apr 8, 2019

w33ble suggested changes Apr 8, 2019

View reviewed changes

LeeDr added v7.3.0 and removed v7.2.0 labels May 23, 2019

Liza Katz added 3 commits May 29, 2019 10:34

Embeddable and Visualize to use type definitions from kbn-es-query pa…

fb72077

…ckage

Merge remote-tracking branch 'upstream/master' into newplatform/data-…

33e3684

…plugin/filter-manager-1

moved state watching to new filter manager and removed query filter d…

f243294

…ependency on rootScope

ppisljar added 15 commits June 27, 2019 12:54

updating tagcloud and markdown to use prepareValue helper

cd1e474

updating based on review

8dfb03a

adding integration tests

3d6790f

fixing errors

1f443a7

updating based on review

5438dd8

fix type

17abbdf

fix

1e4ee9e

fixing tests

08b61ce

range type to ts

75dccec

updating jest snapshots

ab9dfe7

review update

0541336

fixing interpreter functional tests

2a82801

fixing selenium tests

66155dd

adding inline-style dependency to oss (required by interpreter font f…

baa3201

…unction)

updating screenshot generation and screenshot baselines

afba947

ppisljar force-pushed the interpreter/semantics/metric branch from d9b3451 to afba947 Compare June 27, 2019 11:46

removing screenshot comparison on multi metrics

ed5eb7d

lukeelmers approved these changes Jun 28, 2019

View reviewed changes

ppisljar merged commit 444bafb into elastic:master Jun 28, 2019

ppisljar added a commit to ppisljar/kibana that referenced this pull request Jun 28, 2019

updating metricvis interpreter func arguments (elastic#34532)

c85699b

ppisljar mentioned this pull request Jun 28, 2019

[7.x] updating metricvis interpreter func arguments (#34532) #39872

Merged

ppisljar added a commit that referenced this pull request Jun 28, 2019

updating metricvis interpreter func arguments (#34532) (#39872)

5341d11

ppisljar added a commit that referenced this pull request Jul 1, 2019

Revert "updating metricvis interpreter func arguments (#34532)"

c9f334c

This reverts commit 444bafb.

ppisljar mentioned this pull request Jul 1, 2019

Revert "updating metricvis interpreter func arguments" #40008

Closed

This was referenced Jul 2, 2019

Revert "Query Filter \ Filter Manager: de-angularize and move to data plugin" #40201

Closed

Failing test: Browser Unit Tests."before each" hook."before each" hook #40220

Closed

This was referenced Jul 26, 2019

Fix error preventing tagcloud vis from rendering with significant terms aggs. #42079

Merged

[Tag Cloud] Significant Terms error: JSON.stringify(...) is undefined #41856

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

updating metricvis interpreter func arguments #34532

updating metricvis interpreter func arguments #34532

ppisljar commented Apr 4, 2019 •

edited

Loading

elasticmachine commented Apr 4, 2019

elasticmachine commented Apr 4, 2019

lukeelmers left a comment

lukeelmers Apr 5, 2019

lukeelmers Apr 5, 2019

lukeelmers Apr 5, 2019

w33ble Apr 8, 2019 •

edited

Loading

lukeelmers Apr 5, 2019

lukeelmers Apr 5, 2019

lukeelmers Apr 5, 2019

ppisljar Apr 8, 2019

w33ble Apr 8, 2019 •

edited

Loading

w33ble Apr 8, 2019 •

edited

Loading

elasticmachine commented Apr 8, 2019

elasticmachine commented Apr 8, 2019

elasticmachine commented Apr 8, 2019

w33ble left a comment

w33ble Apr 8, 2019

w33ble Apr 8, 2019

w33ble Apr 8, 2019

ppisljar Apr 9, 2019

w33ble Apr 9, 2019

lukeelmers Jun 25, 2019

w33ble commented Apr 8, 2019

ppisljar commented Apr 9, 2019

w33ble commented Apr 9, 2019 •

edited

Loading

elasticmachine commented May 7, 2019

elasticmachine commented May 8, 2019

ppisljar commented May 8, 2019

elasticmachine commented May 9, 2019

w33ble commented May 20, 2019

elasticmachine commented Jun 27, 2019

elasticmachine commented Jun 27, 2019

lukeelmers left a comment

	defaultMessage: 'Color ranges: array of objects with from and to property.'
	defaultMessage: 'A range object specifying groups of values to which different colors should be applied.'

updating metricvis interpreter func arguments #34532

updating metricvis interpreter func arguments #34532

Conversation

ppisljar commented Apr 4, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Apr 4, 2019

💔 Build Failed

elasticmachine commented Apr 4, 2019

💔 Build Failed

lukeelmers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w33ble Apr 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w33ble Apr 8, 2019 • edited Loading

Choose a reason for hiding this comment

w33ble Apr 8, 2019 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Apr 8, 2019

💔 Build Failed

elasticmachine commented Apr 8, 2019

💔 Build Failed

elasticmachine commented Apr 8, 2019

💔 Build Failed

w33ble left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w33ble commented Apr 8, 2019

ppisljar commented Apr 9, 2019

w33ble commented Apr 9, 2019 • edited Loading

elasticmachine commented May 7, 2019

💔 Build Failed

elasticmachine commented May 8, 2019

💔 Build Failed

ppisljar commented May 8, 2019

elasticmachine commented May 9, 2019

💔 Build Failed

w33ble commented May 20, 2019

elasticmachine commented Jun 27, 2019

💔 Build Failed

elasticmachine commented Jun 27, 2019

💚 Build Succeeded

lukeelmers left a comment

Choose a reason for hiding this comment

ppisljar commented Apr 4, 2019 •

edited

Loading

w33ble Apr 8, 2019 •

edited

Loading

w33ble Apr 8, 2019 •

edited

Loading

w33ble Apr 8, 2019 •

edited

Loading

w33ble commented Apr 9, 2019 •

edited

Loading