Refactor of the value cache #2151

wildum · 2024-11-25T11:27:07Z

What is the "value cache"

The value cache (value_cache.go) stores arguments and exports of the components in the graph to build scope. The scope is used by the Alloy syntax on evaluation to resolve the expressions on the arguments of the components.
For example, when you have forward_to = [prometheus.remote_write.default.receiver], the Alloy syntax will check in the scope to find the receiver which corresponds to "prometheus.remote_write.default.receiver".

Why we should refactor this part of the code

It contains some dead code and overcomplicated logic which are artifacts from the past:
- There is no need to cache the component's arguments in the value cache because they are not added to the scope
- It has a complicated recursive process to build the Scope on every evaluation.
For the foreach feature, we are using custom components to represent the instances of the foreach and we would like these instances to have access to the root scope. This could be done by using the parent scope of the scope but the collisions are not handled the way we need (see the slack thread for more context).
This is something that we wanted to do post modules 2.0 but did not have time with the Alloy launch (we had a google doc for this but I could not find it)

About the solution

I first considered merging the parent scope with the built scope but merging the nested maps brings more complexity to the value cache which is already hard to follow and this feels very hacky to merge the parent scope with the child just because the Alloy syntax does not resolve it the needed way.

To avoid this I came up with a different approach:

instead of building a new Scope on every evaluation, the value cache keeps track of the current scope and updates it every time a component has a new exported value. When a component needs the scope to evaluate its arguments, it will just get a copy of it from the value_cache.
I removed the parent scope because it's not used and its usage in the Alloy syntax is misleading. The root scope can be passed down to custom components and used as a basis in their value cache.

Tests

I added more unit tests + error handling. I also manually tested it with the foreach feature that's currently in progress + did some manual tests.

internal/runtime/internal/controller/value_cache.go

syntax/vm/vm.go

internal/runtime/internal/controller/value_cache.go

approved by accident

ptodev

Thank you for this change! I suppose the simplification is a step in the right direct. I find both the new and the old code hard to understand though... Maybe it's just me but it feels too convoluted. I tried to review and to leave useful comments, but I'll need to review it again and hopefully I'll understand it better.

internal/runtime/internal/controller/value_cache.go

internal/runtime/internal/controller/loader.go

internal/runtime/internal/controller/value_cache.go

ptodev · 2024-12-04T13:37:14Z

internal/runtime/internal/controller/value_cache.go

-			continue
+	// Remove the component exports from the scope.
+	for _, id := range cleanupIds {
+		err := cleanup(vc.scope.Variables, id, 0)


What does the 0 mean? It might be nice to have a comment above func cleanup( to mention it.

this is the index that refers to the first part of the componentID (see the answer of the comment below). I will add some comments

can you instead have

func cleanup(m map[string]any, id ComponentID) error { return cleanup_from_index(m, id, 0) }

that could make it easier to read. Both me and Paulin seem to have found this somewhat confusing.

internal/runtime/internal/controller/value_cache.go

wildum · 2024-12-06T14:32:54Z

I find both the new and the old code hard to understand though... Maybe it's just me but it feels too convoluted.

Thanks for the review, it's definitely not just you, it's hard to simplify it because it must build the scope tree which is quite a complex object to make + the fact that it must handle the args for the modules and the "parent" context. I also don't want to start a massive refactoring of everything. I will work on the review feedback and see whether I can simplify it a bit more.

Maybe further refactoring PRs can follow to simplify other areas + add doc + add tests to consolidate the logic and simplify the code

internal/runtime/internal/controller/loader.go

internal/runtime/internal/controller/value_cache.go

wildum · 2024-12-09T15:10:29Z

@mattdurham @ptodev Thank you both for the review, I answered your comments and made some modifications. Please take another look when you have time and feel free to add more comments or unresolved existing ones. We can also discuss it over a call if you have concerns about the approach or find some parts confusing and would like to talk about alternatives

thampiotr

I have a few questions

thampiotr · 2024-12-11T10:44:04Z

internal/runtime/internal/controller/loader.go

+// TODO: Refactor this function to avoid using an error as input solely for shadowing purposes.
+// This design choice limits extensibility and makes it harder to modify or add functionality.


What do you mean that this function is using error "solely for shadowing purposes"?

I can see that it just logs the error as part of post evaluation steps, doesn't seem too odd for a function called postEvaluate.

thampiotr · 2024-12-11T10:45:46Z

internal/runtime/internal/controller/loader.go

+		err2 := l.cache.CacheExports(c.ID(), c.Exports())
+		if err2 != nil {
+			level.Error(logger).Log("msg", "failed to cache exports after evaluation", "err", err2)
+			// Don't mask the previous evaluation error.


Should we join errors here so we don't lose anything?

thampiotr · 2024-12-11T10:51:07Z

internal/runtime/internal/controller/value_cache.go

+	keyMap := make(map[string]any)
+	keyMap["value"] = value
+	vc.moduleArguments[key] = keyMap


Hmm, why do we have a map of maps if the second layer of maps always has only one key "value"? Why not a map of keys to values?

because to refer to an argument value in the config file you need to write "argument.LABEL.value". To resolve it, it must have the path ["argument"][LABEL]["value"]

Ah gotcha. Yeah now I remember this syntax. Would things break if we made both foo.bar.value and foo.bar work? It could be UX improvement 🤔

that would be nice but it's tricky to do because you cant have two values for the same key in the map + you would need to handle the weird cases where the user names its argument "value".

thampiotr · 2024-12-11T10:52:44Z

internal/runtime/internal/controller/value_cache.go

+	v, exist := vc.moduleArguments[key]
+	return v, exist


Suggested change

v, exist := vc.moduleArguments[key]

return v, exist

return vc.moduleArguments[key]

thampiotr · 2024-12-11T10:55:33Z

internal/runtime/internal/controller/value_cache.go

+	// Remove the component exports from the scope.
+	for _, id := range cleanupIds {
+		// Start with the index "0". It refers to the first part of the componentID and it's used for recursion.
+		err := cleanup(vc.scope.Variables, id, 0)


Can we try to make this a bit more readable - I find the index argument set to 0 here pretty hard to follow and I think we can probably find a way to simplify.

thampiotr · 2024-12-11T10:58:05Z

internal/runtime/internal/controller/value_cache.go

-			continue
+	// Remove the component exports from the scope.
+	for _, id := range cleanupIds {
+		err := cleanup(vc.scope.Variables, id, 0)


can you instead have

func cleanup(m map[string]any, id ComponentID) error { return cleanup_from_index(m, id, 0) }

that could make it easier to read. Both me and Paulin seem to have found this somewhat confusing.

thampiotr · 2024-12-11T11:09:02Z

internal/runtime/internal/controller/value_cache.go

-	nodeID := id.String()
-	vc.components[nodeID] = id
+	variables := vc.scope.Variables
+	// Build nested maps.


Can you help me understand why do we choose to take on the complexity of the nested maps instead of using a flattened map with a concatenated component id parts, like "foo.bar.baz" instead of {foo: { bar: baz}}?

because that's the structure that we need to give to the Alloy syntax to evaluate the expression. It's also the structure that we use for modules to propagate the context. The current implementation builds it on every evaluation whereas this new implementation stores the values directly in it

wildum requested a review from a team as a code owner November 25, 2024 11:27

Merge scope hierarchy for evaluation

924df73

wildum force-pushed the fix-vm-parent-scope-lookup branch from e64d648 to 924df73 Compare November 25, 2024 11:35

ptodev previously approved these changes Nov 25, 2024

View reviewed changes

internal/runtime/internal/controller/value_cache.go Outdated Show resolved Hide resolved

internal/runtime/internal/controller/value_cache.go Outdated Show resolved Hide resolved

get rid of parent scope

cc9c0a3

mattdurham reviewed Nov 25, 2024

View reviewed changes

syntax/vm/vm.go Show resolved Hide resolved

mattdurham reviewed Nov 25, 2024

View reviewed changes

internal/runtime/internal/controller/value_cache.go Outdated Show resolved Hide resolved

wildum force-pushed the fix-vm-parent-scope-lookup branch from 1a1f1a9 to 0a65e49 Compare November 27, 2024 14:24

wildum marked this pull request as draft November 27, 2024 14:25

refactor value_cache

00bd528

wildum force-pushed the fix-vm-parent-scope-lookup branch from 0a65e49 to 00bd528 Compare November 29, 2024 13:07

wildum changed the title ~~Merge scope hierarchy for evaluation~~ Refactor of the value_cache Nov 29, 2024

wildum changed the title ~~Refactor of the value_cache~~ Refactor of the value cache Nov 29, 2024

wildum marked this pull request as ready for review November 29, 2024 14:21

ptodev reviewed Dec 4, 2024

View reviewed changes

mattdurham reviewed Dec 6, 2024

View reviewed changes

internal/runtime/internal/controller/loader.go Show resolved Hide resolved

mattdurham reviewed Dec 6, 2024

View reviewed changes

internal/runtime/internal/controller/loader.go Show resolved Hide resolved

mattdurham reviewed Dec 6, 2024

View reviewed changes

internal/runtime/internal/controller/value_cache.go Outdated Show resolved Hide resolved

mattdurham reviewed Dec 6, 2024

View reviewed changes

internal/runtime/internal/controller/value_cache.go Show resolved Hide resolved

review feedback

8bc2094

thampiotr reviewed Dec 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor of the value cache #2151

Refactor of the value cache #2151

wildum commented Nov 25, 2024 •

edited

Loading

ptodev left a comment

ptodev Dec 4, 2024

wildum Dec 9, 2024

thampiotr Dec 11, 2024

wildum commented Dec 6, 2024

wildum commented Dec 9, 2024

thampiotr left a comment

thampiotr Dec 11, 2024

thampiotr Dec 11, 2024

thampiotr Dec 11, 2024

wildum Dec 11, 2024

thampiotr Dec 11, 2024

wildum Dec 11, 2024

thampiotr Dec 11, 2024

thampiotr Dec 11, 2024

thampiotr Dec 11, 2024

thampiotr Dec 11, 2024

wildum Dec 11, 2024

		// TODO: Refactor this function to avoid using an error as input solely for shadowing purposes.
		// This design choice limits extensibility and makes it harder to modify or add functionality.

	v, exist := vc.moduleArguments[key]
	return v, exist
	return vc.moduleArguments[key]

Refactor of the value cache #2151

Are you sure you want to change the base?

Refactor of the value cache #2151

Conversation

wildum commented Nov 25, 2024 • edited Loading

What is the "value cache"

Why we should refactor this part of the code

About the solution

Tests

ptodev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wildum commented Dec 6, 2024

wildum commented Dec 9, 2024

thampiotr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wildum commented Nov 25, 2024 •

edited

Loading