Utilise index in log item flyout to prevent scanning of all available indices #57761
Conversation
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
| @@ -52,7 +52,7 @@ interface ScrollableLogTextStreamViewProps { | |||
| }) => any; | |||
| loadNewerItems: () => void; | |||
| reloadItems: () => void; | |||
| setFlyoutItem: (id: string) => void; | |||
| setFlyoutRef: (ref: { id: string; index: string }) => void; | |||
There was a problem hiding this comment.
There's two setFlyoutItem in code base so this made sense. One is setting the loaded item. This one setting the one we want to load. So made sense to rename for two reason, it now takes more than just ID and it takes reference not an item
wylieconlon
added
Feature:Logs UI
|
Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui) |
|
Thanks for contributing these changes. ❤️ Unfortunately they collided with a move of the plugin directory (#54583) and therefore exhibit many conflicts. It's a good improvement, but I can't make any promises about when we'll get to it. 🙈 |
|
Aha I did see things were moving. If I get some time I might be able to quickly rebase or reapply the changes as that shouldn’t take too long. If someone could review in the meantime if I get to it and there’s any tweaks needed I could check those too |
… indices on getLogItem which can bring down small clusters
driskell
force-pushed
the
flyout-performance
branch
from
8544b6c to
284037b
Compare
|
@weltenwort Rebase completed. I haven't been able to test fully yet. |
|
Thank you for investing the effort - I'll try to take a look this week. Would you mind me pushing to this branch to resolve the new conflicts? |
|
@weltenwort sure no problem, thanks! |
|
Unfortunately I was unable to resolve the merge conflicts. It should be easy to re-apply your changes to a clean |
|
I'll create a new PR, but set you as the author in the respective commits. |
|
ℹ️ the new PR is #67004 |
Utilise index in log item flyout to prevent scanning of all available indices on getLogItem which can bring down small clusters
Summary
For more details see #38240
In Logs, when streaming, each item has an icon to display the full log entry in a fly out. However, on a large cluster with many logstash index partitions (we have about 100) this flyout never loads and brings down the Elasticsearch cluster due to memory and CPU going sky high. This is because the request to load the single log item only contains its ID and does not contain the partition/index. As such, all 100 logstash partitions/indices need to be checked for the ID. The debate on correctly sizing ES is a separate issue. Generally speaking there seems to be no reason why the flyout should not specify the partition/index the entry came from, and it indeed resolves the issue in #38240
Unfortunately could not run tests to find out what needs updating as the command ran for 30 minutes and never got past eslint, even though the git pre-commit hook eslint finished in seconds. I also could not use the GraphSQL code generation due to same issue as #46754 (though I was in 7.6 branch initially) so the files were updated manually.
It would be great if someone at Elastic could pick this up to finish if there's changes needed as I've very little time left. Due to restrictions in Elastic License I also can't use this derivative in production to test the issue is resolved so can't confirm whether or not the issue is resolved and makes the flyout instantaneous or not, which it quite possibly would, but I could not say if it does and certainly cannot say if it did as I have not done it, absolutely not.
Hopefully it's a good starting point and can make 7.7!
Checklist
Delete any items that are not applicable to this PR.
For maintainers