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

[collector] Memory limiter processor cannot be disabled #1272

Closed
rogercoll opened this issue Jul 23, 2024 · 2 comments
Closed

[collector] Memory limiter processor cannot be disabled #1272

rogercoll opened this issue Jul 23, 2024 · 2 comments
Labels
bug Something isn't working chart:collector Issue related to opentelemetry-collector helm chart enhancement New feature or request

Comments

@rogercoll
Copy link
Contributor

The collector's Helm Chart allows disabling any configuration's component by setting it to null, for example:

  receivers:
    jaeger: null
    prometheus: null
    zipkin: null

In addition, custom pipelines that don't use these components must also be provided. This is particularly useful when using the Helm Chart with a collector distribution that does not embed some of the default chart components.

The same concept cannot be applied to the memory_limiter processor because it is always added by the chart when not defined by the user, see template:

{{- define "opentelemetry-collector.baseConfig" -}}
{{- $processorsConfig := get .Values.config "processors" }}
{{- if not $processorsConfig.memory_limiter }}
{{-   $_ := set $processorsConfig "memory_limiter" (include "opentelemetry-collector.memoryLimiter" . | fromYaml) }}
{{- end }}

Is the memory_limiter component a requirement for this chart? What do you think of adding a new value to parametrize it like the .Values.useGOMEMLIMIT but for the memory_limiter? (e.g .Values.useK8SMEMLIMIT)

@dosubot dosubot bot added bug Something isn't working chart:collector Issue related to opentelemetry-collector helm chart enhancement New feature or request labels Jul 23, 2024
@TylerHelmuth
Copy link
Member

Although it is a best practice, the chart shouldn't be requiring it since the collector can run fine without it (unlike who we must have the health_check extension).

I'd be ok with updating the template to not add it back if it is not present. I don't think we need any other fields to control it. Would you like to submit that fix?

@rogercoll
Copy link
Contributor Author

I'd be ok with updating the template to not add it back if it is not present. I don't think we need any other fields to control it. Would you like to submit that fix?

👍 Definitely, preparing the fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working chart:collector Issue related to opentelemetry-collector helm chart enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants