Use default/hardcoded configuration #1273

pavolloffay · 2020-07-07T16:02:38Z

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

Description: <Describe what has changed.

This patch enabled default/hardcoded configuration. By default OTEL collector will use batch and queued_retry processors. User-defined processors take precedence over default configuration.

Merging configuration is done via https://github.com/imdario/mergo. Viper does not support merging of go structs. Alternatively, we could use https://godoc.org/github.com/spf13/viper#MergeConfig, but the default configuration would have to be specified as a string. Also the Mergo library is nicely configurable.

Link to tracking Issue:

Partially implements #886. Missing is memory_limiter and health check.

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

Documentation: < Describe the documentation added.>

pavolloffay · 2020-07-07T16:02:54Z

@tigrannajaryan could you please review?

codecov · 2020-07-08T12:31:41Z

Codecov Report

Merging #1273 into master will decrease coverage by 0.02%.
The diff coverage is 86.20%.

@@            Coverage Diff             @@
##           master    #1273      +/-   ##
==========================================
- Coverage   91.96%   91.94%   -0.03%     
==========================================
  Files         254      256       +2     
  Lines       17286    17314      +28     
==========================================
+ Hits        15897    15919      +22     
- Misses        990      993       +3     
- Partials      399      402       +3

Impacted Files	Coverage Δ
service/service.go	`54.91% <50.00%> (-0.30%)`	⬇️
service/defaultconfig/config.go	`100.00% <100.00%> (ø)`
service/defaultconfig/merge.go	`100.00% <100.00%> (ø)`
translator/internaldata/resource_to_oc.go	`82.97% <0.00%> (-2.13%)`	⬇️

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 4025987...a96fd56. Read the comment docs.

pavolloffay · 2020-07-08T15:27:38Z

Windows test failed it does not seem related to this PR.

pavolloffay · 2020-07-31T06:13:19Z

@tigrannajaryan could you please have a look?

tigrannajaryan · 2020-08-13T18:43:56Z

@pavolloffay apologies for the delay, this slipped off my radar.

A couple questions:

How does overriding of existing keys in maps and arrays work? For example if I specify a single attributes processor in my config file will it be the only processor enabled or the batch processor will be also enabled because it is in the default. Similarly, if I specify a pipeline "traces" in my config file does it completely override everything in the default? For example if I have this in the config file:

pipelines:
   receivers: [opencensus]

What's by effective config? Is it this?

pipelines:
   receivers: [opencensus]
   processors: [batch, queued_retry]

Or the "processors" will not be included in the effective config?

Missing is memory_limiter and health check.

I think we can enable health check by default. Anything that prevents it?

tigrannajaryan · 2020-08-13T18:54:22Z

How does overriding of existing keys in maps and arrays work? For example if I specify a single attributes processor in my config file will it be the only processor enabled or the batch processor will be also enabled because it is in the default. Similarly, if I specify a pipeline "traces" in my config file does it completely override everything in the default? For example if I have this in the config file:
pipelines:
   receivers: [opencensus]
What's by effective config? Is it this?
pipelines:
   receivers: [opencensus]
   processors: [batch, queued_retry]
Or the "processors" will not be included in the effective config?

Nevermind, I can see how it works in the tests.

tigrannajaryan · 2020-08-13T18:56:43Z

@pavolloffay please add this change to the Unreleased section of CHANGELOG.md. This changes the default behavior of the Collector so we want to make sure users are aware of it.

tigrannajaryan · 2020-08-13T19:04:20Z

service/defaultconfig/config.go

+ queuedRetry := factories.Processors["queued_retry"].CreateDefaultConfig()
+ return &configmodels.Config{
+ Receivers: configmodels.Receivers{otlpRec.Name(): otlpRec},
+ Processors: configmodels.Processors{batch.Name(): batch, queuedRetry.Name(): queuedRetry},


We intend to declare "queue_retry" obsolete, since the exporters now are supposed to have separate queues. I think we don't need "queue_retry" in the default config anymore.
cc @bogdandrutu

pavolloffay · 2020-08-19T13:35:04Z

@tigrannajaryan I have updated the PR: qretry is removed from the default config and I have added the PR to the changelog.

tigrannajaryan · 2020-08-19T14:08:22Z

@pavolloffay please fix lint/test errors.

pavolloffay · 2020-08-19T14:34:47Z

It should be good now

pavolloffay · 2020-08-19T15:13:04Z

There is an issue with the load test, I am looking into it.

pavolloffay · 2020-08-19T17:14:21Z

service/defaultconfig/config.go

+func CreateDefaultConfig(factories component.Factories, forConfig *configmodels.Config) *configmodels.Config {
+ // the default config cannot be installed to the missing pipeline
+ // because the service will fail to start as there is no default exporter.
+ if forConfig == nil || forConfig.Service.Pipelines["traces"] == nil {


I had to add forConfig param for the reason described ^^ e.g. the service would fail to start if there was defined only metrics pipeline.

I am not sure if I like this API now. Maybe we should not return the default config but instead modify the supplied one by installing missing components. Now is the challenge what we should do with the receivers, processors array from the pipeline. Shall we not touch it if the user defined it (this allows disabling default components)?

Yeah, this is unfortunate. I agree with you. Also the behavior is hard to explain to the users and can be a source of confusion. We may actually make it worse by trying to make the user's life easier.

The more I think about it the less appealing I find the initially idea of the degault config. I am inclined to give up on the notion of the default config unless we can come up with a clear and intuitive behavior. I have a hard time suggesting a good approach myself.

I was more talking here from the internal API perspective. I still see benefits in the default config. Users shouldn't be required to always configure batching, memory limiter, the minimal user interaction the better.

For the end-user the rationale should be:

the default config should not cause start failure, any failures

the default config should be overridable

user should know the final configuration

Sure, I agree that the concept of the default config is generally useful. But I don't like how our particular implementation of it works from the user's perspective.

The workaround you have on line 27 is not going to work if the traces pipeline has a different name (which we allow). Even if it worked, how do we rationalize that default config only applies when there is a traces pipeline but not when there is a metrics pipeline only?

The workaround you have on line 27 is not going to work if the traces pipeline has a different name (which we allow)

We could make it work by checking the pipeline type and properly injecting the configuration. But I agree that it might be cryptic.

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

tigrannajaryan · 2020-09-09T16:49:47Z

@pavolloffay Sorry, I haven't been able to come up with better suggestions. Absent any new proposal on how to handle this better I think we should give up on the idea, close this PR and go with what I suggest in this comment: include a default config in the distribution.

pavolloffay · 2020-09-16T12:01:04Z

Agree, closing this.

pavolloffay requested review from bogdandrutu, dmitryax, flands, james-bebbington, owais, pjanotti, rghetia and tigrannajaryan as code owners July 7, 2020 16:02

pavolloffay force-pushed the default-config branch from 958ddd1 to 828f9b3 Compare July 8, 2020 08:54

jrcamp assigned tigrannajaryan Jul 21, 2020

tigrannajaryan reviewed Aug 13, 2020

View reviewed changes

pavolloffay force-pushed the default-config branch from e1ac469 to f48be5f Compare August 19, 2020 13:34

pavolloffay requested a review from nilebox as a code owner August 19, 2020 13:34

pavolloffay commented Aug 19, 2020

View reviewed changes

pavolloffay force-pushed the default-config branch 2 times, most recently from 6ab3ec5 to 2b5a7d9 Compare August 25, 2020 14:49

pavolloffay added 2 commits August 26, 2020 10:33

Use default/hardcoded configuration

ab7aca5

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

Reverse configs and allow both nil

2a561e5

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

pavolloffay force-pushed the default-config branch from 2b5a7d9 to 2a561e5 Compare August 26, 2020 11:08

pavolloffay added 3 commits August 26, 2020 14:02

Fix service test

9dde2b6

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

Add comment

8d35734

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

Add comment

a96fd56

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

pavolloffay mentioned this pull request Aug 28, 2020

Support for default configuration #886

Closed

pavolloffay closed this Sep 16, 2020

Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024

install sdk config (open-telemetry#1273)

e072daa

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use default/hardcoded configuration #1273

Use default/hardcoded configuration #1273

pavolloffay commented Jul 7, 2020

pavolloffay commented Jul 7, 2020

codecov bot commented Jul 8, 2020 •

edited

Loading

pavolloffay commented Jul 8, 2020

pavolloffay commented Jul 31, 2020

tigrannajaryan commented Aug 13, 2020

tigrannajaryan commented Aug 13, 2020

tigrannajaryan commented Aug 13, 2020

tigrannajaryan Aug 13, 2020

pavolloffay commented Aug 19, 2020

tigrannajaryan commented Aug 19, 2020

pavolloffay commented Aug 19, 2020

pavolloffay commented Aug 19, 2020

pavolloffay Aug 19, 2020

tigrannajaryan Aug 19, 2020

pavolloffay Aug 20, 2020

tigrannajaryan Aug 27, 2020

pavolloffay Aug 28, 2020

tigrannajaryan commented Sep 9, 2020

pavolloffay commented Sep 16, 2020

Use default/hardcoded configuration #1273

Use default/hardcoded configuration #1273

Conversation

pavolloffay commented Jul 7, 2020

pavolloffay commented Jul 7, 2020

codecov bot commented Jul 8, 2020 • edited Loading

Codecov Report

pavolloffay commented Jul 8, 2020

pavolloffay commented Jul 31, 2020

tigrannajaryan commented Aug 13, 2020

tigrannajaryan commented Aug 13, 2020

tigrannajaryan commented Aug 13, 2020

tigrannajaryan Aug 13, 2020

Choose a reason for hiding this comment

pavolloffay commented Aug 19, 2020

tigrannajaryan commented Aug 19, 2020

pavolloffay commented Aug 19, 2020

pavolloffay commented Aug 19, 2020

pavolloffay Aug 19, 2020

Choose a reason for hiding this comment

tigrannajaryan Aug 19, 2020

Choose a reason for hiding this comment

pavolloffay Aug 20, 2020

Choose a reason for hiding this comment

tigrannajaryan Aug 27, 2020

Choose a reason for hiding this comment

pavolloffay Aug 28, 2020

Choose a reason for hiding this comment

tigrannajaryan commented Sep 9, 2020

pavolloffay commented Sep 16, 2020

codecov bot commented Jul 8, 2020 •

edited

Loading