Store AST directly in provider mappings #6114

swiatekm · 2024-11-21T12:49:16Z

What does this PR do?

For all variable provider types, we store mappings as untyped dictionaries and only convert them to AST when we're about to generate actual vars. This means we have to recompute all of it every time there's a change to any of the provider data, even if all the other data is unaffected. For example, if a Kubernetes Pod gains a new label, we need to regenerate vars for all Pods.

Instead, we can store mappings as AST to begin with. Then the only cost of emitting new vars after a change is updating a bunch of references.

Why is it important?

This unnecessary recomputation can be quite expensive when there's a lot of mappings from the dynamic provider. See the benchstat report below:

goos: linux
goarch: amd64
pkg: github.com/elastic/elastic-agent/internal/pkg/composable
cpu: 13th Gen Intel(R) Core(TM) i7-13700H
                       │ bench_main.txt │          bench_dynamic.txt          │
                       │     sec/op     │   sec/op     vs base                │
GenerateVars100Pods-20     755.49µ ± 3%   76.87µ ± 2%  -89.82% (p=0.000 n=10)

                       │ bench_main.txt │          bench_dynamic.txt           │
                       │      B/op      │     B/op      vs base                │
GenerateVars100Pods-20    735.00Ki ± 0%   98.69Ki ± 0%  -86.57% (p=0.000 n=10)

                       │ bench_main.txt │          bench_dynamic.txt          │
                       │   allocs/op    │  allocs/op   vs base                │
GenerateVars100Pods-20     21.335k ± 0%   1.364k ± 0%  -93.61% (p=0.000 n=10)

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas

Related issues

elasticmachine · 2024-11-21T13:59:19Z

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

swiatekm · 2024-11-21T15:05:58Z

/test

elastic-sonarqube · 2024-11-21T19:20:44Z

Quality Gate passed

Issues
0 New issues
1 Fixed issue
0 Accepted issues

Measures
0 Security Hotspots
93.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

pkoutsovasilis · 2024-11-21T22:06:27Z

internal/pkg/agent/transpiler/ast.go

 func (a *AST) Equal(other *AST) bool {
+	if a.root == nil || other.root == nil {
+		return a.root == other.root
+	}


nit 🙂

Suggested change

}

}

// nil check above is mandatory as calling Hash() on a nil may cause a panic

internal/pkg/composable/controller.go

pkoutsovasilis

This code change LGTM. the CI failures are due to TestOtelAPMIngestion (#4148) and TestRpmFleetUpgrade. The last one fails because it doesn't download in time the elastic-agent-8.17.0-SNAPSHOT-x86_64.rpm artifact with a context deadline exceeded (code ref)

cc @ycombinator should we create an issue about the latter?

ycombinator · 2024-11-21T22:40:05Z

the CI failures are due to TestOtelAPMIngestion (#4148) and TestRpmFleetUpgrade. The last one fails because it doesn't download in time the elastic-agent-8.17.0-SNAPSHOT-x86_64.rpm artifact with a context deadline exceeded (code ref)

cc @ycombinator should we create an issue about the latter?

I'm not seeing these tests failing on main so either something in this PR is causing them to fail (unlikely) or they are flaky.
I just re-ran the integration tests step. If it passes, it means those tests are flaky and we should create an issue for each of them.

blakerouse

This is a great performance improvement! Nicely done.

* Store AST directly in dynamic provider mappings * Store AST directly in context provider mappings (cherry picked from commit fe0f6b0)

* Store AST directly in dynamic provider mappings * Store AST directly in context provider mappings (cherry picked from commit fe0f6b0) # Conflicts: # internal/pkg/composable/controller.go

* Store AST directly in dynamic provider mappings * Store AST directly in context provider mappings (cherry picked from commit fe0f6b0) Co-authored-by: Mikołaj Świątek <mail@mikolajswiatek.com>

* Store AST directly in dynamic provider mappings * Store AST directly in context provider mappings (cherry picked from commit fe0f6b0) # Conflicts: # internal/pkg/composable/controller.go

(cherry picked from commit fe0f6b0) # Conflicts: # internal/pkg/composable/controller.go # Conflicts: # internal/pkg/composable/controller.go

(cherry picked from commit fe0f6b0) # Conflicts: # internal/pkg/composable/controller.go # Conflicts: # internal/pkg/composable/controller.go Co-authored-by: Mikołaj Świątek <mail@mikolajswiatek.com>

…ic#6159)" This reverts commit 24e60b6.

We only signal from providers if the emitted data is different from the current state. This caused a bug where the data wasn't ever emitted if it was empty. The provider controller now distinguishes between no data (yet) and empty data. As a result, it's again possible to effectively disable all providers by using a local provider with no data. This is a regression caused by #6114. It doesn't apply to the main and 8.x branches due to #6169 refactoring provider initialisation in those.

We only signal from providers if the emitted data is different from the current state. This caused a bug where the data wasn't ever emitted if it was empty. The provider controller now distinguishes between no data (yet) and empty data. As a result, it's again possible to effectively disable all providers by using a local provider with no data. This is a regression caused by #6114. It doesn't apply to the main and 8.x branches due to #6169 refactoring provider initialisation in those. (cherry picked from commit 8ea9439)

We only signal from providers if the emitted data is different from the current state. This caused a bug where the data wasn't ever emitted if it was empty. The provider controller now distinguishes between no data (yet) and empty data. As a result, it's again possible to effectively disable all providers by using a local provider with no data. This is a regression caused by #6114. It doesn't apply to the main and 8.x branches due to #6169 refactoring provider initialisation in those. (cherry picked from commit 8ea9439) Co-authored-by: Mikołaj Świątek <mail@mikolajswiatek.com>

swiatekm requested a review from a team as a code owner November 21, 2024 12:49

swiatekm requested review from andrzej-stencel and michel-laterman November 21, 2024 12:49

mergify bot assigned swiatekm Nov 21, 2024

swiatekm mentioned this pull request Nov 21, 2024

Add var generation benchmark #6028

Merged

2 tasks

pierrehilbert added the Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team label Nov 21, 2024

swiatekm force-pushed the fix/dynamic-provider-ast branch from 3c62d14 to 8cd14c8 Compare November 21, 2024 14:03

swiatekm added 2 commits November 21, 2024 19:34

Store AST directly in dynamic provider mappings

54ccfdd

Store AST directly in context provider mappings

6860e91

swiatekm force-pushed the fix/dynamic-provider-ast branch from 8cd14c8 to 6860e91 Compare November 21, 2024 18:34

pkoutsovasilis reviewed Nov 21, 2024

View reviewed changes

internal/pkg/composable/controller.go Show resolved Hide resolved

pkoutsovasilis approved these changes Nov 21, 2024

View reviewed changes

swiatekm removed the backport-8.15 Automated backport to the 8.15 branch with mergify label Nov 22, 2024

blakerouse approved these changes Nov 27, 2024

View reviewed changes

swiatekm merged commit fe0f6b0 into main Nov 27, 2024
15 checks passed

swiatekm deleted the fix/dynamic-provider-ast branch November 27, 2024 15:44

mergify bot pushed a commit that referenced this pull request Nov 27, 2024

Store AST directly in provider mappings (#6114)

19bf8c2

* Store AST directly in dynamic provider mappings * Store AST directly in context provider mappings (cherry picked from commit fe0f6b0)

mergify bot pushed a commit that referenced this pull request Nov 27, 2024

Store AST directly in provider mappings (#6114)

3367c88

* Store AST directly in dynamic provider mappings * Store AST directly in context provider mappings (cherry picked from commit fe0f6b0)

mergify bot mentioned this pull request Nov 27, 2024

[8.16](backport #6114) Store AST directly in provider mappings #6157

Merged

2 tasks

This was referenced Nov 27, 2024

[8.x](backport #6114) Store AST directly in provider mappings #6158

Merged

[8.17](backport #6114) Store AST directly in provider mappings #6159

Merged

swiatekm added a commit that referenced this pull request Dec 13, 2024

Store AST directly in provider mappings (#6114)

f4d052e

(cherry picked from commit fe0f6b0) # Conflicts: # internal/pkg/composable/controller.go # Conflicts: # internal/pkg/composable/controller.go

orestisfl added a commit to orestisfl/elastic-agent that referenced this pull request Jan 24, 2025

Revert "Store AST directly in provider mappings (elastic#6114) (elast…

a783ca8

…ic#6159)" This reverts commit 24e60b6.

swiatekm mentioned this pull request Jan 24, 2025

Emit vars even if provider data is empty from the start #6598

Merged

6 tasks

mergify bot mentioned this pull request Jan 27, 2025

[8.16](backport #6598) Emit vars even if provider data is empty from the start #6607

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Store AST directly in provider mappings #6114

Store AST directly in provider mappings #6114

Uh oh!

swiatekm commented Nov 21, 2024

Uh oh!

elasticmachine commented Nov 21, 2024

Uh oh!

swiatekm commented Nov 21, 2024

Uh oh!

elastic-sonarqube bot commented Nov 21, 2024

Uh oh!

pkoutsovasilis Nov 21, 2024

Uh oh!

Uh oh!

pkoutsovasilis left a comment •

edited

Loading

Uh oh!

ycombinator commented Nov 21, 2024

Uh oh!

blakerouse left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

	}
	}
	// nil check above is mandatory as calling Hash() on a nil may cause a panic

Store AST directly in provider mappings #6114

Store AST directly in provider mappings #6114

Uh oh!

Conversation

swiatekm commented Nov 21, 2024

What does this PR do?

Why is it important?

Checklist

Related issues

Uh oh!

elasticmachine commented Nov 21, 2024

Uh oh!

swiatekm commented Nov 21, 2024

Uh oh!

elastic-sonarqube bot commented Nov 21, 2024

Quality Gate passed

Uh oh!

pkoutsovasilis Nov 21, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pkoutsovasilis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ycombinator commented Nov 21, 2024

Uh oh!

blakerouse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pkoutsovasilis left a comment •

edited

Loading