Refactor of the value cache

[wildum]

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.

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.

[wildum]

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

[wildum]

@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]

LGTM - merge after or before the release?

[wildum]

merge after or before the release

let's do it after to leave this in dev for longer

[wildum]

mmh I thought the release was already cut, it will actually be in the next release. I think that's ok