-
Notifications
You must be signed in to change notification settings - Fork 144
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
Don't deepcopy AST when generating vars #6058
Conversation
This pull request does not have a backport label. Could you fix it @swiatekm? 🙏
|
|
a39ed5e
to
14dd681
Compare
Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane) |
14dd681
to
b012aaf
Compare
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice! Looks good.
🚀 |
(cherry picked from commit db6fbe2) # Conflicts: # internal/pkg/composable/controller.go
(cherry picked from commit db6fbe2) # Conflicts: # internal/pkg/composable/controller.go
(cherry picked from commit db6fbe2)
@swiatekm I think we want to backport this to 8.17 also. WDYT? |
(cherry picked from commit db6fbe2)
Are we 100% sure there is no hidden bug here? |
My two cents:
|
If anything, maybe we should skip the 8.15 backport? It's pretty close, and this isn't that critical of a fix. For 8.17, I think we should backport either way, but I could wait until 8.17.0 is out before merging the PR.
I'm never going to claim 100% for a codebase I haven't been working on for at least a year. I am as sure as I can reasonably be, I think. In practice, the change only has an effect when one of the dynamic variable providers is used. For these:
I could also manually test the docker provider, but I can't think of any reason it would be affected when the others aren't. |
(cherry picked from commit db6fbe2) Co-authored-by: Mikołaj Świątek <[email protected]>
(cherry picked from commit db6fbe2) # Conflicts: # internal/pkg/composable/controller.go
(cherry picked from commit db6fbe2)
(cherry picked from commit db6fbe2)
(cherry picked from commit db6fbe2) Co-authored-by: Mikołaj Świątek <[email protected]>
What does this PR do?
We generate vars from mappings from both context providers and dynamic providers. Dynamic providers can produce multiple var entries - the Kubernetes provider will do so for each Pod, for example. When that happens, we currently make a deep copy of the context provider mapping, and add the dynamic provider mapping as a key.
As we don't modify the mappings, this deep copy can be replaced by a shallow copy. This PR introduces shallow copying to the AST and uses it when generating vars, leading to a major performance improvement.
Why is it important?
All these deep copies can be quite expensive when there's a lot of mappings from the dynamic provider. See the benchstat report below:
Checklist
./changelog/fragments
using the changelog toolRelated issues