[Lens] Allow non-numeric metrics for metric vis #169258

mbondyra · 2023-10-18T14:59:13Z

Summary

Fixes #137756
Allows to set non numeric metrics for metric visualization for primary and secondary metric.

Doesn't include coloring by terms.
When primary metric is non-numeric:

when maximum value is empty, we hide maximum value group
when maximum value has a value we set an error message on dimension
we don’t allow to use a palette for coloring
we don’t allow to set a trendline

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
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2023-10-19T14:25:58Z

Pinging @elastic/kibana-visualizations (Team:Visualizations)

stratoula

This looks great! I found some small bugs:

When the collapse by is on then I see the coloring options in the metric dimension for non numeric fields

I have a configuration like this

I change the metric to last value of @timestamp and my chart is colored blue for some reason, shouldn't this default to the neutral color?

If I remove the dimension it works fine. Also with trendlines it works as expected. The problem is when I have the bars set.

dej611 · 2023-10-23T08:20:41Z

Maybe this does not strictly belong here, but I would like to raise some awareness about long text fields for Metric:

While the secondary metric can go multiline, the primary metric has a no-wrap policy for the CSS white-space property.
CC @gvnmagni

I've tried to quickly check what would happen without that nowrap thing but it introduces other issues:

gvnmagni · 2023-10-23T08:24:11Z

Thank you @dej611, given the nature of the secondary metric it was easier to expect that it could go on multiple lines, it is also way smaller and then way more easy to deal with. At the same time this is an interesting conversation to have because introducing text might make worth to talk about multi-lines for values/strings as well. Thank you for pointing this out!

mbondyra · 2023-10-23T10:06:43Z

@dej611 @gvnmagni I'll create an issue about the pointed out problem, we should definitely find a solution but I think it's out of scope for this PR.

drewdaemon

Nice! Just did a code review.

x-pack/plugins/lens/public/visualizations/metric/dimension_editor.test.tsx

src/plugins/chart_expressions/expression_metric/public/components/metric_vis.tsx

x-pack/plugins/lens/public/visualizations/metric/visualization.test.ts

drewdaemon · 2023-10-23T17:28:23Z

If I remove the dimension it works fine. Also with trendlines it works as expected. The problem is when I have the bars set.

Yeah, IIRC when no max is provided, elastic-charts interprets the color property as the background color instead of the bar color.

mbondyra · 2023-10-24T12:40:28Z

When the collapse by is on then I see the coloring options in the metric dimension for non numeric fields

@stratoula this is an extreme edgecase and also not an easy problem to fix (we already have it in datatable). I could create a bug or we could also just ignore it – in the end, seeing these options don't really break anything.

stratoula · 2023-10-24T13:21:41Z

Well this doesn't look good

in datatable also doesn't make sense but feels a bit better

I agree that is an edge case but can you explain why is it difficult to hide this in that case?

stratoula · 2023-10-24T14:04:26Z

I see that the problem is that the last metric column reports that is a number but is wrong. Can we fix it? If we fix it on the aggs level then it will be solved in both places

gchaps · 2023-10-24T15:51:21Z

x-pack/plugins/lens/public/visualizations/metric/visualization.tsx

+          fixableInEditor: true,
+          displayLocations: [{ id: 'dimensionButton', dimensionId: state.maxAccessor }],
+          shortMessage: i18n.translate('xpack.lens.lnsMetric_maxDimensionPanel.nonNumericError', {
+            defaultMessage: 'Maximum value cannot be defined for non-numeric primary metric.',


How about making this text more positive.

Primary metric must be numeric to set a maximum value.

stratoula · 2023-10-25T05:56:28Z

I see that the problem is that the last metric column reports that is a number but is wrong. Can we fix it? If we fix it on the aggs level then it will be solved in both places

If the fix on the aggs level is difficult you can do it here possibly (I dont think that this will cause any bugs tbh) x-pack/plugins/lens/common/expressions/datatable/utils.ts

We are already doing this for min and max. You can add a check for last value too. If you check the column.meta.params.id the value is string (not numeric) which is the correct type. So you could extend it possibly for other types.

// min and max aggs are reporting as number but are actually dates - work around this by checking for the date formatter until this is fixed at the source

const isNumeric = column?.meta.type === 'number' && column?.meta.params?.id !== 'date';

mbondyra · 2023-10-26T12:59:38Z

@stratoula all the bugs were fixed (by rebasing off the aggs PR)
@gchaps copy replaced with your suggestion, thank you!
@drewdaemon I will start working on replacing the test file with RTL, but I will submit it in the separate PR to not make this one bulky and dependant on my speed.

Please recheck 🙏🏼 😊

@stratoula

…metrics (#169834) ## Summary When checking my PR here #169258 @stratoula noticed that the `column.meta.type` is not set properly for last value aggregation (it always defaults to 'number', same with all the filtered metrics too!). When I dug deeper, I noticed that happens because we calculate it as: ``` type: column.aggConfig.type.valueType column.aggConfig.params.field?.type || 'number', ``` and some of the `AggConfigs` don't have the static `valueType` property nor field, specifically the one with nested aggregations, like top_hits, top_metrics and filtered_metric. instead of a static `valueType`, I've decided to change it to a method `getValueType` where we can pass AggConfigs and get the type from different places internally. This way `top_hits`, `top_metrics` and `filtered_metric` get the type of the field from `customMetric`. I also changed the values for `min` and `max` aggregation - they were set on `number`, but they can also be a `date`.

gchaps · 2023-10-26T18:35:53Z

The text re: the primary metric LGTM.

What does this message mean: At least one color range contains the wrong value or color

mbondyra · 2023-10-27T07:26:16Z

@gchaps this doesn't come from this PR, but I am happy to revisit. This error appears when the input value for the color ranges is not correct (see the screenshot attached to Stratoula's message)

stratoula · 2023-10-27T10:19:09Z

Ok we are very close!

If you set a collapse by on a numerc field and then change the metric to last value of a keyword the color remains. Let's default to the default one

mbondyra · 2023-10-27T19:36:45Z

Fixed!

kibana-ci · 2023-10-27T21:03:42Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: d06095e

Failed CI Steps

Defend Workflows Cypress Tests on Serverless #1

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`expressionMetricVis`	4.6KB	4.9KB	+283.0B
`lens`	1.4MB	1.4MB	+1.2KB
total			+1.5KB

History

💚 Build #171225 succeeded 088ff3ecbe66fa362d6f6212950c21d785616391
💔 Build #171182 failed 8b4738eb01bea12b5f77b02e1b2dc2f250014271
💔 Build #170366 failed 5690aaaf99f6a27bff24d46990e5091a26d3b778
💛 Build #169798 was flaky baad6067678598b8d05decc18b7c903f2714be07
💛 Build #169651 was flaky 0e495acd2ddd40b6ef9cb9164e5e01400f4e8d61
💔 Build #169490 failed 7c86ba35bdfafcc271489308fbe6b484e5c3b755

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

drewdaemon

Tested the broken color cases! Thank you!

mbondyra added release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens v8.12.0 labels Oct 18, 2023

mbondyra force-pushed the lens/non-numerical-metrics branch 6 times, most recently from 5b49775 to 4145f6b Compare October 19, 2023 14:08

mbondyra marked this pull request as ready for review October 19, 2023 14:25

mbondyra requested a review from a team as a code owner October 19, 2023 14:25

stratoula reviewed Oct 23, 2023

View reviewed changes

This was referenced Oct 23, 2023

[Lens] Medium and long text in metric is cut #169505

Closed

[metric] Medium and long text in metric is cut elastic/elastic-charts#2213

Closed

drewdaemon added the ui-copy Review of UI copy with docs team is recommended label Oct 23, 2023

drewdaemon reviewed Oct 23, 2023

View reviewed changes

gchaps reviewed Oct 24, 2023

View reviewed changes

mbondyra mentioned this pull request Oct 25, 2023

[Aggs] Fix column.meta.type for top hit, top metric and all filtered metrics #169834

Merged

mbondyra force-pushed the lens/non-numerical-metrics branch from 5690aaa to f7f939f Compare October 26, 2023 12:49

mbondyra requested a review from a team as a code owner October 26, 2023 12:49

mbondyra force-pushed the lens/non-numerical-metrics branch from e149826 to 088ff3e Compare October 26, 2023 14:21

mbondyra removed the request for review from a team October 26, 2023 14:22

mbondyra added 4 commits October 27, 2023 16:14

[Lens] Allow non-numeric metrics for metric vis

1cb1fed

fix copy

6e5540e

fix tests

3e651fc

fix the palette bug

d06095e

mbondyra force-pushed the lens/non-numerical-metrics branch from 088ff3e to d06095e Compare October 27, 2023 19:32

mbondyra force-pushed the lens/non-numerical-metrics branch from 43a43db to d06095e Compare October 27, 2023 19:43

drewdaemon approved these changes Oct 30, 2023

View reviewed changes

mbondyra merged commit c7e7853 into elastic:main Oct 30, 2023

kibanamachine added the backport:skip This commit does not require backporting label Oct 30, 2023

mbondyra deleted the lens/non-numerical-metrics branch October 30, 2023 19:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Allow non-numeric metrics for metric vis #169258

[Lens] Allow non-numeric metrics for metric vis #169258

mbondyra commented Oct 18, 2023 •

edited

Loading

elasticmachine commented Oct 19, 2023

stratoula left a comment

dej611 commented Oct 23, 2023

gvnmagni commented Oct 23, 2023

mbondyra commented Oct 23, 2023

drewdaemon left a comment

drewdaemon commented Oct 23, 2023

mbondyra commented Oct 24, 2023

stratoula commented Oct 24, 2023

stratoula commented Oct 24, 2023

gchaps Oct 24, 2023

stratoula commented Oct 25, 2023

mbondyra commented Oct 26, 2023 •

edited

Loading

gchaps commented Oct 26, 2023

mbondyra commented Oct 27, 2023

stratoula commented Oct 27, 2023

mbondyra commented Oct 27, 2023

kibana-ci commented Oct 27, 2023

drewdaemon left a comment

[Lens] Allow non-numeric metrics for metric vis #169258

[Lens] Allow non-numeric metrics for metric vis #169258

Conversation

mbondyra commented Oct 18, 2023 • edited Loading

Summary

Checklist

Risk Matrix

For maintainers

elasticmachine commented Oct 19, 2023

stratoula left a comment

Choose a reason for hiding this comment

dej611 commented Oct 23, 2023

gvnmagni commented Oct 23, 2023

mbondyra commented Oct 23, 2023

drewdaemon left a comment

Choose a reason for hiding this comment

drewdaemon commented Oct 23, 2023

mbondyra commented Oct 24, 2023

stratoula commented Oct 24, 2023

stratoula commented Oct 24, 2023

gchaps Oct 24, 2023

Choose a reason for hiding this comment

stratoula commented Oct 25, 2023

mbondyra commented Oct 26, 2023 • edited Loading

gchaps commented Oct 26, 2023

mbondyra commented Oct 27, 2023

stratoula commented Oct 27, 2023

mbondyra commented Oct 27, 2023

kibana-ci commented Oct 27, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Async chunks

History

drewdaemon left a comment

Choose a reason for hiding this comment

mbondyra commented Oct 18, 2023 •

edited

Loading

mbondyra commented Oct 26, 2023 •

edited

Loading