Skip to content

feature(otel-collector): ordered processors list #910

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.

Sign up for GitHub

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

Open
filiphaftek wants to merge 1 commit into
base: main
Choose a base branch
from
+83 −0
Open

feature(otel-collector): ordered processors list #910

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions charts/sourcegraph/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ In addition to the documented values, all services also support the following va
| openTelemetry.enabled | bool | `true` | |
| openTelemetry.gateway.config.traces.exporters | object | `{}` | Define where traces should be exported to. Read how to configure different backends in the [OpenTelemetry documentation](https://opentelemetry.io/docs/collector/configuration/#exporters) |
| openTelemetry.gateway.config.traces.exportersTlsSecretName | string | `""` | Define the name of a preexisting secret containing TLS certificates for exporters, which will be mounted under "/tls". Read more about TLS configuration of exporters in the [OpenTelemetry Collector documentation](https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/configtls/README.md) |
| openTelemetry.gateway.config.traces.processorOrderedList | list | `[]` | Define the order in which trace processors run in the pipeline. When set, this takes precedence over the iteration order of `processors` (which cannot be relied on to preserve insertion order). Must contain exactly the same processor names as the keys of `processors`. |
| openTelemetry.gateway.config.traces.processors | object | `{}` | Define trace processors. Read how to configure sampling in the [OpenTelemetry documentation](https://docs.sourcegraph.com/admin/observability/opentelemetry#sampling-traces) |
| openTelemetry.gateway.containerSecurityContext.allowPrivilegeEscalation | bool | `false` | |
| openTelemetry.gateway.containerSecurityContext.runAsGroup | int | `101` | |
Expand Down
6 changes: 6 additions & 0 deletions charts/sourcegraph/templates/otel-collector/otel-collector.ConfigMap.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,15 @@ data:
- otlp
{{- if .Values.openTelemetry.gateway.config.traces.processors }}
processors:
{{- if .Values.openTelemetry.gateway.config.traces.processorOrderedList }}
Comment on lines 45 to +47

@bobheadxi bobheadxi Jul 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't this mean without processors, processorOrderedList does not work?

@filiphaftek filiphaftek Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, processorOrderedList is just addition to processors, not replacement, pls see above comment/

{{- range .Values.openTelemetry.gateway.config.traces.processorOrderedList }}
- {{ . }}
{{- end }}
{{- else }}
{{- range $key, $val := .Values.openTelemetry.gateway.config.traces.processors }}
- {{ $key }}
{{- end }}
{{- end }}
Comment on lines +47 to +55

@bobheadxi bobheadxi Jul 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should processors and processorOrderedList be mutually exclusive? Curious why we shouldn't deprecate processors or simply change its shape as processors is effectively buggy it seems

@filiphaftek filiphaftek Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bobheadxi currently processorOrderedList only sets the order, not defines the processors. If you think having them separated is better, we need to make list with full definitions, similar to https://sourcegraph.sourcegraph.com/r/github.com/sourcegraph/deploy-sourcegraph-helm/-/blob/charts/sourcegraph/values.yaml?L667 - if you think this is more clear, I can refactor?

@bobheadxi bobheadxi Jul 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm

@bobheadxi bobheadxi Jul 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be called pipeline then. in OTEL the concepts are separate, pipelines are "do things in this order"

service:
  pipelines:
    traces:
      receivers: [opencensus, jaeger]
      processors: [memory_limiter]
      exporters: [opencensus, zipkin]

while what processors are available, are configured separately

@bobheadxi bobheadxi Jul 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, it seems like here we assume all processors apply to all signals, is that right? we need to separate them to stay true to OTEL, because not every pipeline needs to apply to every signal, and most importantly, not every pipeline is compatible with every signal

eg you can have the following

service:
  pipelines:
    metrics:
      receivers: [opencensus, prometheus]
      exporters: [opencensus, prometheus]
    traces:
      receivers: [opencensus, jaeger]
      processors: [memory_limiter]
      exporters: [opencensus, zipkin]

see https://opentelemetry.io/docs/collector/configuration/#processors

Configuring a processor does not enable it. Processors are enabled by adding them to the appropriate pipelines within the service section.

@bobheadxi bobheadxi Jul 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so I think we should say:

  • processors: what processors are available (not ordered, just definitions)
  • tracePipelineProcessors: what processors apply to the trace signal (and ordered list)

{{- end }}
exporters:
{{- range $key, $val := .Values.openTelemetry.gateway.config.traces.exporters }}
Expand Down
72 changes: 72 additions & 0 deletions charts/sourcegraph/tests/otelCollectorProcessors_test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
---
suite: otelCollectorProcessors
templates:
- otel-collector/otel-collector.ConfigMap.yaml
set:
openTelemetry:
gateway:
config:
traces:
exporters:
otlp:
endpoint: "otlp.service.com:443"
tests:
- it: should not render a processors section when none are configured
asserts:
- notMatchRegex:
path: data["config.yaml"]
pattern: "processors:"
- it: should order pipeline processors by map key when processorOrderedList is not set
set:
openTelemetry:
gateway:
config:
traces:
processors:
memory_limiter:
check_interval: 1s
batch:
timeout: 1s
asserts:
- matchRegex:
path: data["config.yaml"]
pattern: "processors:\\s*\\n\\s*- batch\\s*\\n\\s*- memory_limiter"
- it: should order pipeline processors using processorOrderedList when set
set:
openTelemetry:
gateway:
config:
traces:
processors:
memory_limiter:
check_interval: 1s
batch:
timeout: 1s
processorOrderedList:
- memory_limiter
- batch
asserts:
- matchRegex:
path: data["config.yaml"]
pattern: "processors:\\s*\\n\\s*- memory_limiter\\s*\\n\\s*- batch"
- it: should still define processor configs regardless of ordering source
set:
openTelemetry:
gateway:
config:
traces:
processors:
memory_limiter:
check_interval: 1s
batch:
timeout: 1s
processorOrderedList:
- memory_limiter
- batch
asserts:
- matchRegex:
path: data["config.yaml"]
pattern: "check_interval: 1s"
- matchRegex:
path: data["config.yaml"]
pattern: "timeout: 1s"
4 changes: 4 additions & 0 deletions charts/sourcegraph/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -665,6 +665,10 @@ openTelemetry:
# -- Define trace processors.
# Read how to configure sampling in the [OpenTelemetry documentation](https://docs.sourcegraph.com/admin/observability/opentelemetry#sampling-traces)
processors: {}
# -- Define the order in which trace processors run in the pipeline. When set, this takes
# precedence over the iteration order of `processors` (which cannot be relied on to preserve
# insertion order). Must contain exactly the same processor names as the keys of `processors`.
processorOrderedList: []
# -- Define where traces should be exported to.
# Read how to configure different backends in the [OpenTelemetry documentation](https://opentelemetry.io/docs/collector/configuration/#exporters)
exporters: {}
Expand Down
Loading