Empty objects as component config doesn't work anymore

[dmitryax]

Describe the bug The collector is failing to start if an empty object provided as a config for any of the components. For example

extensions:
  health_check: {}

The collector fails to start with the following message:

2020-10-02T23:05:49.275-0700    INFO    service/service.go:298  Loading configuration...
Error: cannot load configuration: Service references extension "health_check" which does not exist
2020/10/02 23:05:49 application run finished with error: cannot load configuration: Service references extension "health_check" which does not exist

Empty values still work as expected:

extensions:
  health_check:

Steps to reproduce Set an empty object as a config for any of the components. For example

extensions:
  health_check: {}

What did you expect to see? Support empty objects the same way as empty values.

What did you see instead? Collector failing to start.

What version did you use? Version: 035aa5cf6c92678c3f8362d6916e18444a8250fb

Additional context Empty objects are pretty common in k8s world https://github.com/open-telemetry/opentelemetry-collector/blob/master/examples/k8s/otel-config.yaml#L35.

Also currently helm chart has to use empty objects in order to have config overrides working. In Helm empty/null value in object overrides removes the key from an object.

[bogdandrutu]

Unfortunately I cannot reproduce this in tests, can you post a full config? One other thing that is important is the name of the config file.

[dmitryax]

here is the config example I used (just added {} to health_check in examples/local/otel-config.yaml):

extensions:
  health_check: {}
  pprof:
    endpoint: 0.0.0.0:1777
  zpages:
    endpoint: 0.0.0.0:55679

receivers:
  otlp:
    protocols:
      grpc:
      http:

  opencensus:

  jaeger:
    protocols:
      grpc:
      thrift_binary:
      thrift_compact:
      thrift_http:

  zipkin:

  # Collect own metrics
  prometheus:
    config:
      scrape_configs:
        - job_name: 'otel-collector'
          scrape_interval: 10s
          static_configs:
            - targets: [ '0.0.0.0:8888' ]

processors:
  batch:

exporters:
  logging:
    logLevel: debug

service:

  pipelines:

    traces:
      receivers: [otlp, opencensus, jaeger, zipkin]
      processors: [batch]
      exporters: [logging]

    metrics:
      receivers: [otlp, opencensus, prometheus]
      processors: [batch]
      exporters: [logging]

  extensions: [health_check, pprof, zpages]

[bogdandrutu]

@dmitryax identified the culprit https://github.com/open-telemetry/opentelemetry-collector/pull/1640

[dmitryax]

Nice! Thanks @bogdandrutu! Should we ask @pavolloffay to fix this?

[pavolloffay]

Guys, I won't have time to have a look at it next week. If it's urgent we could revert the set flag feature for now.

[bogdandrutu]

@pavolloffay I can take care of this if you explain the comment better:

// The configuration is being loaded from two places: the config file and --set command line flag. // The --set flag implementation (AddSetFlagProperties) creates a properties file from the --set flag // that is loaded by a different viper instance. // Viper implementation of v.MergeConfig(io.Reader) or v.MergeConfigMap(map[string]interface) // does not work properly. // The workaround is to call v.Set(string, interface) on all loaded properties from the config file // and then do the same for --set flag in AddSetFlagProperties.

[bogdandrutu]

I am not sure I understand:

1. why need to call set for all loaded properties initially?
2. what is the problem with Merge?

[pavolloffay]

Merge didn't work properly, I don't remember the exact issue but we could not make it work with @jrcamp.

Set sets a property to override map which is necessary because we call set afterwards with the properties from the --set flag to simulate merge.

[tigrannajaryan]

Let's revert https://github.com/open-telemetry/opentelemetry-collector/pull/1640 for now. I created a reverting PR: https://github.com/open-telemetry/opentelemetry-collector/pull/1905

@pavolloffay when you have time please take a look at how to fix this correctly while keeping the setflag functionality.

[jrcamp]

I am not sure I understand:

1. why need to call set for all loaded properties initially?
2. what is the problem with Merge?

IIRC, when using MergeInConfig viper.UnmarshalKey wasn't working correctly after merging but I could be wrong.