Skip to content

[cmd/opampsupervisor] Fix merging lists when using local config #39947

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

Merged

Conversation

evan-bradley
Copy link
Contributor

@evan-bradley evan-bradley commented May 8, 2025

Description

With #38671, we supply the local config to the Collector in two places:

  1. The effective (merged) config
  2. A list of files passed through --config flags to the Collector.

While the second approach is preferable in the long term, we have to manually merge the config for now since the Collector doesn't support merging lists in config without a feature gate enabled. This PR therefore opts for the first option until we enable this feature gate.

Fixes #39931

Copy link
Member

@douglascamata douglascamata left a comment

Choose a reason for hiding this comment

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

I think this PR is useful. But it's solving a different problem than what's stated, imho.

@douglascamata
Copy link
Member

douglascamata commented May 8, 2025

@evan-bradley we might also want to change the priority order for merging configuration:

// Merge own metrics config.
ownMetricsCfg, ok := s.agentConfigOwnMetricsSection.Load().(string)
if ok {
if err = k.Load(rawbytes.Provider([]byte(ownMetricsCfg)), yaml.Parser(), koanf.WithMergeFunc(configMergeFunc)); err != nil {
return false, err
}
}
// Merge local config last since it has the highest precedence.
if err = k.Load(rawbytes.Provider(s.composeExtraLocalConfig()), yaml.Parser(), koanf.WithMergeFunc(configMergeFunc)); err != nil {
return false, err
}
if err = k.Load(rawbytes.Provider(s.composeOpAMPExtensionConfig()), yaml.Parser(), koanf.WithMergeFunc(configMergeFunc)); err != nil {
return false, err
}
if err = k.Load(rawbytes.Provider(s.composeAgentConfigFiles()), yaml.Parser(), koanf.WithMergeFunc(configMergeFunc)); err != nil {
return false, err
}

This doesn't seem right to me. Local configuration (from composeAgentConfigFiles) is being merged last (on top of everything), so it can overwrite the built-in/internal configuration (from composeExtraLocalConfig, which I think we should rename to avoid confusion), the opamp configuration (from composeOpAMPExtensionConfig), and also own metrics configuration (from ownMetricsCfg).

I think local configuration should be the very first to be merged, as they shouldn't override any other configuration.

@evan-bradley
Copy link
Contributor Author

@douglascamata Thanks for taking such a close look. I agree that we should probably make local configuration the lowest priority when merging, but the reason this was done like this is described here: #38671 (comment). I think that comment also makes some good points, and in my opinion points to the idea that this should be configurable.

For now I'd like to leave the ordering as-is and fix it in its own PR where we can consider it independently of this issue.

@douglascamata
Copy link
Member

douglascamata commented May 8, 2025

@evan-bradley sounds good to me. My use case for wanting to have local configuration being the lowest priority is because I want to use it as a "base layer" configuration to allow the Supervisor to start Collectors even without being able to talk to the upstream OpAMP backend.

I don't want to have a default remote configuration that is required to be delivered to all Supervisor before Collectors can start.

@evan-bradley evan-bradley merged commit 929656d into open-telemetry:main May 8, 2025
174 checks passed
@github-actions github-actions bot added this to the next release milestone May 8, 2025
@douglascamata
Copy link
Member

For the record, I created an issue for the use cas I mentioned of local configuration with the lowest priority (aka base layer config): #39963

dragonlord93 pushed a commit to dragonlord93/opentelemetry-collector-contrib that referenced this pull request May 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[opampsupervisor] Local config with extensions overwriting opamp extension
4 participants