-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[confmap] - new feature flag for append merging strategy #12097
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
Conversation
3ac6d58 to
9059b29
Compare
|
@Aneurysm9 @evan-bradley @mx-psi can you take another look? i've removed feature flag and added command line argument to control the paths user wants merged. I'll work on the documentation once we've established a common ground. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12097 +/- ##
==========================================
+ Coverage 92.02% 92.04% +0.01%
==========================================
Files 469 470 +1
Lines 25318 25372 +54
==========================================
+ Hits 23299 23353 +54
Misses 1612 1612
Partials 407 407 ☔ View full report in Codecov by Sentry. |
7b5fe09 to
5685baa
Compare
mx-psi
left a comment
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.
Thanks for taking this Vihas! I have two comments:
- I think this is the kind of thing that is hard to get right on the first try, so I would want to do this under some sort of experimental flag.
- I would like to do this if possible with little to no modification of stable confmap APIs. confmap is 1.0, it is a very important piece of the Collector, and if we get it wrong it would be quite difficult to reverse course.
|
@mx-psi Thanks for your comments!
|
0f66985 to
95913fd
Compare
dmitryax
left a comment
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.
LGTM
|
Please update the nolint comment to fix CI |
|
A few of test cases are failing but they seem unrelated to my PR. |
e32eaa2 to
62ebea5
Compare
|
@mx-psi I have fixed the the go.mod and I'm not reverting |
…12926) Follow-up of #12097 <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> ### Description Ideally, users want to merge lists in pipelines only. In this PR, I'm adding support for merging following components: 1. extensions 2. receivers 3. exporters We make use of glob-like pattern to match the path because there can be multiple pipelines configured. Inspired by @evan-bradley's [idea](#12097). I'm leaving `processors` out of this list because we make an ordering guarantee. #### _**Future plan:**_ Once this PR gets merged, it would make it easier for us to decide upon the command line flags to make this option configurable over the RFC. Right now, `patterns` is hardcoded to include above mentioned components, but we can easily make it configurable after making some tweaks. <!-- Issue number if applicable --> ### Link to tracking issue Relates #8754 <!--Describe what testing was performed and which tests were added.--> ### Testing Expanded the existing test cases <!--Describe the documentation added.--> #### Documentation
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### 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. <!-- Issue number if applicable --> #### Link to tracking issue Relates: - #8754 - #8394 - #10370 Thanks to @mx-psi @dmitryax and @evan-bradley for their feedback on the [first PR](#12097)!! --------- Co-authored-by: Pablo Baeyens <[email protected]> Co-authored-by: Evan Bradley <[email protected]> Co-authored-by: Hélia Barroso <[email protected]>
Koanf's default merging strategy currently overrides static values such as slices, numbers, and strings. However, lists of components should be treated as a special case. This pull request introduces a new command line option to allow for merging lists instead of discarding the existing ones.
With this new merging strategy:
Link to tracking issue
Related issues:
Testing
Documentation
Example
Consider the following configs,
If you run the collector with following command,
then the final configuration after config resolution will look like following:
For backward compatibly, the default behaviour is not to merge lists. Users who want to explicitly merge lists can enable the command line option.
Note: I’d appreciate your feedback on this 🙏