Declare new modules (second attempt) #6291
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DeclareNode is a new node which represents a `declare` block. Another node `ImportConfigNode` is added as an empty shell to help understanding the design choices of the PR. Its implementation will come later. For now it is not populated and has no impact.
wildum
force-pushed
the
declare-new-modules-2
branch
from
eb67d89 to
b20d2ae
Compare
CustomComponentRegistry holds custom component definitions that are available in the loaded config. It's structured as a tree that maps the hierarchy between the declare and import blocks.
CustomComponent is defined in the controller and aimed to be managed by CustomComponentNodes. This replaces the component.Module interface which will be removed with the old modules. This change aligns with the new modules terminology and keeps the custom components within the controller package. A custom component can load a config via LoadBody and run via Run. LoadOptions are added to allow users to pass additional options to the loader with the new config. For now, the LoadOptions contain a CustomComponentRegistry which provides custom component templates that the custom component can use to instantiate other custom components.
CustomComponentNode is a controller node which manages a custom component. On evaluation, it fetches its config(template+customComponentRegistry) via a callback and starts a new CustomComponent.
This change is necessary to show the running components inside of a custom component in the UI.
The ComponentNodeManager is responsible for creating new components nodes and obtaining the necessary information to run them. The data is stored in a builtinComponentRegistry and a customComponentRegistry. The created custom components must be wired to the declare and import nodes that they depend on to ensure that their config is loaded before they evaluate and to trigger a re-evaluation when an import node they depend on received new content.
Add tests to check that the declare functionality with custom components works as intended in various scenarios (from basic to several levels of nesting) Test also that cycle dependencies are prevented.
wildum
force-pushed
the
declare-new-modules-2
branch
from
b20d2ae to
b9e198a
Compare
rfratto
reviewed
Feb 5, 2024
There was a problem hiding this comment.
This is my only round of feedback before approving. I have problems following the code, particularly tracing where custom component registries are created and passed around, as the combined piece of logic spread across a few files.
But at this point, where we need modules done, I'm not interested in blocking this PR and trying to rearchitect how data is made available to nested controllers. Let's disagree and commit after my last bits of feedback here are addressed.
Shadow namespace - name a declare block with an existing namespace (prometheus for example) (correct) Shadow declare - redeclare a declare block (correct) OutOfScopeReference - trying to reference the exports of a node outside of the declare (error) OutOfScopeDefinition - trying to instantiate a custom component via an out of scope template (error) ForbiddenDeclareLabel - using the label "declare" for a declare block (error) UpdateDeclare - reload the config with a different declare definition (correct)
rfratto
approved these changes
Feb 7, 2024
There was a problem hiding this comment.
Thanks William! Feel free to merge after this last comment is addressed.
BarunKGP
pushed a commit
to BarunKGP/grafana-agent
that referenced
this pull request
Feb 20, 2024
* Add declare node and import config node skeleton DeclareNode is a new node which represents a `declare` block. Another node `ImportConfigNode` is added as an empty shell to help understanding the design choices of the PR. Its implementation will come later. For now it is not populated and has no impact. * Add custom component registry CustomComponentRegistry holds custom component definitions that are available in the loaded config. It's structured as a tree that maps the hierarchy between the declare and import blocks. * Add CustomComponent interface CustomComponent is defined in the controller and aimed to be managed by CustomComponentNodes. This replaces the component.Module interface which will be removed with the old modules. This change aligns with the new modules terminology and keeps the custom components within the controller package. A custom component can load a config via LoadBody and run via Run. LoadOptions are added to allow users to pass additional options to the loader with the new config. For now, the LoadOptions contain a CustomComponentRegistry which provides custom component templates that the custom component can use to instantiate other custom components. * Add CustomComponentNode CustomComponentNode is a controller node which manages a custom component. On evaluation, it fetches its config(template+customComponentRegistry) via a callback and starts a new CustomComponent. * Add modulesIDs to the ComponentNode interface This change is necessary to show the running components inside of a custom component in the UI. * Add ComponentNodeManager The ComponentNodeManager is responsible for creating new components nodes and obtaining the necessary information to run them. The data is stored in a builtinComponentRegistry and a customComponentRegistry. The created custom components must be wired to the declare and import nodes that they depend on to ensure that their config is loaded before they evaluate and to trigger a re-evaluation when an import node they depend on received new content. * Add test declare Add tests to check that the declare functionality with custom components works as intended in various scenarios (from basic to several levels of nesting) Test also that cycle dependencies are prevented. * forbid using 'declare' as a label for declare blocks * Add tests for declare Shadow namespace - name a declare block with an existing namespace (prometheus for example) (correct) Shadow declare - redeclare a declare block (correct) OutOfScopeReference - trying to reference the exports of a node outside of the declare (error) OutOfScopeDefinition - trying to instantiate a custom component via an out of scope template (error) ForbiddenDeclareLabel - using the label "declare" for a declare block (error) UpdateDeclare - reload the config with a different declare definition (correct) * remove import logic * add examples to circle and self dependencies * change how apply options are passed to the loader * refactor collectCustomComponentReferences to reduce nesting * remove stale comment * reduce redundancy * add missing error check with test * change error message * change error msg for unrecognized component
github-actions
bot
added
the
frozen-due-to-age
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Description
This PR introduces the declare functionality defined by the #5547.
Notes to the Reviewer
The PR can be reviewed commit by commit.
It contains some parts of the import logic to help the reviewers understand how the import will fit into the current design. The missing parts are left as "TODO" and will come via another PR. (it has already been implemented and tested with this design)
The structure of the design relies on a ComponentNodeManager which holds a builtinComponentRegistry and a customComponentRegistry. It is used by the loader to create ComponentNodes and provide the data that they need to run.
PR Checklist