[Expression] Cached expression can keep their own side effects by dej611 · Pull Request #216519 · elastic/kibana

dej611 · 2025-03-31T16:48:58Z

Summary

Fixes the #207204

This PR introduces a new complementary function for an Expression definition named sideEffects, this goes together with the other fn function and it is used to restore any side effect when the caching system kicks in.

I haven't found how to programmatically test this.
Will add an FTR if it can be reliable to reproduce an expression caching scenario.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

Release notes

The request inspector now shows the correct request and response in any successful scenario.

dej611 · 2025-04-02T08:24:57Z

src/platform/plugins/shared/data/common/search/aggs/agg_configs.ts

    }
-    const transformedRawResponse = cloneDeep(response.rawResponse);
-    if (!transformedRawResponse.aggregations) {
+    let transformedRawResponse = response.rawResponse;


Just a tiny perf improvement

dej611 · 2025-04-03T08:52:19Z

src/platform/plugins/shared/data/public/search/expressions/esaggs.ts

    ) {
      return defer(async () => {
-        const { aggs, indexPatterns, searchSource, getNow } = await getStartDependencies();
+        const [{ aggs, indexPatterns, searchSource, getNow }, { handleEsaggsRequest }] =


Another tiny perf improvement

elasticmachine · 2025-04-03T14:21:56Z

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

jbudz

packages/kbn-optimizer/limits.yml

nreese · 2025-04-04T14:38:14Z

src/platform/plugins/shared/inspector/common/adapters/request/request_adapter.ts

+    this.responses = responses;
  }

  public reset(): void {


Should reset also reset this.responses?

Due to the WeakMap nature the responses should be empty as soon as the requests get cleared.

nreese · 2025-04-04T14:39:46Z

src/platform/plugins/shared/inspector/common/adapters/request/request_adapter.ts

  constructor() {
    super();
    this.requests = new Map();
+    this.responses = new WeakMap();


Why is responses a WeakMap instead of just a map keyed to the request id?

I thought about that, but:

there's no need to list the entries

using the request itself saves a map cycle altogether

it could reset as side effect automatically

Given the use cases it makes no specific difference between the two, other than no providing the iterator API.

thanks for the explanation

nreese

kibana-presentation changes LGTM
code review only

lukasolson

Tested and the behavior seems to be fixed (for Lens visualizations using classic mode).

The bug still happens for ES|QL visualizations on a dashboard using inline editing, so we will need to make the same changes to the esql function.

I wanted to ask a couple of quick questions regarding the approach:

Could this be fixed by moving the this.context.inspectorAdapters.requests?.reset(); call into the esaggs function (prior to making the search request)? It seems odd we're clearing the inspector adapters in the executor but then utilizing them inside the functions themselves.
If we decide this approach is best, can we somehow tie these sideEffects into the same place we register the allowCache: true so that it's clear they're related? Right now it's not clear if you support caching that you have to also worry about side effects.

dej611 · 2025-04-09T09:10:38Z

Tested and the behavior seems to be fixed (for Lens visualizations using classic mode).

The bug still happens for ES|QL visualizations on a dashboard using inline editing, so we will need to make the same changes to the esql function.

Great catch! On it...

I wanted to ask a couple of quick questions regarding the approach:

Could this be fixed by moving the this.context.inspectorAdapters.requests?.reset(); call into the esaggs function (prior to making the search request)? It seems odd we're clearing the inspector adapters in the executor but then utilizing them inside the functions themselves.

If we decide this approach is best, can we somehow tie these sideEffects into the same place we register the allowCache: true so that it's clear they're related? Right now it's not clear if you support caching that you have to also worry about side effects.

Makes sense. I'll try 1) or improve the current code with 2) if it doesn't work.

kibanamachine · 2025-04-11T12:51:04Z

Starting backport for target branches: 8.16, 8.17, 8.18, 8.x, 9.0

https://github.com/elastic/kibana/actions/runs/14403537896

kibanamachine · 2025-04-11T13:02:09Z

💔 All backports failed

Status	Branch	Result
❌	8.16	Backport failed because of merge conflicts
❌	8.17	Backport failed because of merge conflicts
❌	8.18	Backport failed because of merge conflicts
❌	8.x	Backport failed because of merge conflicts
❌	9.0	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 216519

Questions ?

Please refer to the Backport tool documentation

…ic#216519) ## Summary Fixes the elastic#207204 This PR introduces a new complementary function for an Expression definition named `sideEffects`, this goes together with the other `fn` function and it is used to restore any side effect when the caching system kicks in. ![side_effects_cache](https://github.com/user-attachments/assets/74b1ddff-a45c-4983-ac09-57559155fba8) I haven't found how to programmatically test this. Will add an FTR if it can be reliable to reproduce an expression caching scenario. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### Release notes The request inspector now shows the correct request and response in any successful scenario.

kibanamachine · 2025-04-15T13:48:44Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 216519 locally

…ic#216519) ## Summary Fixes the elastic#207204 This PR introduces a new complementary function for an Expression definition named `sideEffects`, this goes together with the other `fn` function and it is used to restore any side effect when the caching system kicks in. ![side_effects_cache](https://github.com/user-attachments/assets/74b1ddff-a45c-4983-ac09-57559155fba8) I haven't found how to programmatically test this. Will add an FTR if it can be reliable to reproduce an expression caching scenario. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### Release notes The request inspector now shows the correct request and response in any successful scenario. (cherry picked from commit 6984530) # Conflicts: # packages/kbn-optimizer/limits.yml

dej611 · 2025-04-16T12:13:01Z

💚 All backports created successfully

Status	Branch	Result
✅	9.0
✅	8.x
✅	8.18

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…ic#216519) ## Summary Fixes the elastic#207204 This PR introduces a new complementary function for an Expression definition named `sideEffects`, this goes together with the other `fn` function and it is used to restore any side effect when the caching system kicks in. ![side_effects_cache](https://github.com/user-attachments/assets/74b1ddff-a45c-4983-ac09-57559155fba8) I haven't found how to programmatically test this. Will add an FTR if it can be reliable to reproduce an expression caching scenario. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### Release notes The request inspector now shows the correct request and response in any successful scenario. (cherry picked from commit 6984530) # Conflicts: # packages/kbn-optimizer/limits.yml

kibanamachine · 2025-04-16T14:50:47Z