[cmd/opampsupervisor] config not updated in opampSupervisor/opampExtension when processor pipeline already exists

[keisukesakasai]

Component(s)

cmd/opampsupervisor

What happened?

Description

I am using the OpenTelemetry Collector Contrib cmd/opampsupervisor to run the Supervisor and the OTel Collector.
https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/cmd/opampsupervisor

To add additional configurations to the OTel Collector, I set the following in extraconfig.yaml:

receivers:
  otlp:
    protocols:
      grpc:
        endpoint: 0.0.0.0:4317

exporters:
  debug:

  otlp:
    endpoint: localhost:4317
    tls:
      insecure: true

processors:
  batch:
        
service:
  pipelines:
    traces:
      receivers: [otlp]
      exporters: [debug, otlp]
    metrics:
      receivers: [otlp]
      processors: [batch]
      exporters: [otlp]    
  telemetry:
    logs:
    ...

With this configuration, I send the following config via OpAMP from the OpAMP Backend:

processors:
  batch:
  attributes:
    actions:
      - key: hoge
        value: piyo
        action: insert
service:
  pipelines:
    traces:
      processors:
        - attributes

This works perfectly.

Next, I update the extraconfig.yaml to include the batch processor in the trace pipeline beforehand:

receivers:
  otlp:
    protocols:
      grpc:
        endpoint: 0.0.0.0:4317

exporters:
  debug:

  otlp:
    endpoint: localhost:4317
    tls:
      insecure: true

processors:
  batch:
        
service:
  pipelines:
    traces:
      receivers: [otlp]
      processors: [batch] # <==================== add
      exporters: [debug, otlp]
    metrics:
      receivers: [otlp]
      processors: [batch]
      exporters: [otlp]    
  telemetry:
    logs:
    ...

However, when I send the same config from the OpAMP Backend, the config (effective.yaml) is not updated.
Is this behavior as intended?

Steps to Reproduce

Experimenting with the supervisor
https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/cmd/opampsupervisor#experimenting-with-the-supervisor

Expected Result

In both cases, the config is updated.

Actual Result

In the following case, the config is not updated, and the OTel Collector is not restarted.

Collector version

v0.113.0

Environment information

Environment

OpenTelemetry Collector configuration

No response

Log output

No response

Additional context

No response

[github-actions]

Pinging code owners:

• cmd/opampsupervisor: @evan-bradley @atoulme @tigrannajaryan

See Adding Labels via Comments if you do not have permissions to add labels yourself.

[bacherfl]

I also could reproduce that behaviour using the instructions provided in the issue description. I believe the reason this is happening is because of the merging behavior of koanf used internally, as list values are overriden during the merge. When receiving an updated config via the opamp server, the current config is applied last, in which case this will override any list values defined in the additional configuration - in this case the already existing processor list of the traces pipeline. There is some logic already to handle this specifically for extensions:

opentelemetry-collector-contrib/cmd/opampsupervisor/supervisor/supervisor.go

Lines 1411 to 1430 in 97659b5

	// The default koanf behavior is to override lists in the config.
	// Instead, we provide this function, which merges the source and destination config's
	// extension lists by concatenating the two.
	// Will be resolved by https://github.com/open-telemetry/opentelemetry-collector/issues/8754
	func configMergeFunc(src, dest map[string]any) error {
	srcExtensions := maps.Search(src, []string{"service", "extensions"})
	destExtensions := maps.Search(dest, []string{"service", "extensions"})
	maps.Merge(src, dest)
	if destExt, ok := destExtensions.([]any); ok {
	if srcExt, ok := srcExtensions.([]any); ok {
	if service, ok := dest["service"].(map[string]any); ok {
	service["extensions"] = append(destExt, srcExt...)
	}
	}
	}
	return nil
	}

QQ to the code owners (@evan-bradley @atoulme @tigrannajaryan) - should this logic also be applied to the traces/metrics/logs pipelines, i.e. the receivers, processors and exporters lists?

[evan-bradley]

I think we could probably extend the merging logic to pipeline components, but there are a few things we need to consider if we do so:

• Adding this for processors, where order matters, has implications for the shape of data in pipelines (probably the biggest risk).
• The only reason we have this right now is just for the OpAMP extension, and extensions are "global" within the service, as opposed to pipelines, which are a bit more localized. Users can use the fact that they can create multiple pipelines to work around this for processors, which I will detail below.
• We're adding more exceptions to the current rule of overriding lists, and users would need to understand this.
• Everywhere we override the default behavior for resolving conflicts for lists at the same key in multiple configs makes an impactful change to user's ability to override lists instead of merging them.

I think that we should document whatever choice we take in the Supervisor readme.

@keisukesakasai You could work around this by doing something like the following:

extraconfig.yaml:

service:
  pipelines:
    traces/in:
      receivers: [otlp]
      processors: [filter]
      exporters: [forward]   
    traces/out:
      receivers: [forward]
      # extra processors go here
      exporters: [debug, otlp]

remote config:

service:
  pipelines:
    traces/out:
      processors: [transform]

This allows us to simply merge maps instead of having to figure out how to merge lists. Additional traces pipelines could be used to put the processors in more specific locations, for example before or in the middle of processing done in the extra config pipelines. Something similar could be done with receivers or exporters.