Fix --set flag

[joe-elliott]

Description:
Re-adds the --set parameter in a way that works with {} being used in configs.

Link to tracking Issue:
Fixes #1907

Testing:
The original tests were added along with the addition of the following:

extensions:
  health_check: {}

This failed the previous tests, but now passes.

Documentation:

[codecov]

Codecov Report

Merging #2162 (9720e4c) into master (7c28105) will decrease coverage by 0.02%.
The diff coverage is 76.92%.

@@            Coverage Diff             @@
##           master    #2162      +/-   ##
==========================================
- Coverage   91.97%   91.94%   -0.03%     
==========================================
  Files         271      272       +1     
  Lines       15653    15675      +22     
==========================================
+ Hits        14397    14413      +16     
- Misses        854      857       +3     
- Partials      402      405       +3     

Impacted Files	Coverage Δ
service/service.go	52.67% <50.00%> (-0.23%)	⬇️
service/set_flag.go	80.95% <80.95%> (ø)
config/config.go	98.87% <100.00%> (ø)
internal/data/traceid.go	84.61% <0.00%> (-7.70%)	⬇️
internal/data/spanid.go	100.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c28105...9720e4c. Read the comment docs.

[tigrannajaryan]

@pavolloffay can you review please?

[pavolloffay]

Here is the revert #1905 and the original PR https://github.com/open-telemetry/opentelemetry-collector/pull/1640/files

[pavolloffay]

@joe-elliott could you highlight what fixed the issue?

[joe-elliott]

@pavolloffay
Previously it was iterating through .AllKeys() in the config file viper and calling .Set() on each one to force them into viper's config "override". This allowed the --set values to nicely merge with the config values. However, .AllKeys() just failed to return a key with an empty object like : healthcheck: {} and so it was never set.

By iterating through all keys and only calling .Set on the root keys we are preserving the entire viper config object faithfully.
https://github.com/open-telemetry/opentelemetry-collector/pull/2162/files#diff-2a9451c3c8352620ac89fc278d2e47f08ce3fa76d427cb1694ff8119003caa9fR68-R77

I also attempted other changes like .MergeConfig() and can confirm they will not work for this.

[joe-elliott]

To continue this conversation. If your file looks like this:

a: {}
b:
  c: 2
  d:

If you load this with viper and run AllSettings() you will get the following structure back:

b:
  c: 2

If you run AllKeys() and log each one you get the following keys:

b.c
b.d

Viper does not enumerate keys that are empty objects but it will enumerate nil keys. By calling Get() on the root keys only we are able to faithfully reconstruct the original config in the viper overrides so it will nicely merge with the --set params.

@pavolloffay thoughts?

[joe-elliott]

@jpkrohling do you mind taking a look?

[tigrannajaryan]

@pavolloffay
Previously it was iterating through .AllKeys() in the config file viper and calling .Set() on each one to force them into viper's config "override". This allowed the --set values to nicely merge with the config values. However, .AllKeys() just failed to return a key with an empty object like : healthcheck: {} and so it was never set.

By iterating through all keys and only calling .Set on the root keys we are preserving the entire viper config object faithfully.
https://github.com/open-telemetry/opentelemetry-collector/pull/2162/files#diff-2a9451c3c8352620ac89fc278d2e47f08ce3fa76d427cb1694ff8119003caa9fR68-R77

I also attempted other changes like .MergeConfig() and can confirm they will not work for this.

To continue this conversation. If your file looks like this:

a: {}
b:
  c: 2
  d:

If you load this with viper and run AllSettings() you will get the following structure back:

b:
  c: 2

If you run AllKeys() and log each one you get the following keys:

b.c
b.d

Viper does not enumerate keys that are empty objects but it will enumerate nil keys. By calling Get() on the root keys only we are able to faithfully reconstruct the original config in the viper overrides so it will nicely merge with the --set params.

@pavolloffay thoughts?

@joe-elliott these ^^^^ are very nice explanations. Would you be able to add them as comments to the relevant parts of the code? Perhaps drop the "previously" part and keep the part that describes how it works now and most importantly why it works correctly.

[jpkrohling]

LGTM. There are a couple of minor improvements that could be made, but those can be safely ignored...

[jpkrohling]

From what I understood, it's still OK to have nil here instead of empty. If that's the case, keep this as nil, as a proof that this change didn't break compat with previous versions.

[joe-elliott]

Yes, nil is fine here as well as empty object. The problem is that empty object was not working and was not being tested for. With these changes this file now tests both empty object and nil object:

...
extensions:
  health_check: {}                  <- empty object test
  pprof:                                  <- nil test
...

[joe-elliott]

I believe I've addressed all comments and improved the in code comments regarding the behavior of viper and why we are setting the root keys.

I am getting a failed contribtest:

--- FAIL: TestNewLogsExporter (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xbde2df]

Could this be due to changes I made? should I dig into this?

[tigrannajaryan]

I am getting a failed contribtest:

--- FAIL: TestNewLogsExporter (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xbde2df]

Could this be due to changes I made? should I dig into this?

Unlikely that your change could cause this. Looks like a bug in TestNewLogsExporter.

[tigrannajaryan]

@joe-elliott please rebase from latest master.

[joe-elliott]

Rebased but now getting a build error on contrib:

# github.com/open-telemetry/opentelemetry-collector-contrib/exporter/awsxrayexporter/translator [github.com/open-telemetry/opentelemetry-collector-contrib/exporter/awsxrayexporter/translator.test]
translator/segment_test.go:743:6: val1.InitEmpty undefined (type pdata.AttributeValue has no field or method InitEmpty)
translator/segment_test.go:746:6: val2.InitEmpty undefined (type pdata.AttributeValue has no field or method InitEmpty)

[tigrannajaryan]

Rebased but now getting a build error on contrib:

# github.com/open-telemetry/opentelemetry-collector-contrib/exporter/awsxrayexporter/translator [github.com/open-telemetry/opentelemetry-collector-contrib/exporter/awsxrayexporter/translator.test]
translator/segment_test.go:743:6: val1.InitEmpty undefined (type pdata.AttributeValue has no field or method InitEmpty)
translator/segment_test.go:746:6: val2.InitEmpty undefined (type pdata.AttributeValue has no field or method InitEmpty)

This is expected, ignore it.