-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[RFC] - Configuration Merging revamped #13256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[RFC] - Configuration Merging revamped #13256
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #13256 +/- ##
==========================================
+ Coverage 91.62% 91.64% +0.01%
==========================================
Files 522 522
Lines 29208 29208
==========================================
+ Hits 26763 26767 +4
+ Misses 1926 1923 -3
+ Partials 519 518 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff, @VihasMakwana. I like this. I left comments but they are non-blockers to me.
|
||
## Open questions | ||
|
||
- What to do if an invalid option is provided for `merge_mode` or `merge_paths`? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My personal opinion on this is that we should error out and not start the Collector. If we log an error and merge the default way the output configuration might have very different behavior than what the user expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree.
We will support new parameters to config URIs as follows: | ||
1. `merge_paths`: A comma-separated list of glob patterns which will be used while config merging | ||
- This setting will control the paths user wants to merge from the given config. | ||
- Example: | ||
- `otelcol --config main.yaml --config extra.yaml?merge_paths=service::extensions,service::**::receivers` | ||
- In this example, we will merge the list of extensions and receivers from pipeline, excluding lists in the rest of the config. | ||
- `otelcol --config main.yaml --config ext.yaml?merge_paths=service::extensions --config rec.yaml?merge_paths=service::**::receivers` | ||
- In this example, we will merge all list of extensions from `ext.yml` and list of receivers from `rec.yaml`, excluding lists in the rest of the config. | ||
2. `merge_mode`: One of `prepend` or `append`. | ||
- This setting will control the ordering of merged list. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know how technically feasible is this today, @VihasMakwana? I am thinking about questions like:
- Does
koanf
support this syntax of the URI parameters in the file paths? Or will we have to "pre-process" the CLI args before passing them tokoanf
? - Can we support these gob-like patterns of
**
? Maybe we should start simpler to have some room for experimentation before adding more advanced path logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does koanf support this syntax of the URI parameters in the file paths? Or will we have to "pre-process" the CLI args before passing them to koanf?
We will do the preprocessing and merge as per user's CLI args.
Can we support these gob-like patterns of **? Maybe we should start simpler to have some room for experimentation before adding more advanced path logic?
We already have the logic in place that does this, but only for components
opentelemetry-collector/confmap/merge.go
Lines 20 to 30 in a33fdf3
patterns := []string{ | |
"service::extensions", | |
"service::**::receivers", | |
"service::**::exporters", | |
} | |
var globs []glob.Glob | |
for _, p := range patterns { | |
if g, err := glob.Compile(p); err == nil { | |
globs = append(globs, g) | |
} | |
} |
TL;DR;
- It constructs globs.
- We flatten the map and go through each key and match it against glob.
- If a match is found, we merge the lists.
- Else, we continue and don't touch the unmatched key
- In this example, we will merge the list of extensions and receivers from pipeline, excluding lists in the rest of the config. | ||
- `otelcol --config main.yaml --config ext.yaml?merge_paths=service::extensions --config rec.yaml?merge_paths=service::**::receivers` | ||
- In this example, we will merge all list of extensions from `ext.yml` and list of receivers from `rec.yaml`, excluding lists in the rest of the config. | ||
2. `merge_mode`: One of `prepend` or `append`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably need more options. For example, if I want to add a receiver in a merging config while it may be already defined in the base config. Also, we need the default replace
behavior. What about the following list:
append
: append entries to the listappend_unique
: append only unique entries to the listprepend
: insert entries in front of the listprepend_unique
: insert only unique entries in front of the list
replace
(default): overwrite the list
keep
: keep the original list
Also, I would call the options list_merge_mode
and list_merge_paths
to make it clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmitryax I see. I'll update the RFC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you touch a good point there with the _unique
modifiers, @dmitryax. Shouldn't both append and prepend always ensure lists containing only "basic types" (strings, ints, etc) do not have duplicated values? I don't know about any scenario in the Collector involving lists of basic types where duplicated values wouldn't be a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For service components, duplicates will cause error.
I'm not sure about other places though.
1. `merge_paths`: A comma-separated list of glob patterns which will be used while config merging | ||
- This setting will control the paths user wants to merge from the given config. | ||
- Example: | ||
- `otelcol --config main.yaml --config extra.yaml?merge_paths=service::extensions,service::**::receivers` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using the query params is fine. However, we need to keep in mind that some confmap providers use the same pattern to pass extra information. With this approach, they won't be able to use these keys which is probably fine.
If anyone else have any other ideas, please share.
cc @open-telemetry/collector-approvers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, we can extract the parameters here
opentelemetry-collector/confmap/resolver.go
Line 176 in 6aa2b81
for _, uri := range mr.uris { |
Then we can remove our parameters from URI and let resolver do its job:
u, _ := url.Parse("otel.yaml?key=val&merge_mode=append")
q := u.Query()
q.Del("merge_mode")
u.RawQuery = q.Encode()
// uri will now be otel.yaml?key=val&key2=val2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed this in this RFC and came up with a few issues with query parameters that we'll want to consider before going that route.
@dmitryax I believe you, @mx-psi, and I had an offline conversation at KubeCon and tacitly agreed we should go the route of configuring these inside a config file (this option in the RFC). Do you still think we should go this route? I think we should probably at least explore it before deciding on an approach.
I haven't had time to look into this proposal in detail but one thing I would like to see explored is whether we can use YAML custom tags https://yaml.org/spec/1.2.2/#tags to specify this in the YAML itself per-array |
@mx-psi Thanks for sharing your thoughts! I'll surely go the documentation and see if they can be integrated. I'm all in if we can get it work through just yaml. |
@mx-psi I had a brief look at yaml tags and their support in golang. Here are my thoughts:
var node Node
err := yaml.Unmarshal(data, &node)
// recursively go through the node and its children to find the nodes with tags and store their path.
Here are some initial pros and cons that come to mind:
Let me know your thoughts! |
Description
This RFC is a follow-up of #12097. The first PR introduced the feature gate to merge the components' lists and left out the options to configure the merging behaviour.
This RFC proposes an approach to extend the current behaviour by enabling merging of specified config parts and support different modes.
Link to tracking issue
Relates:
Thanks to @mx-psi @dmitryax and @evan-bradley for their feedback on the first PR!!