Skip to content

[cmd/opampsupervisor] Make configuration merge more customizable through special config files #40295

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.

Sign up for GitHub

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

Jump to bottom
Merged
evan-bradley merged 36 commits into from
Jul 7, 2025
Merged

[cmd/opampsupervisor] Make configuration merge more customizable through special config files #40295

evan-bradley merged 36 commits into from
Jul 7, 2025

Conversation

@douglascamata
Copy link
Member

@douglascamata douglascamata commented May 26, 2025 •
edited
Loading

Description

This PR refactors the logic of the configuration loading and merging in the cmd/opampsupervisor package 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

  • Put very good notes on the changelog.
  • Update the specification docs.

@douglascamata douglascamata requested review from a team, atoulme and evan-bradley as code owners May 26, 2025 18:35
@github-actions github-actions bot requested a review from tigrannajaryan May 26, 2025 18:35
@douglascamata douglascamata mentioned this pull request May 27, 2025
Closed
@douglascamata douglascamata mentioned this pull request May 28, 2025
Closed
@atoulme
Copy link
Contributor

atoulme commented May 29, 2025

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.

@douglascamata
Copy link
Member Author

douglascamata commented May 29, 2025

Any chance to avoid the term magic?

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?

Would you please describe the intent, it looks like you have enough design going on a small RFC would help.

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?

TylerHelmuth
TylerHelmuth reviewed May 30, 2025
View reviewed changes
evan-bradley
evan-bradley reviewed May 30, 2025
View reviewed changes
Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@douglascamata
Copy link
Member Author

douglascamata commented Jun 2, 2025

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:

  • $OWN_METRICS_CONFIG
  • $BUILTIN_CONFIG
  • <USER_PROVIDED_CONFIG_FILES>
  • $OPAMP_EXTENSION_CONFIG
  • $REMOTE_CONFIG

@douglascamata douglascamata changed the title [cmd/opampsupervisor] Make configuration merge more customizable through magic config files [cmd/opampsupervisor] Make configuration merge more customizable through special config files Jun 2, 2025
@douglascamata
Add test case for special config file validation
e9b5276 
Signed-off-by: Douglas Camata <[email protected]>
@douglascamata
Add a few test cases for Supervisor.addSpecialConfigFiles()
ce6b08a 
Signed-off-by: Douglas Camata <[email protected]>
@douglascamata
Merge branch 'main' of github.com:open-telemetry/opentelemetry-collec…
343812f 
…tor-contrib into supervisor-config-file-revamp
@douglascamata
Retrigger build due to weird failure
e3eefa6 
Failure link: https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/16021365184/job/45198938497?pr=40295

Exact failure message:

```
panic: failed to create SDK: binding address 127.0.0.1:38499 for Prometheus exporter: listen tcp 127.0.0.1:38499: bind: address already in use
```
@douglascamata
Copy link
Member Author

douglascamata commented Jul 2, 2025

@evan-bradley thanks again for another great review. Everything was taken care off. Please have another look when you can.

@evan-bradley
Copy link
Contributor

evan-bradley commented Jul 2, 2025 •
edited
Loading

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?

@douglascamata Sorry I missed this before. I think this part can be implemented separately from the rest of this PR. It relates to the work in this RFC, though we may be able to just use the existing feature gate without any issues. I haven't written this down anywhere, I was just thinking we should do it because we want to rely on the Collector to handle its own configuration as much as possible.

evan-bradley
evan-bradley reviewed Jul 2, 2025
View reviewed changes
evan-bradley
evan-bradley reviewed Jul 3, 2025
View reviewed changes
@douglascamata
Fix changelog for breaking change
a40349d 
Signed-off-by: Douglas Camata <[email protected]>
@douglascamata
Rename changelog file
c33d92f 
Signed-off-by: Douglas Camata <[email protected]>
@douglascamata
Merge OWN_TELEMETRY_CONFIG into BUILTIN_CONFIG
bfd4eb4 
Signed-off-by: Douglas Camata <[email protected]>
@douglascamata
Rename built-in special config file
d0b4864 
Signed-off-by: Douglas Camata <[email protected]>
@douglascamata
Renames for consistency (extra config -> extra telemetry config, …
07d6db3 
…`own metrics` -> `own telemetry` )

Signed-off-by: Douglas Camata <[email protected]>
@douglascamata
Delete old extraconfig.yaml file
ceca3ef 
Signed-off-by: Douglas Camata <[email protected]>
evan-bradley
evan-bradley reviewed Jul 6, 2025
View reviewed changes
Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More or less looks good to me, just two questions.

@douglascamata
Rename konf to conf
56078a1 
Signed-off-by: Douglas Camata <[email protected]>
evan-bradley
evan-bradley approved these changes Jul 7, 2025
View reviewed changes
Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @douglascamata!

If you can fix the merge conflict, this looks good to me.

@douglascamata
Merge branch 'main' of github.com:open-telemetry/opentelemetry-collec…
d34f600 
…tor-contrib into supervisor-config-file-revamp

Signed-off-by: Douglas Camata <[email protected]>
@douglascamata
Copy link
Member Author

douglascamata commented Jul 7, 2025

@evan-bradley conflict fixed. Thanks for bearing with me all this time. 🥂

@evan-bradley evan-bradley merged commit 46ca6d0 into open-telemetry:main Jul 7, 2025
177 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 7, 2025
This was referenced Jul 8, 2025
Merged
Closed
Dylan-M pushed a commit to Dylan-M/opentelemetry-collector-contrib that referenced this pull request Aug 5, 2025
@douglascamata @evan-bradley @Dylan-M
[cmd/opampsupervisor] Make configuration merge more customizable thro…
33a363a 
…ugh special config files (open-telemetry#40295)

Co-authored-by: Evan Bradley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TylerHelmuth TylerHelmuth TylerHelmuth left review comments

@evan-bradley evan-bradley evan-bradley approved these changes

@atoulme atoulme Awaiting requested review from atoulme atoulme is a code owner

@tigrannajaryan tigrannajaryan Awaiting requested review from tigrannajaryan tigrannajaryan is a code owner

Assignees

@TylerHelmuth TylerHelmuth

Labels

cmd/opampsupervisor

Projects

None yet

Milestone

v0.131.0

Development

Successfully merging this pull request may close these issues.

[cmd/opampsupervisor] Add support for "base layer" local config for the Collector

4 participants

@douglascamata @atoulme @evan-bradley @TylerHelmuth