Skip to content

[Lens][ES|QL] Do not rerun the hook in case of an error in the query#225067

Merged
stratoula merged 8 commits intoelastic:mainfrom
stratoula:lens-do-not-run-in-errors
Jun 26, 2025
Merged

[Lens][ES|QL] Do not rerun the hook in case of an error in the query#225067
stratoula merged 8 commits intoelastic:mainfrom
stratoula:lens-do-not-run-in-errors

Conversation

@stratoula
Copy link
Contributor

@stratoula stratoula commented Jun 24, 2025 •
edited by kibanamachine
Loading

Summary

While testing the ES|QL charts I realized that in case of an error in the query, the hook goes into a loop and causes performance issues.

As the error is being reported we do not need to re-run the query to get the results

For example if you create a control wrongly.

e.g.

  1. Create a chart and add a control which will create an error:
image
  1. Add to the control no-date fields. e.g. clientip
  2. Check the editor is not going into a rendering loop
image

Release notes

Fixes a performance issue in the Lens ES|QL charts in case of errors in the query.

Checklist
@stratoula stratoula added Feature:Lens Feature:ES|QL ES|QL related features in Kibana v9.1.0 v8.19.0 release_note:fix backport:version Backport to applied version labels labels Jun 25, 2025
@stratoula stratoula marked this pull request as ready for review June 25, 2025 08:52
@stratoula stratoula requested a review from a team as a code owner June 25, 2025 08:52
dej611
dej611 reviewed Jun 25, 2025
View reviewed changes
...ns/shared/lens/public/editor_frame_service/editor_frame/config_panel/use_initialize_chart.ts Outdated
});

initializeChartFunc(abortController);
}, [args]);
Copy link
Contributor

@dej611 dej611 Jun 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A new object is passed every time, triggering this useEffect all the times.
Also the object is shallow copied at every execution.

I think we should do something better here and avoid useless executions.
Copy link
Contributor Author

@stratoula stratoula Jun 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the check is not allowing to set the states or execute the query , what do you have in mind?
Copy link
Contributor Author

@stratoula stratoula Jun 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a different solution in the beginning 166f263 but it wasn't so easy to test and I am not sure if relying in the errors is the best solution here.

I fixed the shallow copy here and memoized (and reused the runQuery callback) eb42ecd

Are you ok with it?
dej611
dej611 approved these changes Jun 26, 2025
View reviewed changes
Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally and it fixes the issue ���
Left a minor code comment, but will approve to unblock it.
...ns/shared/lens/public/editor_frame_service/editor_frame/config_panel/use_initialize_chart.ts Outdated
]
);
useEffect(() => {
const abortController = new AbortController();
Copy link
Contributor

@dej611 dej611 Jun 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this can be moved inside the initializeChartFunc content? I do not see any other context where it will be passed elsewhere.
Copy link
Contributor Author

@stratoula stratoula Jun 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely, done here d2170af
@stratoula stratoula enabled auto-merge (squash) June 26, 2025 13:09
@stratoula stratoula merged commit 731ab84 into elastic:main Jun 26, 2025
10 checks passed
@kibanamachine
Copy link
Contributor

kibanamachine commented Jun 26, 2025

Starting backport for target branches: 8.19

https://github.com/elastic/kibana/actions/runs/15905316060
@elasticmachine
Copy link
Contributor

elasticmachine commented Jun 26, 2025

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #129 / Serverless Observability - Deployment-agnostic Synthetics API integration tests SyntheticsAPITests EnableDefaultAlerting deletes (and recreates) the default rule when settings are updated

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 1445 1446 +1

Async chunks

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

id before after diff
lens 1.5MB 1.5MB +642.0B

History
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jun 26, 2025
@stratoula
[Lens][ES|QL] Do not rerun the hook in case of an error in the query (e…
e424078 
…lastic#225067)

## Summary

While testing the ES|QL charts I realized that in case of an error in
the query, the hook goes into a loop and causes performance issues.

As the error is being reported we do not need to re-run the query to get
the results

For example if you create a control wrongly.

e.g.

1. Create a chart and add a control which will create an error:

<img width="508" alt="image"
src="https://github.com/user-attachments/assets/f2013d2c-e161-47bf-a3cb-d5033be9de59"
/>

2. Add to the control no-date fields. e.g. clientip
3. Check the editor is not going into a rendering loop

<img width="482" alt="image"
src="https://github.com/user-attachments/assets/cc541b68-b317-41ae-b4a6-87569466edd6"
/>

### Release notes
Fixes a performance issue in the Lens ES|QL charts in case of errors in
the query.

### Checklist

- [ ] [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

(cherry picked from commit 731ab84)
@kibanamachine kibanamachine mentioned this pull request Jun 26, 2025
Merged
@kibanamachine
Copy link
Contributor

kibanamachine commented Jun 26, 2025

💚 All backports created successfully

Status Branch Result
✅ 8.19

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

Questions ?

Please refer to the Backport tool documentation
kibanamachine added a commit that referenced this pull request Jun 26, 2025
@kibanamachine @stratoula
[8.19] [Lens][ES|QL] Do not rerun the hook in case of an error in the…
43db938 
… query (#225067) (#225480)

# Backport

This will backport the following commits from `main` to `8.19`:
- [[Lens][ES|QL] Do not rerun the hook in case of an error in the query
(#225067)](#225067)

<!--- Backport version: 9.6.6 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sorenlouv/backport)

<!--BACKPORT [{"author":{"name":"Stratoula
Kalafateli","email":"efstratia.kalafateli@elastic.co"},"sourceCommit":{"committedDate":"2025-06-26T14:56:50Z","message":"[Lens][ES|QL]
Do not rerun the hook in case of an error in the query (#225067)\n\n##
Summary\n\nWhile testing the ES|QL charts I realized that in case of an
error in\nthe query, the hook goes into a loop and causes performance
issues.\n\nAs the error is being reported we do not need to re-run the
query to get\nthe results\n\nFor example if you create a control
wrongly.\n\ne.g.\n\n1. Create a chart and add a control which will
create an error:\n\n<img width=\"508\"
alt=\"image\"\nsrc=\"https://github.com/user-attachments/assets/f2013d2c-e161-47bf-a3cb-d5033be9de59\"\n/>\n\n2.
Add to the control no-date fields. e.g. clientip\n3. Check the editor is
not going into a rendering loop\n\n<img width=\"482\"
alt=\"image\"\nsrc=\"https://github.com/user-attachments/assets/cc541b68-b317-41ae-b4a6-87569466edd6\"\n/>\n\n\n###
Release notes\nFixes a performance issue in the Lens ES|QL charts in
case of errors in\nthe query.\n\n### Checklist\n\n- [ ] [Unit or
functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere
updated or added to match the most common
scenarios","sha":"731ab844879e068da6c9d60e206ddc501c49f415","branchLabelMapping":{"^v9.1.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Feature:Lens","Feature:ES|QL","backport:version","v9.1.0","v8.19.0"],"title":"[Lens][ES|QL]
Do not rerun the hook in case of an error in the
query","number":225067,"url":"https://github.com/elastic/kibana/pull/225067","mergeCommit":{"message":"[Lens][ES|QL]
Do not rerun the hook in case of an error in the query (#225067)\n\n##
Summary\n\nWhile testing the ES|QL charts I realized that in case of an
error in\nthe query, the hook goes into a loop and causes performance
issues.\n\nAs the error is being reported we do not need to re-run the
query to get\nthe results\n\nFor example if you create a control
wrongly.\n\ne.g.\n\n1. Create a chart and add a control which will
create an error:\n\n<img width=\"508\"
alt=\"image\"\nsrc=\"https://github.com/user-attachments/assets/f2013d2c-e161-47bf-a3cb-d5033be9de59\"\n/>\n\n2.
Add to the control no-date fields. e.g. clientip\n3. Check the editor is
not going into a rendering loop\n\n<img width=\"482\"
alt=\"image\"\nsrc=\"https://github.com/user-attachments/assets/cc541b68-b317-41ae-b4a6-87569466edd6\"\n/>\n\n\n###
Release notes\nFixes a performance issue in the Lens ES|QL charts in
case of errors in\nthe query.\n\n### Checklist\n\n- [ ] [Unit or
functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere
updated or added to match the most common
scenarios","sha":"731ab844879e068da6c9d60e206ddc501c49f415"}},"sourceBranch":"main","suggestedTargetBranches":["8.19"],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/225067","number":225067,"mergeCommit":{"message":"[Lens][ES|QL]
Do not rerun the hook in case of an error in the query (#225067)\n\n##
Summary\n\nWhile testing the ES|QL charts I realized that in case of an
error in\nthe query, the hook goes into a loop and causes performance
issues.\n\nAs the error is being reported we do not need to re-run the
query to get\nthe results\n\nFor example if you create a control
wrongly.\n\ne.g.\n\n1. Create a chart and add a control which will
create an error:\n\n<img width=\"508\"
alt=\"image\"\nsrc=\"https://github.com/user-attachments/assets/f2013d2c-e161-47bf-a3cb-d5033be9de59\"\n/>\n\n2.
Add to the control no-date fields. e.g. clientip\n3. Check the editor is
not going into a rendering loop\n\n<img width=\"482\"
alt=\"image\"\nsrc=\"https://github.com/user-attachments/assets/cc541b68-b317-41ae-b4a6-87569466edd6\"\n/>\n\n\n###
Release notes\nFixes a performance issue in the Lens ES|QL charts in
case of errors in\nthe query.\n\n### Checklist\n\n- [ ] [Unit or
functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere
updated or added to match the most common
scenarios","sha":"731ab844879e068da6c9d60e206ddc501c49f415"}},{"branch":"8.19","label":"v8.19.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:version Backport to applied version labels Feature:ES|QL ES|QL related features in Kibana Feature:Lens release_note:fix v8.19.0 v9.1.0

4 participants

@stratoula @kibanamachine @elasticmachine @dej611