Skip to content
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

Create tool or subcommand that allows migrating configuration #7631

Open
2 tasks
mx-psi opened this issue May 4, 2023 · 14 comments
Open
2 tasks

Create tool or subcommand that allows migrating configuration #7631

mx-psi opened this issue May 4, 2023 · 14 comments
Assignees
Labels
area:config discussion-needed Community discussion needed enhancement New feature or request priority:p1 High

Comments

@mx-psi
Copy link
Member

mx-psi commented May 4, 2023

Is your feature request related to a problem? Please describe.

Changes in recommended configuration happen frequently in the Collector, including both Collector-wide changes (e.g. see #7111 which is the main motivation for this) and in individual components (e.g. most recently see open-telemetry/opentelemetry-collector-contrib/issues/18642, see also #6334 for a change that probably affected many configurations). They are a normal part of component stabilization and will continue happening across the Collector.

Currently, we rely on warnings, updated examples and documentation to reflect our recommendation and expect users to check these and then upgrade their configuration manually. Our current approach probably means that only a small number of users migrate to the new configuration, and this makes it hard for us to drop old syntax/configuration flags without hurting many users. It also makes it hard to come to a consensus in issues like #5686.

Describe the solution you'd like

Add a subcommand that automates as much as possible of any given configuration migration. Ideally, the user passes the configuration in the same way they would do it for a normal Collector run and the Collector then prints out the updated configuration as well as a list of the changes that were made and their rationale.

Describe alternatives you've considered

An alternative is to provide a separate tool for this. This would work for the ${...} -> ${env:..} migration, but would make it hard for components to introduce custom migration rules.

Additional context

Note: This section lists some requirements of a fully working solution. While we should consider all of these when designing a first solution, we don't have to support all of them, and an MVP should leave things like custom component migration out of scope for now.

Some requirements

The solution should:

  1. Allow adding support for components providing custom migration rules (similar to what we do for unmarshaling and validation). We should take this into account in the design phase but leave implementation explicitly out of the first version of this subcommand.
  2. Work at the confmap level, at least for the ${..} to ${env:...} migration. It probably is useful also for components to work at the confmap level.
  3. Provide output that is as close as possible to the way the user provided the configuration. With the current implementation it's hard to map back a given configuration to the individual files/environment variables/other sources where it comes from, so doing this 'the right way' means adding some sort of metadata to confmap.Conf noting the source of each configuration section/scalar value.

Open questions

  1. How should we provide the output? If configuration comes from multiple sources, should we map back to these sources? How do we do this given merging happens very early? We can't modify user input in some cases (protected files, env variables, URLs), so should we just print things out to stdout?
  2. Should components be able to recommend configuration changes outside of their configuration section? This could be useful for situations like [Proposal] Move batching to exporterhelper #4646, where we want to move configuration from one component to another.
  3. For some configuration changes, we may not be able to fully automate the process no matter how clever we are (e.g. if we drop some functionality because it is moved to the SDKs). What should we do in these cases? Should we have support for warnings/lints? This functionality would also be useful for Log a warning when an environment variable without a value is expanded #5615.

Some follow up tasks once we have this

  • Add this to our CI to run on examples for dogfooding
  • Add warnings in logs to nudge users to migrate to the new configuration (e.g. by doing a dry-run and checking if there are changes reported)
@mx-psi mx-psi added enhancement New feature or request discussion-needed Community discussion needed priority:p1 High area:config labels May 4, 2023
@mx-psi
Copy link
Member Author

mx-psi commented May 4, 2023

cc @Aneurysm9 @bogdandrutu because of #7111, also cc @TylerHelmuth to review requirements/needs for the open-telemetry/opentelemetry-collector-contrib/issues/18642 example (this is probably also useful for the long term transform processor work).

To be clear one more time, I want to get all requirements/desiderata for use cases related to this first so we consider them during the design phase, even if the subcommand does not do everything we would want it to at first.

@TylerHelmuth
Copy link
Member

This sounds great and will be a big deal for open-telemetry/opentelemetry-collector-contrib#18643 as a whole. I have started work on a bridge between the old configuration and OTTL and it would be great to be able to convert user's configs for them instead of only printing out the equivalent statement and hoping they update.

@mx-psi
Copy link
Member Author

mx-psi commented May 10, 2023

Thinking about this a bit more, the biggest open question is what the output should be. Should we have something like this?

file:path/to/file/one
---
# Contents of file path/to/file/one go here
---
env:SOME_ENV
---
# Contents of environment variable SOME_ENV go here
---
...

How would that work if and when we add support for custom migration rules from components?

@kentquirk
Copy link
Member

It so happens that I'm working on a tool right now for my product that does configuration conversion in a way that maintains existing choices but allows reconfiguration.

The strategy I'm using is to load the existing config into a generic map[string]any. The output file is a Go text/template that expresses the new configuration, and contains references to existing config values (included in the template as .Data) where appropriate.

An example from the template showing a renamed config item is:

Logging:
    ## HoneycombAPI is the URL for the upstream Honeycomb API for logs; this
    ## is the destination to which refinery sends its own logs. Only used if
    ## the logging type is "honeycomb". 
    ## 
    ## Not eligible for live reload.
    {{ nonDefaultOnly .Data "HoneycombAPI" "HoneycombLogger.LoggerHoneycombAPI" "https://api.honeycomb.io" }}

nonDefaultOnly is defined as:

func nonDefaultOnly(data map[string]any, key, oldkey string, def any) string {
...
}

Its behavior is that the item is commented out if the original value was either missing or equivalent to the default; otherwise it's included with the original value of this field (but now it has a new name).

This function is an example of one of several special-purpose functions for our particular conversion needs. The value is that the conversion code is quite generic -- most of the work is in the template and the helper functions the template is given.

The disadvantage is that it's very "meta" -- coding in templates requires thinking about the template logic at a different level than the template support code. It can get confusing to write, nevermind read.

However, because templates are so expressive, this model could allow components to include templates for converting component configurations to a new format without having to consider the larger file within which they're contained, and the tool could know to look for those in a well-defined place.

Independent of the stuff above, I do strongly recommend that:

  • The config file itself should be versioned going forward so that future converters know what they're trying to load and can stamp what they're writing.
  • Any components that need a converter should also be required to include a version within that component's config in all future configurations for the same reason.
  • It wouldn't hurt for the config to include a "minimum supported version" of the collector to which it applies. This would allow a collector to avoid trying to load a config it won't understand.

@kentquirk
Copy link
Member

To follow up from my comment above, we deployed this last summer and have had very good results. I'd be happy to talk about whether there's a way to extract it for collector use.

@atoulme
Copy link
Contributor

atoulme commented Jan 24, 2024

I wasn't aware this issue existed, linking a proposal to donate our configconverter module: open-telemetry/opentelemetry-collector-contrib#30654

@dmitryax
Copy link
Member

The converter implementation is part of the core API already https://github.com/open-telemetry/opentelemetry-collector/blob/main/confmap/converter.go.

Do we really need a separate subcommand or we can jus utilize it instead?

@mx-psi
Copy link
Member Author

mx-psi commented Jan 25, 2024

One thing that is unclear to me with the converter approach is if we are encouraging people to change their actual configuration files. At least before 1.0, if we are to remove old functionality users need to actually change their configuration files. The way to push for that IMO is to either offer the option to modify the files for them (if they are using files and these are writable) or, if that's not possible, offer users something that they can copy-paste

@dmitryax
Copy link
Member

Applying conversions to migrate configuration to the new version should probably be opt-in; maybe we can use another flag for that. We can also have a flag to show the final configuration without starting the collector. Applying those two options would provide pretty much the same behavior as described in the issue

@songy23
Copy link
Member

songy23 commented Jan 26, 2024

So it sounds like we still want the migration functionality, but not with a subcommand, rather 2 flags to ./otelcolcore. Something like ./otelcolcore --auto-migrate --dry-run

Is this a fair summary? @dmitryax

@TylerHelmuth
Copy link
Member

@mx-psi is this feature required for 1.0 or could it be added after?

@mx-psi mx-psi moved this to Todo in Collector: v1 Apr 18, 2024
@mx-psi
Copy link
Member Author

mx-psi commented Apr 19, 2024

@TylerHelmuth The intent of having this on 1.0 was to help people migrate from $ENV to ${ENV}. I guess whether we need this for 1.0 depends on what amount of people we expect would have to do the migration. We don't need to build this tool specifically for it, but having some sort of helper would be very helpful in the transition to 1.0 IMO

@TylerHelmuth
Copy link
Member

@mx-psi with the new error logs for $ENV and the ongoing work to move $ENV expansion behind a feature gate with eventual plans of deprecation I feel we can move this feature out of scope of 1.0.

@mx-psi
Copy link
Member Author

mx-psi commented Jun 11, 2024

@TylerHelmuth Oki, I am fine with that.

I think this continue to be a high priority issue, so I am going to leave p1 there, but since the impact is smaller given that we are going to keep supporting ${ENV}, we can focus on this before moving the feature gate that drops support for $ENV stable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:config discussion-needed Community discussion needed enhancement New feature or request priority:p1 High
Projects
None yet
Development

No branches or pull requests

6 participants