-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Chart expressions] new metric vis expression #135461
[Chart expressions] new metric vis expression #135461
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app services code LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works pretty well already, thanks a lot! Left some comments in the code.
In general a single metric seems to render a little weird (why is there a black bar on the top?)
Is this style intended?
kibana
| esaggs index={indexPatternLoad id="ff959d40-b880-11e8-a6d9-e546fe2bba5f"}
aggs={aggTerms id="0" enabled=true schema="segment" field="day_of_week" orderBy="1" order="desc" size=6 otherBucket=false otherBucketLabel="Other" missingBucket=false missingBucketLabel="(missing value)"}
aggs={aggSum id="1" enabled=true schema="metric" field="products.base_price"} metricsAtAllLevels=false partialRows=false
aggs={aggMedian id="2" enabled=true schema="metric" field="products.base_unit_price"} metricsAtAllLevels=false partialRows=false
| metricVis metric="col-1-1" progressMin=0 progressDirection="horizontal" extraText="fswefsdf" progressMax=400000 maxCols=5 palette={palette color="purple" color="orange" stop=26 stop=28 reverse=false range="number" rangeMax=30 rangeMin=0}
...ugins/chart_expressions/expression_metric/common/expression_functions/metric_vis_function.ts
Show resolved
Hide resolved
src/plugins/chart_expressions/expression_metric/public/components/metric_vis.tsx
Show resolved
Hide resolved
src/plugins/chart_expressions/expression_metric/public/expression_renderers/metric_vis.scss
Outdated
Show resolved
Hide resolved
src/plugins/chart_expressions/expression_metric/public/components/metric_vis.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great to see this working in Kibana. It looks great!
I've added a few comments, nothing that blocks the PR from being merged, just general comments on the code and behaviours.
We should chat about the rendering in a dashboard panel and within Lens editor because we want to remove paddings and we don't want to limit the size of metric when rendering it within lens
...ugins/chart_expressions/expression_metric/common/expression_functions/metric_vis_function.ts
Outdated
Show resolved
Hide resolved
...ugins/chart_expressions/expression_metric/common/expression_functions/metric_vis_function.ts
Outdated
Show resolved
Hide resolved
types: ['boolean'], | ||
help: i18n.translate('expressionMetricVis.function.autoScale.help', { | ||
defaultMessage: 'Enable auto scale', | ||
minTiles: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we work by columns and rows instead of maxCols
and minTiles
?
The selection of columns x rows is a natural configuration in most existing tools (see powerpoint). IMHO this express a more clear definition of a grid, where maxCols and minTiles has some intrinsic behaviors that are not immediate to understand.
From what I've understood, currently works like, along those lines:
numberOfTiles = Math.max(minTiles, theNumberOfMetricsToDisplay) // this could represent both empty or filled tiles, but is not the final number of cells in a grid, because it depends also on the maxCols
numberOfColumns = Math.min(maxCols, theNumberOfMetricsToDisplay, minTiles)
numbefOfRows = Math.ceil(numberOfTiles / numberOfColumns)
using two explicit cols and rows values you got it as simple as it is: the number of cols is exactly the one configured, the number of rows is exactly the one specified. The grid has rows x columns tiles.
If the number of metric exceed the available space it is truncated, if is less, then the grid is still rendered entirely.
Adding auto
to the row
or to the column can help simplify the definition of a single horizontal line or a single vertical one like: cols: 'auto' rows: 1
or cols:1 rows:'auto'
It could be auto, or, to avoid a union type you can also use just the Infinity
number.
You can add that with the condition that you can't specify Infinity
in bot columns and rows.
You could also specify the wrapping method of that grid like
left->right->bottom>left->right
as in a LTR writing systemleft-right->bottom->right->left
as in a zigzag patterntop->bottom->left->top->bottom
vertical zigzag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice explanation. I'm of two minds on this one.
What you're saying makes sense in the grid paradigm, but there are other precedents out there (for example, inline HTML elements which are laid out in wrapping rows given a width constraint).
I chose the minTiles
argument because we plan to have the Lens datasource predict a maximum number of rows in the data table. For example, Top 5 Values will never have more than 5 datatable rows. So, it's nice to be able to just pass that number down to the expression to make sure the visualization keeps its shape even if there are fewer than 5 metrics to display given a different time range or something.
But, I guess it isn't a bad thing to have Lens perform a simple calculation to come to the correct number of metric vis rows.
Thinking about it more, it might be nice to expose rows/columns as settings to the user in Lens editor at some point. If that's so, your model makes most sense.
Any thoughts @flash1293 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong opinion on this as as Andrew mentioned it won't be relevant for the Lens case, but I can see the point. @andrewctate if you think it would make sense to tackle this right now then go for it, but I'm also happy with pushing out API polishing work like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markov00 another question on this...
What would be the expected behavior if there are 5 metrics and user specifies 3 columns and 1 row?
src/plugins/chart_expressions/expression_metric/public/components/metric_vis.tsx
Show resolved
Hide resolved
src/plugins/chart_expressions/expression_metric/public/components/metric_vis.tsx
Outdated
Show resolved
Hide resolved
src/plugins/chart_expressions/expression_metric/public/components/metric_vis.tsx
Show resolved
Hide resolved
Strangely, I can't reproduce. |
ML changes LGTM 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style and class name changes look fine from my perspective.
@elasticmachine merge upstream |
…ctate/kibana into 134242/new-metrics-vis-expression
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good overall! I'm wondering how we are going to handle the size constraint (never rendering a metric larger than x pixels). There are two general ways I guess:
- Inside the expression renderer (probably behind some additional argument which is always set by the Lens vis)
- Outside of the expression renderer as part of the workspace panel / embeddable code
No strong opinion, we need custom styles outside of the expression renderer anyway to achieve the desired padding less dashboard styles so we could handle this in the same go.
Approving as this can also be handled on a separate PR.
@elasticmachine merge upstream |
…ion_functions/metric_vis_function.ts Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com>
💛 Build succeeded, but was flakyFailed CI StepsMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Canvas Sharable Runtime
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested this out locally and everything looks great in Canvas!
Code review, looks like mostly just renaming of the two metric vis for clarity!
Had a short discussion with Andrew for my own understanding, which can be summed up as follows:
No migration is required for existing usages of metricVis
to be renamed to legacyMetricVis
because the metricVis
expression function is not available yet via the menus, and will not be until #134841 is complete.
As part of making the unified renderers available in Canvas, we should likely skip over the legacy metric vis, and make only the new metric vis introduced in this PR available.
The expression should be used for testing is this. progressMax="col-1-1" had to be changed to max.
|
Summary
Prepares for #134242
Introduces a new metric vis chart expression function + renderer that uses the new @elastic/elastic-charts metric. Also, renames the existing metric vis to reflect its legacy status.
In other words,
src/plugins/chart_expressions/expression_metric
is now the new function and the existing one has moved tosrc/plugins/chart_expressions/expression_legacy_metric
. Unfortunately, this makes the file count high and diff a bit hard-to-follow. The meat of the new logic is actually mostly in this one file.Testing
Manual testing can be performed on a Canvas workpad.
To get started,
kibana_sample_data_ecommerce
products.base_price
andproducts.min_price
in the data viewFormatting is tied to the field formatters, so you can try out currency, bytes, percentage, duration, or just vanilla numbers. Just keep in mind, that this vis doesn't respect user-defined formatting patterns.
You can see more about expected formatting behavior in the unit test and in the original issue.
Screenshots
Checklist
Delete any items that are not applicable to this PR.