Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for default configuration #886

Closed
pavolloffay opened this issue Apr 29, 2020 · 16 comments
Closed

Support for default configuration #886

pavolloffay opened this issue Apr 29, 2020 · 16 comments
Labels
help wanted Good issue for contributors to OpenTelemetry Service to pick up
Milestone

Comments

@pavolloffay
Copy link
Member

The document https://docs.google.com/document/d/1NeheFG7DmcUYo_h2vLtNRlia9x5wOJMlV4QKEK05FhQ/edit# explains that OTEL collector will by default use a default configuration and if a configuration is provided it will be merged. Is it still on the roadmap?

@bogdandrutu
Copy link
Member

The behavior is already supported, the only problem is that the default config for every component is very minimal right now and a lot of times incomplete

@bogdandrutu
Copy link
Member

@pavolloffay Can you clarify what we miss here. For the moment we do offer every component to provide a default configuration, probably one missing part is to have default components registered by default.

When it comes to pipeline we don't have a default concept, that's something that we probably miss and need to add.

@tigrannajaryan
Copy link
Member

@bogdandrutu we have default configs for components, but they are only effective if you manually enable the component.

In theory a service-wide default config can also enable certain components, so that if you run Collector without any config file for example it will always have several receivers enabled (e.g. OTLP, maybe other open-source ones), memory_limiter, batch and queue processors enabled, traces and metrics pipelines configured and ready. The only thing the end user will need to do to begin running a Collector is specify exporter endpoint.

For now we can achieve similar end-user experience simply by providing a default config.yaml file in this repo or in Docker images we publish.

@pavolloffay
Copy link
Member Author

For the moment we do offer every component to provide a default configuration, probably one missing part is to have default components registered by default.

Correct, each component at the moment provides a default configuration. However these components have to be explicitly enabled and registered to a pipeline in the config file. The current behavior is that the collector fails to start without configuration fine.

Jaeger OTEL collector by default enables a set of hardcoded components - Jaeger receiver, some processors and exporter (storage implementation).

So I was wondering if the upstream OTEL collector will do the same and how that would affect Jaeger. I assume the default config could be changed for every distribution.

@tigrannajaryan tigrannajaryan self-assigned this Jun 11, 2020
@tigrannajaryan tigrannajaryan added this to To do in Collector via automation Jun 11, 2020
@flands flands added this to the GA 1.0 milestone Jun 18, 2020
@tigrannajaryan
Copy link
Member

I think by default we need to enable the following components:

receivers:
  otlp:

processors:
  memory_limiter:
    check_interval: 5s
    limit_mib: 128
    spike_limit_mib: 64
    ballast_size_mib: 0
  batch:
  queued_retry:

extensions:
  health_check:
  zpages:

service:
  pipelines:
    traces:
      receivers: [otlp]
      processors: [memory_limiter,batch,queued_retry]
    metrics:
      receivers: [otlp]
      processors: [memory_limiter,batch]

  extensions: [health_check,zpages]

Any of these components can be disabled by overriding the list of receivers, processors or extensions under the service setting.

@tigrannajaryan
Copy link
Member

Do we need to enable any other components by default? The config above is not sufficient to start the Collector. At the minimum you need one exporter. I don't know if we want to have a default exporter. Where would it point to?

@tigrannajaryan
Copy link
Member

I think it should be safe to hard-code this default config in the Collector, and do the merging with user-defined config somewhere in

func FileLoaderConfigFactory(v *viper.Viper, factories config.Factories) (*configmodels.Config, error) {

viper.MergeInConfig() appears to be an easy wat to implement the merging.

We will need to make sure user-defined config overrides the default one on a key-by-key basis. I.e. this should be a sufficient user-defined config to enable an exporter to be used with default receiver and processor:

exporters:
  otlp:
    endpoint: server:1234

service:
  pipelines:
    traces:
      exporters: [otlp]
    metrics:
      exporters: [otlp]

@tigrannajaryan tigrannajaryan added the help wanted Good issue for contributors to OpenTelemetry Service to pick up label Jun 23, 2020
@tigrannajaryan tigrannajaryan removed their assignment Jun 23, 2020
@tigrannajaryan tigrannajaryan removed this from To do in Collector Jun 23, 2020
@nilebox
Copy link
Member

nilebox commented Jun 23, 2020

I don't know if we want to have a default exporter. Where would it point to?

You may enable logging exporter by default if you want to have a working pipeline out of the box, but it's only useful for quick start.

@pavolloffay
Copy link
Member Author

Thanks for looking into this @tigrannajaryan.

In Jaeger OTEL collector we have implemented the merging in the ConfigFactory as you pointed out:
https://github.com/jaegertracing/jaeger/blob/fbbeaacdb47a6c81c3516dd21136a2794e6a2fc4/cmd/opentelemetry/cmd/agent/main.go#L63. However the merging is done via mergo library https://github.com/jaegertracing/jaeger/blob/fbbeaacdb47a6c81c3516dd21136a2794e6a2fc4/cmd/opentelemetry/app/defaultconfig/merge.go#L24. It basically merges any golang structs.

@pavolloffay
Copy link
Member Author

Please assign this to me I will have a look at it.

@pavolloffay
Copy link
Member Author

The PR #1273 adds support for default conf by injecting it into the user-defined configuration. The thread #1273 (comment) shows that there are some problems with ambiguity: injecting the configuration might cause confusion as users would not know what configuration they are running.

The main problem this feature is tackling is to simplify the deployment so users don't have to study all the configuration options to make it right. There are might be alternative solution that come to my mind:

  • a subcommand that would either generate or inject the default configuration to an existing config

@tigrannajaryan WDYT?

@tigrannajaryan
Copy link
Member

tigrannajaryan commented Aug 28, 2020

@pavolloffay another alternate: what if we simply provide a default config file in our distribution? The user will have to specifically use it via --config command line parameter which is slightly more work than completely automatic defaults. However the additional benefit of that will be that we can nicely document the default config file with comments and even fully document components that we don't want to be enabled by default.
The user can then take the default file and tweak it to get what they need.

@pavolloffay
Copy link
Member Author

pavolloffay commented Sep 16, 2020

One caveat is that people will first have to copy the config file from the image before editing it.

It will also make the instructions for docker https://opentelemetry.io/docs/collector/about/ nicer as it will require only docker command and not cloning the full repo.

The file from the container can be copied as

docker create --name otelcol-config otelcol
docker cp otelcol-config:/config.yaml . && docker rm otelcol-config

tigrannajaryan pushed a commit that referenced this issue Sep 22, 2020
Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Add OTLP receiver and qretry processor to example and default config.

**Link to tracking Issue:**
Updates #886 
Updates #1721 

**Testing:**
* `make run`
* build and run docker image
@pavolloffay
Copy link
Member Author

@tigrannajaryan now the default config is in the docker image.

Should we also add the default config to release assets and system distributions (rpm, deb)? If no then we can close this issue.

@tigrannajaryan
Copy link
Member

@pavolloffay yes, I think it would be nice to include the default config in other distributions.

@tigrannajaryan tigrannajaryan modified the milestones: GA 1.0, Backlog Oct 2, 2020
@bogdandrutu
Copy link
Member

Closing this, since I think we have the default config in all the distros.

/cc @jpkrohling to confirm, and maybe open a separate issue if that is not the case.

Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this issue Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Good issue for contributors to OpenTelemetry Service to pick up
Projects
None yet
Development

No branches or pull requests

5 participants