[cmd/opampsupervisor] Make configuration merge more customizable through special config files #40295
Conversation
douglascamata
requested review from
evan-bradley,
atoulme and
a team
as code owners
|
Any chance to avoid the term magic? Would you please describe the intent, it looks like you have enough design going on a small RFC would help. |
The only other term that comes to my head and doesn't create even more confusion would be "special", which we could also use. Do you have any other suggestions, @atoulme?
We already have the initial intent in #39963, which was to be able to have local config files added into the configuration stack at the lowest priority level. The idea for the technical approach comes from a suggestion in the same issue. If really necessary for this I can try to write an RFC. @evan-bradley, by chance do you already have something written somewhere about those future plans for writing all configs to disk and letting the Collector itself merge them all? |
| the configuration files specified are applied after the configuration from the OpAMP server. | ||
| After the configuration files, arguments present in `.agent.args` are passed to the executable binary. | ||
| The environmanet variables specified in `.agent.env` are set in the collector process environment. | ||
| There are a few "magic" configuration files that can be used to completely |
There was a problem hiding this comment.
I vote we do not refer to these as magic configuration files. Something like reserved or key or global, variable or special as it feels more professional. I dont want users to think using these variables are something to be avoided.
There was a problem hiding this comment.
I'll update this to call them "special" for now. 👍
| - If the `$OPAMP_EXTENSION_CONFIG` is not specified, the Collector | ||
| will not be able to connect to the Supervisor, triggering a restart loop. | ||
| - If the `$OWN_METRICS_CONFIG` is not specified, the Collector will not be able | ||
| to report its own metrics. | ||
| - If the `$BUILTIN_CONFIG` is not specified, the Collector will not have the | ||
| expected identifying, non-identifying and resource attributes. | ||
| - If the `$REMOTE_CONFIG` is not specified, the Collector will not be able to | ||
| apply the remote configuration received from the Supervisor. |
There was a problem hiding this comment.
Do we need to provide validation against the merged config that the things we expect to be present are present? I feel like we are opening up ourselves to a lot of troubleshooting issues from users holding this feature incorrectly
There was a problem hiding this comment.
@TylerHelmuth I thought about this a bit. One way we could avoid this problem is to automatically prepend any non-specified special file, so this way what we expect to be there will be there unless modified by later/higher-level configurations at the expense of having a bit of "hidden" behavior. I think this can be a good tradeoff. WDYT?
There was a problem hiding this comment.
Ya always prepending any of our special configurations if it is not specified in the list by the user feels like a safer default.
There was a problem hiding this comment.
I'm okay taking this on a case-by-case basis: for example, I'm not as worried about the own metrics config as the other ones.
However in general I think we should enable default anything that would cause the Supervisor to break in most cases (removing the OpAMP extension config) or introduce unclear behavior (the remote configuration capability is enabled, but the remote config merge order isn't set, so don't use remote config). For now I would be okay just always including all of the special configurations.
There was a problem hiding this comment.
Thanks for working on this @douglascamata. I've only looked at the specification so far since I want to get that right before focusing on the implementation.
| - `$BUILTIN_CONFIG`: built-in base configuration. | ||
| - `$REMOTE_CONFIG`: remote configuration received by the Supervisor. | ||
|
|
||
| If **none** of these are explicitly specified, they are automatically prepended to the |
There was a problem hiding this comment.
This would be a breaking change for the current behavior, but I would vote for having these appended to the list if they're unspecified:
- The OpAMP extension configuration added by the Supervisor is essential for the Supervisor to work and disabling it should be something users have to explicitly opt into by putting the extension config before their custom config.
- Ultimately, remote configuration is the "most specific" configuration, since it's been deliberately sent to a Collector by an operator. I think we should always respect this configuration unless the Collector has been specifically set up to override values that may be present in the remote configuration.
There was a problem hiding this comment.
Sounds good to me, @evan-bradley. I think it's fine for us to have this breaking change now, as the component is in a very early stage (alpha/dev) and we need a strong/solid base for the future.
…tor-contrib into supervisor-config-file-revamp
|
Hey folks, I updated the PR with your feedback. The "magic" config files were renamed to "special". The behavior now when any special config file is not present is to modify the config file list to achieve the following order:
|
douglascamata
changed the title
Signed-off-by: Douglas Camata <[email protected]>
Signed-off-by: Douglas Camata <[email protected]>
Description
This PR refactors the logic of the configuration loading and merging in the
cmd/opampsupervisorpackage to allow for more customization, control and to have a more explicit ordering for users.Link to tracking issue
Fixes #39963.
Testing
Did some manual testing and ensured our current tests pass.
Documentation