updating tagcloud interpreter func arguments #33773

ppisljar · 2019-03-25T09:10:58Z

Summary

updating tagcloud interpreter func arguments:

properties were extracted out of visConfig json to actual arguments

for example:
tagcloud visConfig='{"metric": { "accessor": 1 }, "bucket": { "accessor": 0, "format": { "type": "ip" }}, "scale":"linear", "orientation": "angled", "minFontSize": 12, "maxFontSize": 24 }'
becomes:
tagcloud metric={vis_dimension 1} bucket={vis_dimension 0 format='ip'} scale='linear' orientation='angled' minFontSize: 12, maxFontSize: 24

this one was pretty simple, but indicates what we want to do.

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-03-25T09:30:30Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-26T11:26:22Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-26T12:28:35Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-26T14:12:36Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

lukeelmers

Sorry if I jumped the gun here (I realize this is still WIP), but figured I’d add a couple comments since I was looking at it anyway.

lukeelmers · 2019-03-27T03:29:50Z

src/legacy/ui/public/visualize/loader/pipeline_helpers/build_pipeline.test.js

-          metric: [0],
-          segment: [1, 2]
+          metric: [{ accessor: 0 }],
+          segment: [{ accessor: 1 }]


We should probably add all of the new params here so that the snapshots are testing everything

i'll add another test for that, as this actually hit a bug :)

lukeelmers · 2019-03-27T03:30:41Z

src/legacy/core_plugins/interpreter/public/functions/tagcloud.js

+    bucketFormatParams: {
+      types: ['string'],
+      default: '"{}"',
+    }


The interpreter tagcloud function unit tests will probably need to be updated too

src/legacy/core_plugins/interpreter/public/functions/tagcloud.js

ppisljar · 2019-03-27T09:53:10Z

src/legacy/core_plugins/interpreter/public/functions/tagcloud.js

+      default: 18,
+    },
+    maxFontSize: {
+      types: ['number'],


should we (and how) provide minimum and maximum for this property ?

@w33ble ^^ whats the canvas way ?

Much like options, the interpreter doesn't care. Check it in the function, if the wrong value is provided, just throw with a helpful message.

ppisljar · 2019-03-27T09:53:57Z

src/legacy/core_plugins/interpreter/public/functions/tagcloud.js

-      types: ['string', 'null'],
-      default: '"{}"',
+    scale: {
+      types: ['string'],


should we provide allowed options here ? ('linear', 'log', 'sqrt')
we already have this check on the chart level

@w33ble ^^ whats the canvas way ?

Use the options argument for the function definition, since the auto-complete dialog will show those values. Then in the function itself, just throw with a helpful message if the value is invalid. An example of this is alterColumn

elasticmachine · 2019-03-27T11:11:20Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-27T15:20:46Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

w33ble · 2019-03-27T18:17:40Z

src/legacy/core_plugins/interpreter/public/functions/tagcloud.js

+      args.metric :
+      context.columns.find(c => c.id === args.metric);
+    if (metricAccessor === undefined) {
+      throw new Error('invalid column name');


The user sees these errors, giving them a little more information would likely help them be successful. Right now they get the same message with an invalid metric or bucket, at the very least you should tell them which argument is causing the error. Telling them why it's failing (unable to find column foo, or no column at index X) would be ideal.

also these all need to be translated

w33ble · 2019-03-27T19:49:47Z

So, I suspect you're not going to want all the arguments you've added, otherwise you're going to end up with a ton of code duplication, and it's going to get really hard to use when you have more than a single metric and bucket.

Instead, I would recommend thinking about how you can compose functions and types to build what you need. As one example, and for lack of a better name, maybe a visConfig function and type, so you'd end up with something more like this:

tagcloud visConfig={visConfig metric=1 bucket=0 bucketFormat='ip'} 
  scale='linear' orientation='angled' minFontSize: 12, maxFontSize: 24

Where the visConfig function returns a type: 'visConfig' object. I realize this looks like it just adds more boilerplate, but it allows you to do all the conversion back into a visConfig object in that function, so you don't end up copying and pasting the same code all over the place. It also separates the tagcloud concerns (font sizing, scaling, orientation) from the vis data.

Using composition like that allows you to build visConfig objects in other functions as well, so when you have a more complex use case (tables come to mind), it might make sense to write a new function for that use case, but produce the same type: 'visConfig' object. Or even borrowing from the existing response handlers you have, a function for series and a function for slices... really anything you need.

Hypothetically, you could end up with stuff like this, as examples:

tagcloud visConfig={visConfig metric=1 bucket=0 bucketFormat='ip'} 
  scale='linear' orientation='angled' minFontSize: 12, maxFontSize: 24

metric visConfig={visConfig metric=0} 
  mode=percentage colors='Green to Red'

It might also make sense to go even further and create functions that can produce metric and bucket objects...

tagcloud 
  visConfig={visConfig metric={visMetric 1 format='number'} bucket={visBucket 0 format='ip'}} 
  scale='linear' orientation='angled' minFontSize: 12, maxFontSize: 24

Or even without the function nesting...

tagcloud metrics={visMetric 1 format='number'} buckets={visBucket 0 format='ip'} 
  scale='linear' orientation='angled' minFontSize: 12, maxFontSize: 24

Imagine metrics and buckets are multi-args here...

table 
  metrics={visMetric 0} metrics={visMetric date format=date} 
  buckets={visBucket ip format=ip} perPage=20

I'm not sure which option is the best, I know practically nothing about visConfig objects, so you'd have to experiment a bit to figure that out. But function composition seems like a better way to do this.

lukeelmers · 2019-03-27T23:04:41Z

I'm not sure which option is the best, I know practically nothing about visConfig objects, so you'd have to experiment a bit to figure that out. But function composition seems like a better way to do this.

I like the idea of making this more composable, since some visTypes will certainly have a lot more args than others (e.g. tile/region maps will have a ton). But I also am not sure what the best solution is as I can see pros and cons to both approaches.

For example, visConfig objects are tricky because they largely differ for each visualization, which is why I understand @ppisljar's current implementation of putting the config together inside the function that's specific to the current vis.

On the other hand, we could benefit from some sort of uniformity across visConfigs to enforce things like keeping keynames consistent between vis types. Composition can help here, as could proper (shared) typings for the visConfig object.

The one item that is somewhat consistent across all vis types is dimensions/metrics/buckets, and they are also conceptually different from most other visConfig items (which are more like "options" for the vis that a user might configure in the UI, whereas the dimensions have to do with selecting the right underlying data and aren't something a user is really going to see).

So perhaps @w33ble's idea of visMetric/visBucket functions would be a good middle ground (...or visDimensions or whatever we call them).

Not trying to spin the conversation too far outside the scope of this PR; I'm mostly just thinking through this out loud. 😉 Still not sure what the right answer is.

elasticmachine · 2019-03-28T12:19:46Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

ppisljar · 2019-03-28T12:39:47Z

lets leave this PR for now and not merge it, i will work on another function in the meanwhile.

i'll take metric as its just slightly more complicated, has few options, but allows you to specify multiple metrics. Still only a single bucket and there is no split chart option. Also all of the options are 'global' not per metric.

lets see how that goes, and then i'll give histogram a try: multiple metrics, multiple buckets, split charts and configuration on per-series level (one for each metric-bucket combination) ...

it will be then easier to figure out the best syntax.

elasticmachine · 2019-03-28T13:58:51Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…tics/tagcloud # Conflicts: # src/legacy/ui/public/visualize/loader/embedded_visualize_handler.ts

elasticmachine · 2019-04-01T16:52:13Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

lukeelmers · 2019-04-01T18:53:26Z

src/legacy/core_plugins/interpreter/public/functions/vis_dimension.js

+    format: {
+      types: ['string'],
+      default: 'string'
+    },


nit: would be good to add help text on format too

lukeelmers · 2019-04-01T18:56:02Z

@ppisljar Nice, I think this approach strikes a good balance. vis_dimension feels like a very intuitive way to separate data from UI

w33ble · 2019-04-02T19:19:58Z

src/legacy/core_plugins/interpreter/public/functions/vis_dimension.js

+  fn: (context, args) => {
+    const accessor = Number.isInteger(args.accessor) ?
+      args.accessor :
+      context.columns.find(c => c.id === args.accessor);


You're making assumptions about the type of context you're working with here. You should be explicit about only accepting a datatable type, so the interpreter can report an incorrect type, and also do automatic conversions, instead of the function just throwing.

Adding this to the function definition would be all it takes.

context: { types: ['datatable'], }

w33ble · 2019-04-02T19:30:45Z

src/legacy/core_plugins/interpreter/common/types/kibana_datatable.js

@@ -27,7 +27,12 @@ export const kibanaDatatable = () => ({
      return {
        type: 'kibana_datatable',
        rows: context.rows,
-        columns: context.columns,
+        columns: context.columns.map(column => {


Why is this change part of this PR? And also, do you still need the code on line 26?

context.columns.forEach(c => c.id = c.name);

this was broken before, kibana_datatable requires column.id

Right, but the code on line 26 appears to be attempting to do the same thing. Can that be removed?

w33ble · 2019-04-02T19:35:16Z

src/legacy/core_plugins/tagcloud/public/__snapshots__/tag_cloud_fn.test.js.snap

@@ -9,14 +9,13 @@ Object {
      "listenOnChange": true,
    },
    "visConfig": Object {
+      "bucket": undefined,


Is this safe? And is it safe for metric: undefined as well? You don't check for a value of either property before putting it on the visConfig object in tag_cloud_fn.js, so that can end up as the output.

should be safe, but i will still remove this, better if its not set at all

w33ble · 2019-04-02T19:36:19Z

src/legacy/core_plugins/tagcloud/public/tag_cloud_fn.js

+      minFontSize: args.minFontSize,
+      maxFontSize: args.maxFontSize,
+      showLabel: args.showLabel,
+      metric: args.metric,


metric and bucket both lack a default value, so they can both end up being undefined, as noted in the snapshot. I wanted to point that out since I don't know how safe that behavior is.

…tics/tagcloud

elasticmachine · 2019-04-03T08:53:13Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-03T09:21:24Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

w33ble · 2019-04-03T19:27:25Z

src/legacy/core_plugins/interpreter/public/functions/vis_dimension.js

+import { i18n } from '@kbn/i18n';
+
+export const visDimension = () => ({
+  name: 'vis_dimension',


Quick note on the function name, we've avoided using underscores and other characters in Canvas because it makes the expression much more confusing. It's true for arguments, but it's especially true for function names.

For example, if I know I need the "vis dimension" function, I could type it as visdimension or visDimension, or even ViSdImEnSiOn... it's all case-insensitive for that reason. That underscore breaks that convention, now I need to know your naming scheme.

w33ble

Still think you can remove line 26 in kibana_datatable.js now, but I don't see any reason to block the PR over it. Everything else looks good enough.

I encourage you to consider renaming the vis dimension function, and to generally avoid underscores when naming functions and arguments.

lukeelmers

This LGTM too. Thanks @w33ble for the thoughts on naming & autocomplete... hadn't considered that. Currently we're using underscores in most of our vis function names, so perhaps it's worth updating all of them at once, as this isn't the only place underscores have been used.

Still think you can remove line 26 in kibana_datatable.js now

I agree with this, looks like that line is unnecessary and should probably be deleted.

elasticmachine · 2019-04-04T10:25:27Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-04T11:20:55Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-04T12:28:46Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

ppisljar added discuss WIP Work in progress :AppArch labels Mar 25, 2019

ppisljar requested a review from lukeelmers March 25, 2019 09:10

ppisljar force-pushed the interpreter/semantics/tagcloud branch from ec3850c to ecef915 Compare March 26, 2019 11:01

ppisljar force-pushed the interpreter/semantics/tagcloud branch from ecef915 to b47107d Compare March 26, 2019 11:41

lukeelmers reviewed Mar 27, 2019

View reviewed changes

ppisljar commented Mar 27, 2019

View reviewed changes

ppisljar requested a review from w33ble March 27, 2019 10:04

w33ble suggested changes Mar 27, 2019

View reviewed changes

ppisljar added 6 commits March 28, 2019 06:12

updating tagcloud func

404a207

updating tests

16bbf1f

allowing metric and bucket as column index or column name

813e03a

adding some help

3d9e156

updating tagcloud fn test

46c4688

translating error messages

55cf736

ppisljar force-pushed the interpreter/semantics/tagcloud branch from 6fe2bdb to 55cf736 Compare March 28, 2019 13:16

updating translation ids

aa01b36

ppisljar added 2 commits April 1, 2019 09:06

adding integration tests

e0095a8

Merge remote-tracking branch 'upstream/master' into interpreter/seman…

180a399

…tics/tagcloud # Conflicts: # src/legacy/ui/public/visualize/loader/embedded_visualize_handler.ts

lukeelmers reviewed Apr 1, 2019

View reviewed changes

lukeelmers mentioned this pull request Apr 1, 2019

POC: Expression builder class. #34318

Closed

w33ble reviewed Apr 2, 2019

View reviewed changes

ppisljar added 3 commits April 3, 2019 00:49

Merge remote-tracking branch 'upstream/master' into interpreter/seman…

3364943

…tics/tagcloud

updating based on Joe's review

aefa7c9

updating integration tests

655ac12

w33ble reviewed Apr 3, 2019

View reviewed changes

w33ble approved these changes Apr 3, 2019

View reviewed changes

lukeelmers approved these changes Apr 3, 2019

View reviewed changes

ppisljar added 2 commits April 4, 2019 02:36

Merge branch 'master' into interpreter/semantics/tagcloud

e61fa04

updating based on joe's review

f00256a

renaming function

5815675

ppisljar force-pushed the interpreter/semantics/tagcloud branch from 63f6fa5 to 5815675 Compare April 4, 2019 11:45

ppisljar merged commit 954d3e1 into elastic:master Apr 4, 2019

ppisljar mentioned this pull request Apr 4, 2019

[7.x] updating tagcloud interpreter func arguments (#33773) #34536

Merged

ppisljar added a commit to ppisljar/kibana that referenced this pull request Apr 4, 2019

updating tagcloud interpreter func arguments (elastic#33773)

477ae09

ppisljar added a commit that referenced this pull request Apr 4, 2019

updating tagcloud interpreter func arguments (#33773) (#34536)

ea1c8d5

maryia-lapata mentioned this pull request May 15, 2019

[Tagcloud] Allow to send showLabel parameter with false value #36563

Merged

sulemanof mentioned this pull request Nov 23, 2020

Fix: tagcloud explicitly pass params #84107

Merged

8 tasks

stratoula mentioned this pull request Nov 24, 2020

[DataTable] Improve semantics #84209

Closed

updating tagcloud interpreter func arguments #33773

updating tagcloud interpreter func arguments #33773

Conversation

ppisljar commented Mar 25, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Mar 25, 2019

💔 Build Failed

elasticmachine commented Mar 26, 2019

💔 Build Failed

elasticmachine commented Mar 26, 2019

💔 Build Failed

elasticmachine commented Mar 26, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w33ble Mar 27, 2019 • edited Loading

Choose a reason for hiding this comment

ppisljar Mar 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w33ble Mar 27, 2019 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Mar 27, 2019

💔 Build Failed

elasticmachine commented Mar 27, 2019

💔 Build Failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w33ble commented Mar 27, 2019 • edited Loading

lukeelmers commented Mar 27, 2019

elasticmachine commented Mar 28, 2019

💔 Build Failed

ppisljar commented Mar 28, 2019

elasticmachine commented Mar 28, 2019

💔 Build Failed

elasticmachine commented Apr 1, 2019

💔 Build Failed

Choose a reason for hiding this comment

lukeelmers commented Apr 1, 2019

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

elasticmachine commented Apr 3, 2019

💔 Build Failed

elasticmachine commented Apr 3, 2019

💔 Build Failed

Choose a reason for hiding this comment

w33ble left a comment

Choose a reason for hiding this comment

lukeelmers left a comment

Choose a reason for hiding this comment

elasticmachine commented Apr 4, 2019

💔 Build Failed

elasticmachine commented Apr 4, 2019

💔 Build Failed

elasticmachine commented Apr 4, 2019

💚 Build Succeeded

ppisljar commented Mar 25, 2019 •

edited

Loading

w33ble Mar 27, 2019 •

edited

Loading

ppisljar Mar 27, 2019 •

edited

Loading

w33ble Mar 27, 2019 •

edited

Loading

w33ble commented Mar 27, 2019 •

edited

Loading