Replace default config with config presets

[dmitryax]

Instead of providing default configuration, we should have presets of configurations that are applied to the collector. Suggested presets configuration:

presets:
  traces:
    enabled: true
  metrics:
    enabled: false
    hostMetrics:
      enabled: false
    selfScraping:
      enabled: true
  logs:
    enabled: false
    containerLogs: 
      enabled: false

User also must be able to provide their own config that must be applied after all the presets.

config: {}
# receivers: ...
# processors: ...
# exporters: ...
# service: ...

Each of the enabled presets and user custom config can be provided to the collector as a configuration argument, no need to merge them in the helm chart.

[TylerHelmuth]

@dmitryax the charts now support removing the default config using null. User are able to remove all default configuration and replace it with their own. Does that meet the needs of this issue?

[dmitryax]

Yes, I think we still need to move to the presets as a cleaner way than setting nulls. We already have a preset for logs collection, I think we need to apply similar approach to other use cases.

Some presets can require rendering of other resources not configmap only, e.g. hostMetrics

[TylerHelmuth]

Some presets can require rendering of other resources not configmap only, e.g. hostMetrics

Like using the presets to determine if we need to render extra permissions or something?

no need to merge them in the helm chart.

What do you mean by this?

Finally, if a customer enabled host metrics (or something else also preset) but also defined that component in their config then which wins? I would say their version of the hostMetrics config be what renders in the configmap.

[dmitryax]

Like using the presets to determine if we need to render extra permissions or something?

Yes, e.g. hostMetrics requires host volume mounts

What do you mean by this?

Collector can accept many config files https://github.com/open-telemetry/opentelemetry-collector/blob/150c1ede2f7fb5607035d3c7a10bfcab61c39afe/service/flags.go#L47-L48. I think we should supply each preset as another configuration file and let collector do the merging instead of using helm capabilities. Let me know if it makes sense to you

Finally, if a customer enabled host metrics (or something else also preset) but also defined that component in their config then which wins? I would say their version of the hostMetrics config be what renders in the configmap.

I believe what user provides in config should have precedence over all the presets

[TylerHelmuth]

To avoid breaking changes I think we'll need a presets for each item in the current default config or presets that encompasses them.

[TylerHelmuth]

Collector can accept many config files

Today I learned.

This is an interesting idea. It will certainly reduce logic in the helm chart, but I am initially concerned that by having the collector do the merge the final configuration the collector is truest running is more obscure. Is it well defined how the collector handles multiple configs if those configs conflict?

[dmitryax]

To avoid breaking changes I think we'll need a presets for each item in the current default config or presets that encompasses them.

Right, ideally the set of presets enabled by default should end up in an equivalent config that we have by default now

[dmitryax]

Is it well defined how the collector handles multiple configs if those configs conflict?

I believe it must be done in a predictable way, same way as helm is doing it (not 100% sure tho):

• Configs that are going later in the command arguments list likely have precedence.
• Maps are merged
• Lists are replaced

All the presets should get their order in the command line in a way that combinations of them works well with each other.

[TylerHelmuth]

I'll take a look at this today, but I am not totally convinced yet about the multiple configs. Concerns I'd like to address:

1. Is their documentation we can link to detailing how multiple configs work
2. Does using multiple configs simplify the charts
3. Are presets easy to understand as a consumer of the chart. We'd be moving away from explicit configuration and I want to make sure it is still easy to understand what each preset does.
4. How does it feel to interact in k8s with the installed charts when there are multiple configs.

[TylerHelmuth]

Maintaining support for agentCollector and standaloneCollector with this change is going to be pretty messy. Should we submit a PR to remove that support before handling this change?

[TylerHelmuth]

Lists are replaced

Won't this mean that if someone uses any presets, but also adds their own configuration, they will have to list preset components in their pipeline section?

presets:
  traces:
    enabled: true

config:
  receivers:
    some-extra-receiver:
  service:
    pipelines:
      traces:
        receivers: [some-extra-receiver, otlp, zipkin, jaeger]

I don't love having to list receivers that aren't defined in the config section.

[TylerHelmuth]

After spending a lot of the day on this here is where I am at:

I not a huge fan of defining the config in multiple places, mainly because I think the final configuration gets obscured, but also because lists are replaced and not merged. We have to deal with that today too, but at least in those situations the user is able to look in the value.yaml and see the exact config that will get merged and the components they need to include in their custom list. Right now I would vote for keeping the config the way it is today, but I'd like to hear more arguments in favor of presets.

I really like the idea of presets.metrics.hostMetrics.enabled. The idea of the charts handling all the k8s stuff necessary to use the hostMetrics receiver is very appealing, and there are other components that could benefit from this (k8sattributesprocessor comes to mind). What do you think about an option like components.hostmetrics.isIncluded, that a customer could set to true when they've configured the config to include host metrics? When components.hostmetrics.isIncluded is true we would render all the extra k8s stuff that hostmetrics needs to succeed.

[TylerHelmuth]

Thinking more about containerLogs as an example, the main benefit of that field is the ability to build out the complex receiver and setup the k8s stuff. The fact that it also sets up service.logs.receiver is a "nice-to-have", but will have to get overwritten. In fact, our daemonset-collector-logs example is actually wrong right now because it doesn't update the logs pipeline's receivers.

I'm gonna take a look at this with the perspective of only defining the components and not the pipeline. This will also make it easier to determine when to include receivers used between multiple pipelines, such as otlp.

[TylerHelmuth]

I'm starting to ramble, but here is another consideration:

# components enabled by default will be included in the appropriate pipelines(s) by default.  
# If any presets are enabled or disable, or if a component is configured via the 'config' field, then the appropriate 
# component list needs to be configured in the `config` field to include all desired  components
presets:
  extensions:
    health_check:
      enabled: true
    memory_ballast:
      enabled: true
  receivers:
    otlp: 
      enabled: true
    jaeger:
      enabled: true
    zipkin:
      enabled: true
    prometheus: 
      enabled: true
      selfScraping: true
    hostMetrics:
      enabled: false
    filelog: 
      enabled: false
  processors:
    batch:
      enabled: true
    memory_limiter:
      enabled: true
  exporters:
    logging:
      enabled: true

config: {}

I'm not convinced this is better than our current config, but I like that in the scenario that you need to manually defined the component list you can see each component to include.

[TylerHelmuth]

Reflected on this issue over the last week, I retract my concerns over using presets; we definitely should be using presets.

For the format of how to do presets, I think I like the "by component" version, as it makes it most clear with component(s) need to be added to pipeline(s) when custom configuration is introduced or disabled-by-default presets are enabled.

[dmitryax]

I think the per-component presets makes it too granular and confusing. It moves presets idea the the field that should be controlled by custom configuration :)

I believe presets must be per-story. The highest level presets should control pipelines. Each pipeline-preset can have sub-presets for particular features: e.g hostMetrics, selfScraping in metrics and logsCollection in logs

[dmitryax]

As discussed in the triage session, presets shouldn't end up as different configuration arguments, helm templating should be used to compose one configMap from presets and default config.

Also we decided to wait until backward support of agentCollector and standaloneCollector configs is gone. It's scheduled for removal at 0.52.0 version of the collector

[TylerHelmuth]

I believe presets must be per-story. The highest level presets should control pipelines. Each pipeline-preset can have sub-presets for particular features: e.g hostMetrics, selfScraping in metrics and logsCollection in logs.

I agree that this approach takes advantage of the concept of a preset the most. But I am concerned about the usability of it combined with any custom configuration.

If a user enabled the metrics preset (which would include otlp, and prometheus) and also configured something like the kubeletstatsreceiver, how would they know that they need to add [otlp, prometheus, kubeletstats] to the metrics receiver pipeline in config?

We need to have some indication of what components each preset configures. We may be able to get away with comments in the values.yaml honestly.

[dmitryax]

Potentially we could consider applying upsert list merge for the lists created by presets. It can work well for service->pipelines lists, but it might be confusing behavior for the configuration lists where override is expected.

[TylerHelmuth]

upsert list merge

I thought helm had no way to merge lists. If we wanted to upsert wouldn't we have to switch from arrays to maps? If it can merge lists, I think that would solve all my concerns around the service.pipelines.

configuration lists where override is expected

Not sure what you mean by this.

[dmitryax]

I thought helm had no way to merge lists. If we wanted to upsert wouldn't we have to switch from arrays to maps? If it can merge lists, I think that would solve all my concerns around the service.pipelines.

It's not possible by direct merge operation, but there are other operators e.g. concat + unique http://masterminds.github.io/sprig/lists.html

[dmitryax]

configuration lists where override is expected

Not sure what you mean by this.

There are probably some parts of the otel configuration where upsert operation on list would be not expected. E.g. if user wants to reduce list of detectors set by a preset: https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/processor/resourcedetectionprocessor

[dmitryax]

I'm still not 100% sure what should we do for lists. Overriding seems like a clean way but maybe not easy to use in case of presets + default config.

[TylerHelmuth]

I think we're gonna have to override and make sure users know how to do it properly.

[TylerHelmuth]

Also we decided to wait until backward support of agentCollector and standaloneCollector configs is gone. It's scheduled for removal at 0.52.0 version of the collector

Tracked in Issue #199

[TylerHelmuth]

agentCollector and standaloneCollector have been removed, so I will start working on this again.

[TylerHelmuth]

@dmitryax just ran into an interesting situation.

The deployment-otlp-traces example is a scenario demonstrating how you can remove default config using null. In that example the prometheus, zipkin, and jaegar receivers and the logs and metrics pipelines are removed. This is done using helm's feature where null values set in a values.yaml remove default values during the merge.

With presets, though, we can no longer take advantage of that feature. As a result, any user who has a values.yaml like

mode: deployment

ports:
  jaeger-compact:
    enabled: false
  jaeger-thrift:
    enabled: false
  jaeger-grpc:
    enabled: false
  zipkin:
    enabled: false

config:
  receivers:
    jaeger: null
    prometheus: null
    zipkin: null
  service:
    pipelines:
      traces:
        receivers:
          - otlp
      metrics: null
      logs: null

ends up with a configmap like this once their .Values.config is applied on top of the presets.

exporters:
      logging: {}
    extensions:
      health_check: {}
      memory_ballast: {}
    processors:
      batch: {}
      memory_limiter:
        check_interval: 5s
        limit_mib: 1638
        spike_limit_mib: 512
    receivers:
      jaeger: null
      otlp:
        protocols:
          grpc:
            endpoint: 0.0.0.0:4317
          http:
            endpoint: 0.0.0.0:4318
      prometheus: null
      zipkin: null
    service:
      extensions:
      - health_check
      - memory_ballast
      pipelines:
        logs: null
        metrics: null
        traces:
          exporters:
          - logging
          processors:
          - memory_limiter
          - batch
          receivers:
          - otlp
      telemetry:
        metrics:
          address: 0.0.0.0:8888

I did some experimenting with removing null values from the dictionary but it felt kinda hacky and it wouldn't scale well to being able to remove any preset field. What are your thoughts?

[TylerHelmuth]

@dmitryax we could solve this issue by not trying to merge .Values.config with the config generated by the presets; if .Values.config is set we'd take it verbatim. This would be a significant change from how things work currently, since right now users can take advantage of default values and modify/add as needed. This would also be a breaking change.

[dmitryax]

@dmitryax we could solve this issue by not trying to merge .Values.config with the config generated by the presets; if .Values.config is set we'd take it verbatim.

I like this idea. We can provide two configuration options:

1. A simple one with presets. Used by default
2. Using a custom config. If applied, all the presets and default config are ignored.

This would be a significant change from how things work currently, since right now users can take advantage of default values and modify/add as needed. This would also be a breaking change

We can deprecate .Values.config and use something like .Values.customConfig or .Values.configOverride instead

[TylerHelmuth]

@dmitryax I think to be backwards compatible we'll have to keep the existing .Values.config and use it as it is today, setting all the new preset values to false by default. Then if any of the presets are enabled they will take precedence over .Values.config, and then if .Values.configOverride is used it takes precedence over everything.

Following the same strategy as agentCollector/standaloneColletor and mode, we'll remove config in a future release and enable the presets by default.

[dmitryax]

Sound like a good plan.

[TylerHelmuth]

@dmitryax the only downside to this plan is that users will lose the convenience of the presets pretty quickly. For example, if a user wants to use the hostmetricsreceiver, the presets take care of the extra mounts for you which is great. But the second you want to customize the hostmetricsreceiver, maybe something as small as changing the interval, you'll have to use the .Values.configOverride AND remember to go setup all the mounts yourself :/

The value of the presets is it knows how to take care of all the extra k8s stuff for you, I'm worried that by not being able to customize the presets at all that most users won't be able to take advantage of all the extra stuff they do because they'll be using .Values.configOverride more often.

Should we allow enabling presets and then also supplying .Values.configOverride? If a preset is enabled at the same time as .Values.configOverride is being used, the configmap generated by the presets could be overridden by .Values.configOverride but all the extra stuff the preset sets up could still be honored.

[dmitryax]

Ok, can we keep presets and .Value.config only in a way that it doesn't require to disable anything with null? E.g. for you use case with tracing, we can have something like this:

presets:
  traces:
    enabled: true
    zipkin:
     enabled: true
    jaeger:
     enabled: true

[dmitryax]

Actually, we don't even need to disable receivers with null. They can always be disabled just by removing them from the pipeline:

config:
  service:
    pipelines:
      traces: [otlp] # to disable all other tracing receivers

[TylerHelmuth]

Actually, we don't even need to disable receivers with null. They can always be disabled just by removing them from the pipeline:

config:
  service:
    pipelines:
      traces: [otlp] # to disable all other tracing receivers

Thats true, but from experience being an end user of the collector we didn't like deploying with configuration that wasn't used. Best to keep the configmap as clean as possible.

[TylerHelmuth]

Ok, can we keep presets and .Value.config only in a way that it doesn't require to disable anything with null? E.g. for you use case with tracing, we can have something like this:

presets:
  traces:
    enabled: true
    zipkin:
     enabled: true
    jaeger:
     enabled: true

Yes I think this is a valid solution. It will allow disabling some presets while using others. Will make it easier to merge in .Values.config and won't require the same level of breaking changes.

[TylerHelmuth]

Based on our discussion, we are going to try pivoting the presets idea to be for components that require extra k8s settings, such as filelog, hostmetrics, or k8sattributes, instead of presets for the default config.

This will allow us to have a fully customizable default config, and allow for the charts to helm with more complex components.

[TylerHelmuth]

Even doing only presets for components that need extra k8s config was troublesome due to the fact that helm can't merge lists.

@dmitryax I linked 2 draft PRs with different options (both pretty messy still).

• Presets or default config with customConfig override #293: Full presets idea. Replaces default config with presets. Introduces customConfig field that if non-empty is used instead of the presets config. No merging is attempted between presets and customConfig. The result is that some previous concise scenarios get more verbose. See the daemonset-and-demployment/daemonset-values.yaml as an example. A plus to this option is that you could technically set presets.metrics.hostMetrics.enabled=true and then use customConfig and still get to take advantage of the chart setting the proper volumes/volumeMounts/envs that hostmetrics needs.

• Presets for only logsCollection and hostMetrics #294: Complex component presets idea. Keeps config as the source for the default configuration. Introduces new presets interface for logsCollection and hostMetrics. Merges these preset configs with config, with config taking precedence to continue to allow default config.service.pipeline modifications. Since config.service.pipeline has to take precedence, if the presets are enabled you have to add filelog or hostmetrics to the pipeline 🤮 (this is how containerLogs.enabled works today). If we don't allow config.service.pipeline to take precedence, that the presets will hardcode the pipelines, e.g, logs pipeline would always be [otlp, filelog] when using presets.logsCollection.enabled.

[dmitryax]

I like the second option with a proposal on how to apply list merges