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] Support local variables in formula #153381

Closed
Tracked by #182774
drewdaemon opened this issue Mar 21, 2023 · 7 comments
Closed
Tracked by #182774

[Lens] Support local variables in formula #153381

drewdaemon opened this issue Mar 21, 2023 · 7 comments
Labels
Feature:Lens impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@drewdaemon
Copy link
Contributor

drewdaemon commented Mar 21, 2023

#153381 (comment)

Bears technical similarities to #135265

@drewdaemon drewdaemon added Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. labels Mar 21, 2023
@drewdaemon
Copy link
Contributor Author

@P1llus can you give us a couple of examples of Painless scripts you've had to rewrite as Lens formulas (or weren't able to rewrite as formulas) and describe the performance concerns?

@drewdaemon drewdaemon changed the title [Lens] support painless [Lens] support painless in formula Mar 21, 2023
@P1llus
Copy link
Member

P1llus commented Mar 23, 2023

While I believe painless support was out of the question for now, let's look at some basic TSVB implementations that will either result in very big formulas, or currently not possible:

  1. Anything using _interval context in painless to for example divide a value by the bucket time range size, there is already a separate tracking issue for this.

  2. TSVB often have a combination of max, derivative etc stored as variables first, which is then accessed with painless. This is very useful and makes the overview much clearer.
    Since variables of any sort is not possible currently with formula we end up deeply nesting logic inside eachother, and I am unsure how this impacts performance.
    Could be maybe add a row of functions above the formula, that does not end up in the resulting visualizations, but are only accessible as variables in the formula itself? Example:
    image

  3. In the same example image above, this painless will most likely end up being quite nested, might still be possible, but will it even be readable? Or maintainable?
    Since we don't have variables, if you look at the screenshot + this painless part:

      if (params.deltaUsageDerivNormalizedValue > 0 && params.periodsDerivNormalizedValue >0  && params.quota > 0) {
        // if throttling is configured
        double  factor = params.deltaUsageDerivNormalizedValue / (params.periodsDerivNormalizedValue * params.quota * 1000); 

        return factor * 100; 
      }

      return null;

Since we don't have variables, and since we cannot simply stop/return on null, but rather use ifelse formulas, which again then ends up being another 3 deeper nested formulas, this will end up being huge.

@P1llus
Copy link
Member

P1llus commented Mar 23, 2023

Talking about variables, if the UI and adding functions above the formula UI would be too much, we should be able to write multiple formulas in a single formula, storing them as temp variables. Using one of the steps on the picture above as an example:

deltaUsage = derivative(max(system.process.cgroup.somevalue))

Then later, we can simply create the formula for the painless, using deltaUsage inside it, making the formula much cleaner to read, which also helps making it prettier, ref: #153387

@dej611
Copy link
Contributor

dej611 commented Mar 24, 2023

To me the main problem here sounds like the lack of variables support in Formula. Maybe we can rename the issue?

The formula above can be already written in formula, but I understand it is a bit verbose to do so:

ifelse(
  derivative(max(system.process.cgroup.somevalue)) +
  derivative(max(system.process.cgroup.someOthervalue)) +
  min(system.process.cgroup.somevalue) > 0,

  100 * derivative(max(system.process.cgroup.somevalue)) / 
  ( derivative(max(system.process.cgroup.someOthervalue)) * min(system.process.cgroup.somevalue) * 1000),

  0
)

With variables support it might be a bit easier to read, but probably it would require quite a bit work on the tinymath side of formula:

deltaUsageDerivNormalizedValue := derivative(max(system.process.cgroup.somevalue));
periodsDerivNormalizedValue := derivative(max(system.process.cgroup.someOthervalue));
quota := min(system.process.cgroup.somevalue);

ifelse(
  deltaUsageDerivNormalizedValue + periodsDerivNormalizedValue + quota > 0,
  100 * deltaUsageDerivNormalizedValue / (periodsDerivNormalizedValue * quota * 1000),
  0
)

What do you think?

@timductive timductive added impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. and removed impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. labels Apr 5, 2023
@drewdaemon drewdaemon changed the title [Lens] support painless in formula [Lens] bring the power of painless language to Lens Apr 6, 2023
@drewdaemon
Copy link
Contributor Author

@P1llus do you agree that the main ask from your side is variables in formula?

@P1llus
Copy link
Member

P1llus commented Aug 4, 2023

@drewdaemon I think being able to use local variables would remove the necessary nesting quite alot and make it easier to read :)

@drewdaemon drewdaemon changed the title [Lens] bring the power of painless language to Lens [Lens] Support local variables in formula Aug 4, 2023
@markov00
Copy link
Member

markov00 commented May 7, 2024

Closing this because it's not planned to be resolved in the foreseeable future. It will be tracked in our Icebox and will be re-opened if our priorities change. Feel free to re-open if you think it should be melted sooner.

@markov00 markov00 closed this as not planned Won't fix, can't repro, duplicate, stale May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Lens impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

No branches or pull requests

5 participants