Support set flag for component configs #1640

pavolloffay · 2020-08-26T15:59:01Z

Signed-off-by: Pavol Loffay ploffay@redhat.com

Creating as a draft to collect feedback. Once we agree I will clean it and add tests.

Description:

This patch adds support for configuring components (receivers, processors, exporters...) via --set flag. For example --set=processors.batch.timeout=12s or --receivers.otlp.protocols.grpc.endpoint=123456.

--set stringArray             Set arbitrary component config property. The flag has a higher precedence over config file. The arrays are overridden. Example --set=processors.batch.timeout=2s

Flags have higher precedence than config file
The component has to be defined in the config to apply the configuration otherwise the flag is noop e.g. --set=processor.queued_retry.queue_size=15 is noop if the queued_retry is not defined.
The named components work e.g. --set=processors.batch/bar.timeout=3s
The maps are joined with the existing (file) config
The arrays are overridden
The object arrays are more complicated - with flag it's possible to set only the first entry e.g. --set=processors.resource.attributes.key=key2

The implementation creates a new viper instance that is initialized with a temporary properties file that contains values passed via set flag. Then the new viper is used to apply changes to an existing configuration (loaded via the main config file).

Link to tracking Issue:

Resolves #873

Testing: < Describe what testing was performed and which tests were added.>

Tested locally
RUN_CONFIG=config.yaml make run

receivers:
    otlp:
        protocols:
            grpc:

exporters:
    logging:
        logLevel: debug

processors:
    batch:

service:
  pipelines:
    traces:
      receivers: [otlp]
      processors: [batch]
      exporters: [logging]

Documentation: < Describe the documentation added.>

None

pavolloffay · 2020-08-26T15:59:49Z

@tigrannajaryan @jrcamp could you please review and comment whether you agree with this approach?

jrcamp · 2020-08-26T16:41:09Z

In --set=processors.batch.timeout=70s Are the ='s required? Does --set processors.batch.timeout=70s and --set processors.batch.timeout 70s work?

pavolloffay · 2020-08-26T19:27:12Z

The s for timeout is required bc it is golang's time.Duration.

--set=processors.batch.timeout=70s works
--set processors.batch.timeout=70s works after trimming spaces
--set processors.batch.timeout 70s does not work

pavolloffay · 2020-08-27T11:36:39Z

Changing this to ready for review.

codecov · 2020-08-28T06:40:00Z

Codecov Report

Merging #1640 into master will decrease coverage by 0.90%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master    #1640      +/-   ##
==========================================
- Coverage   91.66%   90.75%   -0.91%     
==========================================
  Files         261      262       +1     
  Lines       18532    15864    -2668     
==========================================
- Hits        16987    14398    -2589     
+ Misses       1114     1037      -77     
+ Partials      431      429       -2

Impacted Files	Coverage Δ
service/service.go	`52.67% <60.00%> (-2.54%)`	⬇️
service/set_flag.go	`75.00% <75.00%> (ø)`
service/internal/templates.go	`30.43% <0.00%> (-21.09%)`	⬇️
processor/cloningfanoutconnector.go	`57.57% <0.00%> (-9.10%)`	⬇️
cmd/mdatagen/metricdata.go	`71.42% <0.00%> (-8.29%)`	⬇️
exporter/kafkaexporter/otlp_marshaller.go	`71.42% <0.00%> (-6.35%)`	⬇️
...rnal/scraper/processesscraper/processes_scraper.go	`80.00% <0.00%> (-5.72%)`	⬇️
extension/pprofextension/pprofextension.go	`57.14% <0.00%> (-5.36%)`	⬇️
service/builder/extensions_builder.go	`80.76% <0.00%> (-4.53%)`	⬇️
...eceiver/internal/scraper/processscraper/factory.go	`55.55% <0.00%> (-4.45%)`	⬇️
... and 246 more

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 e1a90a3...eb6e633. Read the comment docs.

pavolloffay · 2020-08-28T07:57:13Z

The test coverage is below, there are some parts of this PR that I am not sure how to cover (error of creating temp file, err for writing to a temp file, getting an error from ViperSubExtract)

jrcamp

I feel like this can be substantially simplified. See code here based on some of your changes:

jrcamp@5ea89ed

service/set_flag.go

jrcamp · 2020-08-28T21:15:44Z

config/config.go

@@ -319,23 +310,6 @@ func loadService(v *viper.Viper) (configmodels.Service, error) {
 	return service, nil
 }

-// LoadReceiver loads a receiver config from componentConfig using the provided factories.
-func LoadReceiver(componentConfig *viper.Viper, typeStr configmodels.Type, fullName string, factory component.ReceiverFactory) (configmodels.Receiver, error) {


This is used in receivercreator in contrib to create receivers at runtime. Does it need to be changed to support this feature?

I wasn't sure why it is public since it is the only public load func for a component.

pavolloffay · 2020-08-31T06:12:12Z

I feel like this can be substantially simplified. See code here based on some of your changes:

jrcamp/opentelemetry-collector@5ea89ed

Maybe I am doing something wrong but it does not work as expected, the set flags are not being enforced. There are also error messages in the log

ERROR 2020/08/31 08:10:37 svType != tvType; key=batch, st=map[string]interface {}, tt=<nil>, sv=map[timeout:12s], tv=<nil>

pavolloffay · 2020-08-31T07:55:23Z

@jrcamp I made the suggested changes and also the simplification to remove the temp file (great catch!). Your impl does not seem to work properly, the merge config in viper is not working as we want (see my previous comment).

jrcamp · 2020-09-01T19:10:36Z

I feel like this can be substantially simplified. See code here based on some of your changes:

jrcamp/opentelemetry-collector@5ea89ed

Maybe I am doing something wrong but it does not work as expected, the set flags are not being enforced. There are also error messages in the log
ERROR 2020/08/31 08:10:37 svType != tvType; key=batch, st=map[string]interface {}, tt=<nil>, sv=map[timeout:12s], tv=<nil>

I only did a quick test so there could be a bug. Can you share the config and command line params you used?

pavolloffay · 2020-09-02T06:44:47Z

I only did a quick test so there could be a bug. Can you share the config and command line params you used?

The config is in the first comment. Then you can use --set=processors.batch.timeout=3s or --set=exporters.otlp.protocols.grpc.endpoint=localhost:12345.

jrcamp · 2020-09-03T14:58:39Z

@pavolloffay looks like viper MergeConfigMap has issues. See latest change jrcamp@flags-review

pavolloffay · 2020-09-04T14:16:06Z

@jrcamp does not seem to work either try the config from the first comment with --set=receivers.otlp.protocols.grpc.endpoint=localhost:12345. It fails on

GO111MODULE=on go run --race ./cmd/otelcol/... --config config.yaml --set=exporters.otlp.protocols.grpc.endpoint=localhost:12345
2020/09/04 16:14:12 proto: duplicate proto type registered: opentelemetry.proto.metrics.v1.ResourceMetrics
2020/09/04 16:14:12 proto: duplicate proto type registered: opentelemetry.proto.metrics.v1.InstrumentationLibraryMetrics
2020/09/04 16:14:12 proto: duplicate proto type registered: opentelemetry.proto.metrics.v1.Metric
2020/09/04 16:14:12 proto: duplicate proto type registered: opentelemetry.proto.metrics.v1.DoubleDataPoint
2020-09-04T16:14:12.576+0200	INFO	service/service.go:407	Starting OpenTelemetry Collector...	{"Version": "latest", "GitHash": "<NOT PROPERLY GENERATED>", "NumCPU": 8}
2020-09-04T16:14:12.576+0200	INFO	service/service.go:251	Setting up own telemetry...
2020-09-04T16:14:12.590+0200	INFO	service/telemetry.go:106	Serving Prometheus metrics	{"address": "localhost:8888", "legacy_metrics": false, "new_metrics": true, "level": 3, "service.instance.id": "df297e01-87c1-4ae0-a5c7-72d01f7a9b1c"}
2020-09-04T16:14:12.592+0200	INFO	service/service.go:288	Loading configuration...
Error: cannot load configuration: error reading exporters configuration for otlp: 1 error(s) decoding:

* '' has invalid keys: protocols
2020/09/04 16:14:12 application run finished with error: cannot load configuration: error reading exporters configuration for otlp: 1 error(s) decoding:

* '' has invalid keys: protocols
exit status 1
make: *** [Makefile:163: run] Error 1

jrcamp · 2020-09-08T15:23:04Z

@pavolloffay I get the same startup error by adding the equivalent set command to the config file. Maybe you meant --set=receivers.otlp.protocols.grpc.endpoint=localhost:12345 instead?

But there are still more issues even after changing that. See my latest changes (viper isn't very good at merging) https://github.com/jrcamp/opentelemetry-collector/tree/flags-review

I didn't fix the tests for the stuff I refactored, just trying to find an approach that works.

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay · 2020-09-16T07:16:02Z

@jrcamp I have updated the PR with the workaround from your branch and added tests. Now it looks good to me.

for _, k := range v.AllKeys() {
		v.Set(k, v.Get(k))
	}

jrcamp · 2020-09-16T13:36:04Z

As part of my proposed implementation I also did some minor refactoring so that this isn't tied to FileLoaderConfigFactory. In theory this should work for any config factory. Otherwise it's going to be confusing why set flags work in some cases but not in others.

pavolloffay · 2020-09-16T14:41:53Z

As part of my proposed implementation I also did some minor refactoring so that this isn't tied to FileLoaderConfigFactory.

I saw that the problem is that the factory should return the final config object. We use it to construct the default/hardcoded configuration in Jaeger (agent, collector...). This remains me that we should make the set flag public.

EDIT: actually it's fine we can just use FileLoaderConfigFactory and modify the final config.

pavolloffay · 2020-09-17T06:30:24Z

As part of my proposed implementation I also did some minor refactoring so that this isn't tied to FileLoaderConfigFactory. In theory this should work for any config factory. Otherwise it's going to be confusing why set flags work in some cases but not in others.

@jrcamp as I mentioned before the factory has to return the config object. I will document your concerns and make the applySetFlag public so factories can use it.

jrcamp · 2020-09-17T15:11:38Z

Sounds good. Overall lgtm, can you just note (in the PR here) the reason patch is under target (95%). Looks like it's mostly error cases that would be hard to reach but they may ask before merging.

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay · 2020-09-17T15:30:59Z

Yes, the coverage dropped below the limit. I don't know how to cover error cases e.g.

// b is &bytes.Buffer{}
if _, err := fmt.Fprintf(b, "%s\n", property); err != nil {
                        return err
                }

and

if err := viperFlags.ReadConfig(b); err != nil {
                return fmt.Errorf("failed to read set flag config: %v", err)
        }

pavolloffay · 2020-09-19T09:42:55Z

@tigrannajaryan @bogdandrutu could you please have review/merge?

tigrannajaryan · 2020-09-21T13:51:45Z

The component has to be defined in the config to apply the configuration otherwise the flag is noop e.g. --set=processor.queued_retry.queue_size=15 is noop if the queued_retry is not defined.

Is this a silent noop? Can we make this an error instead of noop? Otherwise it will be difficult to understand why things don't work if you make a typo in the command line option.

tigrannajaryan · 2020-09-21T14:03:10Z

This patch adds support for configuring components (receivers, processors, exporters...) via --set flag. For example --set=processors.batch.timeout=12s or --receivers.otlp.protocols.grpc.endpoint=123456.

--set stringArray             Set arbitrary component config property. The flag has a higher precedence over config file. The arrays are overridden. Example --set=processors.batch.timeout=2s

* Flags have higher precedence than config file

* The component has to be defined in the config to apply the configuration otherwise the flag is noop e.g. `--set=processor.queued_retry.queue_size=15` is noop if the `queued_retry` is not defined.

* The named components work e.g. `--set=processors.batch/bar.timeout=3s`

* The maps are joined with the existing (file) config

* The arrays are overridden

* The object arrays are more complicated - with flag it's possible to set only the first entry e.g. `--set=processors.resource.attributes.key=key2`

Can you add this description to some relevant place in the source code?

We will also need it in the README, but it seems like we lost the basic running instructions after refactoring, so this can wait for now.

I just want to make sure the usage instructions are captured at least somewhere. Perhaps we can print them when --help is invoked?

pavolloffay · 2020-09-21T14:05:18Z

service/service_test.go

+	params := Parameters{
+		Factories: factories,
+	}
+	t.Run("unknown_component", func(t *testing.T) {


@tigrannajaryan have a look at this test.

If the flag references an unknown property the factory returns an error. However, if the flag references a valid component that is not in the config file e.g. batch/foo then an error is not returned (see the next test) and a new component instance is created that is not enabled in the pipeline. I think that this behaviour is fine.

However, if the flag references a valid component that is not in the config file e.g. batch/foo then an error is not returned (see the next test) and a new component instance is created that is not enabled in the pipeline. I

This is the part that I don't like. If I have a typo in my --set flag and the name of the component is incorrect then it will have no effect and there will be no indication to the user that something went wrong.

Preferably in this case the startup should fail and tell that the component is not found. Is it possible to do?

I cannot think of a simple solution. The same can happen with the config file a user can forget to add a component to a pipeline.

The ugly solution might be to parse the config in FileLoaderConfigFactory then pass it into AddSetFlagProperties which would parse it's own config and compare which components are not in the pipeline. Then parse the final config at the end of FileLoaderConfigFactory with the final viper. This is a rather horrible solution :).

Maybe we could print a warning for not assigned components during the config validation?

service/service.go

tigrannajaryan · 2020-09-21T14:16:27Z

service/service.go

+	for _, k := range v.AllKeys() {
+		v.Set(k, v.Get(k))
+	}


Why do we do this? Code is not clear and looks suspect (we get and set all keys?). Add comment.

I have added a comment

tigrannajaryan · 2020-09-21T14:20:38Z

service/service_test.go

+	"go.opentelemetry.io/collector/processor/attributesprocessor"
+	"go.opentelemetry.io/collector/processor/batchprocessor"
+	"go.opentelemetry.io/collector/receiver/jaegerreceiver"
+	"go.opentelemetry.io/collector/service/builder"


I am not sure I like how we depend on specific components from here. Can we use componenttest and example components for testing instead?

I would prefer to minimize the coupling between this package and actual component packages.

The test was already using "go.opentelemetry.io/collector/service/defaultcomponents". The components were parsed but not accessed directly. For instance this config was used by this test

opentelemetry-collector/service/testdata/otelcol-config.yaml

Line 1 in 9778b16

receivers:

tigrannajaryan · 2020-09-21T14:23:22Z

service/set_flag.go

+	for _, k := range viperFlags.AllKeys() {
+		v.Set(k, viperFlags.Get(k))
+	}


Why do we build a properties file first? Can't we apply the flagProperties directly to v here? If there is a reason please add comments to explain.

I have added a comment to the source file.

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay · 2020-09-22T07:29:25Z

I just want to make sure the usage instructions are captured at least somewhere. Perhaps we can print them when --help is invoked?

This is what the help command prints:

--set stringArray             Set arbitrary component config property. The component has to be defined in the config file and the flag has a higher precedence. Array config properties are overridden and maps are joined, note that only a single (first) array property can be set e.g. -set=processors.attributes.actions.key=some_key. Example --set=processors.batch.timeout=2s

The component has to be defined in the config to apply the configuration otherwise the flag is noop e.g. --set=processor.queued_retry.queue_size=15 is noop if the queued_retry is not defined.

Is this a silent noop? Can we make this an error instead of noop? Otherwise it will be difficult to understand why things don't work if you make a typo in the command line option.

See the help command it says that the component has to be defined in the config file. I am not sure how to do this, nicely. We could iterate over components and see whether they are defined in a pipeline. The same issue is with a config file a user can forget to add a component to the pipeline.

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay · 2020-09-23T09:07:23Z

@tigrannajaryan could you please have another look?

pavolloffay · 2020-09-28T12:55:34Z

@tigrannajaryan any update on this one?

This reverts commit 450fd93 to ensure empty objects as component config work. Temporarily fixes open-telemetry#1897

This reverts commit 450fd93 to ensure empty objects as component config work. Temporarily fixes #1897

…y#1640)" (open-telemetry#1905)" This reverts commit 2efdb28.

…y#1640) Bumps [boto3](https://github.com/boto/boto3) from 1.24.2 to 1.24.3. - [Release notes](https://github.com/boto/boto3/releases) - [Changelog](https://github.com/boto/boto3/blob/develop/CHANGELOG.rst) - [Commits](boto/boto3@1.24.2...1.24.3) --- updated-dependencies: - dependency-name: boto3 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

tigrannajaryan assigned jrcamp Aug 26, 2020

pavolloffay force-pushed the flags branch from 612ab36 to 4d8f6c5 Compare August 27, 2020 11:11

pavolloffay changed the title ~~Support set flag for component config~~ Support set flag for component configs Aug 27, 2020

pavolloffay marked this pull request as ready for review August 27, 2020 11:36

pavolloffay requested review from bogdandrutu, dmitryax, james-bebbington, nilebox, owais, pjanotti and tigrannajaryan as code owners August 27, 2020 11:36

jrcamp suggested changes Aug 28, 2020

View reviewed changes

pavolloffay force-pushed the flags branch from 73dc33f to 5324509 Compare September 15, 2020 11:54

Add --set flag to set component's properties

2a02f47

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay force-pushed the flags branch from 5324509 to 2a02f47 Compare September 15, 2020 16:31

Fix test

100c39e

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

More tests

6033505

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Add more docs

e67eb7e

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

jrcamp approved these changes Sep 17, 2020

View reviewed changes

pavolloffay commented Sep 21, 2020

View reviewed changes

tigrannajaryan reviewed Sep 21, 2020

View reviewed changes

Fix review comments

1b11e75

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

help message

eb6e633

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

tigrannajaryan approved these changes Sep 29, 2020

View reviewed changes

tigrannajaryan merged commit 450fd93 into open-telemetry:master Sep 29, 2020

bogdandrutu mentioned this pull request Oct 4, 2020

Empty objects as component config doesn't work anymore #1897

Closed

tigrannajaryan added a commit that referenced this pull request Oct 5, 2020

Revert "Support set flag for component configs (#1640)" (#1905)

2efdb28

This reverts commit 450fd93 to ensure empty objects as component config work. Temporarily fixes #1897

joe-elliott mentioned this pull request Oct 15, 2020

Enable OTLP port to be configurable via CLI flags jaegertracing/jaeger#2563

Closed

joe-elliott added a commit to joe-elliott/opentelemetry-collector that referenced this pull request Nov 13, 2020

Revert "Revert "Support set flag for component configs (open-telemetr…

c13553a

…y#1640)" (open-telemetry#1905)" This reverts commit 2efdb28.

joe-elliott added a commit to joe-elliott/opentelemetry-collector that referenced this pull request Nov 30, 2020

Revert "Revert "Support set flag for component configs (open-telemetr…

21713f2

…y#1640)" (open-telemetry#1905)" This reverts commit 2efdb28.

Support set flag for component configs #1640

Support set flag for component configs #1640

Conversation

pavolloffay commented Aug 26, 2020 • edited Loading

pavolloffay commented Aug 26, 2020

jrcamp commented Aug 26, 2020

pavolloffay commented Aug 26, 2020 • edited Loading

pavolloffay commented Aug 27, 2020

codecov bot commented Aug 28, 2020 • edited Loading

Codecov Report

pavolloffay commented Aug 28, 2020

jrcamp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Aug 31, 2020

pavolloffay commented Aug 31, 2020

jrcamp commented Sep 1, 2020

pavolloffay commented Sep 2, 2020

jrcamp commented Sep 3, 2020

pavolloffay commented Sep 4, 2020 • edited Loading

jrcamp commented Sep 8, 2020

pavolloffay commented Sep 16, 2020

jrcamp commented Sep 16, 2020

pavolloffay commented Sep 16, 2020 • edited Loading

pavolloffay commented Sep 17, 2020

jrcamp commented Sep 17, 2020

pavolloffay commented Sep 17, 2020

pavolloffay commented Sep 19, 2020

tigrannajaryan commented Sep 21, 2020

tigrannajaryan commented Sep 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Sep 22, 2020

pavolloffay commented Sep 23, 2020

pavolloffay commented Sep 28, 2020

pavolloffay commented Aug 26, 2020 •

edited

Loading

pavolloffay commented Aug 26, 2020 •

edited

Loading

codecov bot commented Aug 28, 2020 •

edited

Loading

pavolloffay commented Sep 4, 2020 •

edited

Loading

pavolloffay commented Sep 16, 2020 •

edited

Loading