🌊 Streams: Improve performance #230048
Merged
🌊 Streams: Improve performance #230048
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
/ci |
Contributor
Author
|
/ci |
Contributor
Author
|
/ci |
flash1293
added
release_note:skip
Contributor
Author
|
/ci |
Contributor
Author
|
/ci |
Contributor
|
Pinging @elastic/obs-ux-logs-team (Team:obs-ux-logs) |
Contributor
💚 Build Succeeded
Metrics [docs]Async chunks
History
|
miltonhultgren
approved these changes
Aug 5, 2025
Contributor
miltonhultgren
left a comment
miltonhultgren
left a comment
There was a problem hiding this comment.
This looks good to me, my only concern is that we now need to pay much more attention to any kind of schema change to those stored documents and make sure our migration code works well and is tested.
NicholasPeretti
pushed a commit
to NicholasPeretti/kibana
that referenced
this pull request
Aug 18, 2025
When dealing with lots of streams, performance of the Kibana layer of streams can be an issue. This PR is fixing the worst problems - there are further gains possible, but this is shifting the bottleneck to Elasticsearch in most situations. # More performant type guards The zod-based `Definition.is` implementation basically did a complete parse, which is pretty expensive. This PR replaces it with a very simple typeguard. Since the argument passed to `Definition.is` is already limited to a base stream definition, this isn't losing any functionality. # Do not parse when loading from storage We used to parse the documents coming from the storage adapter, but this can be pretty expensive if there are lots of streams, and it shouldn't be required since we make sure we only store good definitions on the way in, so we don't need to check again on the way out. On the contrary, if there is a bug and the documents from storage don't 100% match the zod schema, with the parse in place on read streams stops working completely. Using the malformed doc in a best-effort way might cause other issues somewhere else, but it is minimizing the impact. If you feel strongly about it, maybe we can keep the additional check in test/dev mode, but disable it on prod. Another subtle change this causes is that default values from the zod schema wouldn't be set anymore on definitions coming from the storage adapter - I'm not aware we rely on this right now, and it might be better not to anyway (changing defaults is very close to a breaking change anyway, but more hidden) # Do not re-fetch the same information if we have it already This is the worst offender - in the validation logic for a wired stream upsert we would check whether there are non-wired streams as ancestors by doing requests to Elasticsearch and we would also re-fetch all ancestors and descendants _again_ for definitions. This PR changes this logic to only check for conflicts for new streams and uses `desiredState` to check for ancestors and descendants instead of refetching them # Make it cheap to find whether a stream is an ancestor `isDescendantOf` is called a lot to find ancestors and descendants, but it was written in a super verbose and inefficient way. This PR fixes that.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When dealing with lots of streams, performance of the Kibana layer of streams can be an issue.
This PR is fixing the worst problems - there are further gains possible, but this is shifting the bottleneck to Elasticsearch in most situations.
More performant type guards
The zod-based
Definition.isimplementation basically did a complete parse, which is pretty expensive. This PR replaces it with a very simple typeguard. Since the argument passed toDefinition.isis already limited to a base stream definition, this isn't losing any functionality.Do not parse when loading from storage
We used to parse the documents coming from the storage adapter, but this can be pretty expensive if there are lots of streams, and it shouldn't be required since we make sure we only store good definitions on the way in, so we don't need to check again on the way out.
On the contrary, if there is a bug and the documents from storage don't 100% match the zod schema, with the parse in place on read streams stops working completely. Using the malformed doc in a best-effort way might cause other issues somewhere else, but it is minimizing the impact. If you feel strongly about it, maybe we can keep the additional check in test/dev mode, but disable it on prod.
Another subtle change this causes is that default values from the zod schema wouldn't be set anymore on definitions coming from the storage adapter - I'm not aware we rely on this right now, and it might be better not to anyway (changing defaults is very close to a breaking change anyway, but more hidden)
Do not re-fetch the same information if we have it already
This is the worst offender - in the validation logic for a wired stream upsert we would check whether there are non-wired streams as ancestors by doing requests to Elasticsearch and we would also re-fetch all ancestors and descendants again for definitions. This PR changes this logic to only check for conflicts for new streams and uses
desiredStateto check for ancestors and descendants instead of refetching themMake it cheap to find whether a stream is an ancestor
isDescendantOfis called a lot to find ancestors and descendants, but it was written in a super verbose and inefficient way. This PR fixes that.