Skip to content
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

[Lens] formula - clamp function min/max can be optional #125930

Closed
Tracked by #57708
ghudgins opened this issue Feb 17, 2022 · 8 comments
Closed
Tracked by #57708

[Lens] formula - clamp function min/max can be optional #125930

ghudgins opened this issue Feb 17, 2022 · 8 comments
Labels
enhancement New value added to drive a business result Feature:Lens Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@ghudgins
Copy link
Contributor

ghudgins commented Feb 17, 2022

Describe the feature: Make the lens formula clamp not require a min or max value.

Describe a specific use case for the feature:
When I have data that sometimes could generate negative values
And I don't want negative values
I need to use the clamp function
And I don't want to specify an arbitrarily high value...I want to not include a max value (like Canvas)
So I can get the visualization I want without having to use random constants

Note: if we go forward with this enhancement remember to update in app product documentation as that currently shows all fields required

Related discuss post

@ghudgins ghudgins added enhancement New value added to drive a business result Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens labels Feb 17, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

@ghudgins ghudgins changed the title [Lens] formula - clamp function max can be optional [Lens] formula - clamp function min/max can be optional Feb 17, 2022
@stratoula
Copy link
Contributor

This could simplify TSVB to Lens implementation too for 'positive_only' Here https://github.com/elastic/kibana/blob/main/src/plugins/vis_types/timeseries/public/trigger_action/metrics_helpers.ts#L207
and here https://github.com/elastic/kibana/blob/main/src/plugins/vis_types/timeseries/public/trigger_action/metrics_helpers.ts#L216
I am setting as max the metric value. It can be omitted with this enhancement.

@dej611
Copy link
Contributor

dej611 commented May 18, 2022

I've digged a bit into this issue and found out some interesting insights:

  • the clamp function in Canvas does not work when both min and max arguments are missing, despite the documentation saying otherwise

Screenshot 2022-05-18 at 15 34 23

  • in the tinymath clamp jsdoc documentation two more errors are described, which are somehow ignored by the Canvas tinymath documentation page.

    • I think the Canvas Tinymath documentation should be fixed to correctly reflect the clamp implemented behaviour.
  • Omitting the max argument (i.e. clamp(myMetric, myOtherMetric)), makes the clamp function equivalent to the min function: min(myMetric, myOtherMetric)

  • Omitting both the min and max argument (i.e. clamp(5)) makes it equivalent to the value itself

Perhaps the user experience can be improved by exposing some guidance to use the min function?

@flash1293
Copy link
Contributor

@dej611 Unfortunately we can't use min or max because these names are taken by the Elasticsearch aggregations already. We could introduce clamp_min(metric, min) which behaves like the max tinymath function, what do you think?

@dej611
Copy link
Contributor

dej611 commented May 18, 2022

It works.
Otherwise why not introduce a math_min/math_max functions?

@dej611
Copy link
Contributor

dej611 commented May 18, 2022

I've put together a quick PoC with the following:

  • Two new pick_min/_max operations that wraps the tinymath min/max functions. In the PoC they are just limited to 2 args for now.
  • When using clamp with partial arguments, the correct pick_* operation is suggested in the error message

PoC: #132449

What do you think @flash1293 ?

@flash1293
Copy link
Contributor

I like that approach 👍

@dej611
Copy link
Contributor

dej611 commented Jun 1, 2022

Fixed by #132449

@dej611 dej611 closed this as completed Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result Feature:Lens Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

No branches or pull requests

5 participants