feature(otel-collector): ordered processors list#910
Conversation
filiphaftek
changed the title
| {{- if .Values.openTelemetry.gateway.config.traces.processorOrderedList }} | ||
| {{- range .Values.openTelemetry.gateway.config.traces.processorOrderedList }} | ||
| - {{ . }} | ||
| {{- end }} | ||
| {{- else }} | ||
| {{- range $key, $val := .Values.openTelemetry.gateway.config.traces.processors }} | ||
| - {{ $key }} | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
| {{- if .Values.openTelemetry.gateway.config.traces.processors }} | ||
| processors: | ||
| {{- if .Values.openTelemetry.gateway.config.traces.processorOrderedList }} |
There was a problem hiding this comment.
doesn't this mean without processors, processorOrderedList does not work?
There was a problem hiding this comment.
yes, processorOrderedList is just addition to processors, not replacement, pls see above comment/
3 participants
otel-collector processors list is using map keys, which is sorted alphabethically.
Some processoers should be used in order i.e. tail_sampling before batch, so this PR adds additional property to set proper order of the processors. It can only be used together with processors definition.
Change is backward compatible
Checklist
Test plan
Unit tests.
e2e