Allow other nodes than the component nodes to have dependants

[wildum]

PR Description

Currently, only component nodes can trigger other nodes for re-evaluation at run time.
This PR introduces an interface to allow other nodes that implement it to have dependants.
For now, only the component nodes implement this interface but the new modules PR will introduce other types of nodes that will have dependants to send for re-evaluation.

[rfratto]

I want to make sure I understand the refactoring being done here and how it relates to the new modules work. What is a high-level overview of the list of nodes we're planning to introduce, and what events will they emit that should cause other components to re-evaluate? (For example, components emit an event when they change their exported values, so components which rely on those exports should be updated)

[mattdurham]

At this moment this is mostly a rename with future changes to allow config blocks/others to trigger evaluations?

[wildum]

That's correct. Custom node components and import config nodes will implement this interface.
Custom node components will act like builtin node components: they will update their exports which other nodes can reference.
Custom node components might also depend on Import config nodes if their config is imported or/and if they contain custom components that depend on imported config. When an Import Config Node receives new content, the custom components that depend on it should be re-evaluated.

[rfratto]

The code itself is fine, but I have a concern about the interface design and whether we can simplify it by rolling up extra responsibilities into BlockNode.

[rfratto]

I think there's a few issues with this interface:

• The name is potentially misleading. The name implies it's about a characteristic of a particular node, but the description is more about whether a BlockNode can emit an event informing its dependents to be updated. In fact, from the perspective of the DAG, any node can have dependents regardless of whether it implements this interface.

• The set of methods is perhaps too broad; it's not obvious why a node which can emit updates to cause other nodes to be evaluated needs to have component exports, or a component ID.

Because of the points above, I think it's hard to justify introducing this interface as a new concept to keep inside one's mental model of the controller code.

I would recommend that we aim to not introduce this interface, opting instead for the following:

1. Move LastUpdateTime() to BlockNode instead. This would be a reasonable expectation of a BlockNode; as all BlockNodes support evaluating, it makes sense for them to also report their last update time. (Other methods should not be moved, just LastUpdateTime.)
2. Instead of changing code to accept NodeWithDependents, update it to accept a BlockNode instead. You will need to add some type switching when a more concrete type is necessary.
   • This will also have the added benefit of making some of the changed code a bit shorter; OnNodeWithDependantsUpdate is a bit harder to read than OnBlockNodeUpdate.

WDYT?

[wildum]

Makes sense, I was not really happy with the name in the first place so I'm happy to change this :)

[wildum]

I made the changes to see how it looks but after putting more thought into it I'm not 100% happy with it:

• For the component node, LastUpdateTime actually refers to the last time its exports were updated, which is not necessarily tied to when the node was updated or evaluated.
• In the Evaluate dependant I use a type cast to cache the exports because the BlockNode does not have the Export() function but I feel like this should be the default behavior. All BlockNodes can be evaluated which means that they have a concept of "Arguments" (inputs). I think that it actually makes sense that they also have a concept of "Exports" (outputs) and that these exports should be cached if not nil.

But for now the definitions of Arguments and Exports are really tied to components and not nodes so it would require bigger changes.

[rfratto]

For the component node, LastUpdateTime actually refers to the last time its exports were updated, which is not necessarily tied to when the node was updated or evaluated.

Ah, I see. Honestly, I don't think a node should have to track its own export time anyway; the controller could be tracking that when OnBlockNodeUpdate is called. What do you think? Should that be scoped into this PR so we can take it out of the interface?

[wildum]

Sounds good, the change is small and the time is only used by one metric. I think that it is worth doing it in the PR as it prevents introducing redundant code. I will take care of it

[rfratto]

LGTM, but there's an open question about whether LastUpdateTime should be removed from the interface entirely and move responsibility of update time tracking to the Loader.

Let me know if you want to make that change here or if we should merge this as-is.

[wildum]

@thampiotr I changed the way the LastUpdateTime is passed for the metric "agent_component_dependencies_wait_seconds". Since this was introduced by you some time ago you might want to have a look

[rfratto]

I think this is fine too; though I find it very slightly unfortunate that the loader has to be aware of node queueing.

Another to handle this would be for the loader to maintain a map of node -> evaluated time, and for the loader to wrap the OnBlockNodeUpdate function. That way you don't have to introduce the QueueNode type for information only relevant to the loader and not the queue.

Let me know what you'd like to do (keep it as-is and merge or change it)

[wildum]

Another to handle this would be for the loader to maintain a map of node -> evaluated time

I thought about it but then the map would be accessed concurrently so it would need to be protected by a mutex and I was worried that it might slow down evaluation. I found that introducing the QueueNode type made things simpler

[rfratto]

Thanks William! Will merge as soon as the CI goes green.