Miretpl commented on code in PR #64902:
URL: https://github.com/apache/airflow/pull/64902#discussion_r3059696870
##########
chart/values.yaml:
##########
@@ -3926,10 +3963,15 @@ config:
remote_logging: '{{- ternary "True" "False" (or
.Values.elasticsearch.enabled .Values.opensearch.enabled) }}'
colored_console_log: 'False'
metrics:
- statsd_on: '{{ ternary "True" "False" .Values.statsd.enabled }}'
+ statsd_on: '{{ ternary "True" "False" (and .Values.statsd.enabled (not
.Values.otelCollector.metricsEnabled)) }}'
statsd_port: 9125
statsd_prefix: airflow
statsd_host: '{{ printf "%s-statsd" (include "airflow.fullname" .) }}'
+ otel_on: '{{ ternary "True" "False" .Values.otelCollector.metricsEnabled
}}'
+ otel_host: '{{ if .Values.otelCollector.metricsEnabled }}{{ printf
"%s-otel-collector" (include "airflow.fullname" .) }}{{ end }}'
+ otel_port: '4318'
Review Comment:
```suggestion
otel_port: '{{ .Values.ports.otelCollectorOtlpHttp }}'
```
##########
chart/templates/_helpers.yaml:
##########
@@ -403,6 +421,11 @@ If release name contains chart name it will be used as a
full name.
{{- printf "%s:%s" .Values.images.gitSync.repository
.Values.images.gitSync.tag }}
{{- end }}
+{{- define "otel_collector_image" -}}
+ {{- printf "%s:%s" .Values.images.otelCollector.repository
.Values.images.otelCollector.tag }}
+{{- end }}
+
Review Comment:
```suggestion
```
##########
chart/templates/_helpers.yaml:
##########
@@ -140,6 +140,24 @@ If release name contains chart name it will be used as a
full name.
name: {{ template "opensearch_secret" . }}
key: connection
{{- end }}
+ {{- if or .Values.otelCollector.tracesEnabled
.Values.otelCollector.metricsEnabled }}
+ - name: OTEL_SERVICE_NAME
+ value: "airflow"
+ - name: OTEL_EXPORTER_OTLP_PROTOCOL
+ value: "http/protobuf"
+ {{- end }}
+ {{- if .Values.otelCollector.tracesEnabled }}
+ - name: OTEL_TRACES_EXPORTER
+ value: "otlp_proto_http"
+ - name: OTEL_EXPORTER_OTLP_TRACES_ENDPOINT
+ value: "http://{{ include "airflow.fullname" . }}-otel-collector:{{
.Values.ports.otelCollectorOtlpHttp }}/v1/traces"
+ {{- end }}
+ {{- if .Values.otelCollector.metricsEnabled }}
+ - name: OTEL_EXPORTER_OTLP_METRICS_ENDPOINT
+ value: "http://{{ include "airflow.fullname" . }}-otel-collector:{{
.Values.ports.otelCollectorOtlpHttp }}/v1/metrics"
+ - name: OTEL_METRIC_EXPORT_INTERVAL
+ value: "30000"
Review Comment:
Could we make it configurable? I think it will depend heavily on the user
requirements regarding the monitoring resolution.
##########
chart/templates/otel-collector/otel-collector-deployment.yaml:
##########
@@ -0,0 +1,118 @@
+{{/*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+*/}}
+
+################################
+## OTel Collector Deployment
+################################
+{{- if .Values.otelCollector.enabled }}
+{{- $nodeSelector := or .Values.otelCollector.nodeSelector
.Values.nodeSelector }}
+{{- $affinity := or .Values.otelCollector.affinity .Values.affinity }}
+{{- $tolerations := or .Values.otelCollector.tolerations .Values.tolerations }}
+{{- $topologySpreadConstraints := or
.Values.otelCollector.topologySpreadConstraints
.Values.topologySpreadConstraints }}
+{{- $revisionHistoryLimit := include "airflow.revisionHistoryLimit" (list
.Values.otelCollector.revisionHistoryLimit .Values.revisionHistoryLimit) }}
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+ name: {{ include "airflow.fullname" . }}-otel-collector
+ labels:
+ tier: airflow
+ component: otel-collector
+ release: {{ .Release.Name }}
+ chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
+ heritage: {{ .Release.Service }}
+ {{- if or .Values.labels .Values.otelCollector.labels }}
+ {{- mustMerge .Values.otelCollector.labels .Values.labels | toYaml |
nindent 4 }}
+ {{- end }}
+ {{- with .Values.otelCollector.annotations }}
Review Comment:
```suggestion
{{- with .Values.otelCollector.configMapAnnotations }}
```
for consistency with other ConfigMaps annotations within the chart (in
general `annotations` are rather used for deployment annotations, not
configmaps).
##########
chart/templates/otel-collector/otel-collector-deployment.yaml:
##########
@@ -0,0 +1,118 @@
+{{/*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+*/}}
+
+################################
+## OTel Collector Deployment
+################################
+{{- if .Values.otelCollector.enabled }}
+{{- $nodeSelector := or .Values.otelCollector.nodeSelector
.Values.nodeSelector }}
+{{- $affinity := or .Values.otelCollector.affinity .Values.affinity }}
+{{- $tolerations := or .Values.otelCollector.tolerations .Values.tolerations }}
+{{- $topologySpreadConstraints := or
.Values.otelCollector.topologySpreadConstraints
.Values.topologySpreadConstraints }}
+{{- $revisionHistoryLimit := include "airflow.revisionHistoryLimit" (list
.Values.otelCollector.revisionHistoryLimit .Values.revisionHistoryLimit) }}
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+ name: {{ include "airflow.fullname" . }}-otel-collector
+ labels:
+ tier: airflow
+ component: otel-collector
+ release: {{ .Release.Name }}
+ chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
+ heritage: {{ .Release.Service }}
+ {{- if or .Values.labels .Values.otelCollector.labels }}
+ {{- mustMerge .Values.otelCollector.labels .Values.labels | toYaml |
nindent 4 }}
+ {{- end }}
+ {{- with .Values.otelCollector.annotations }}
+ annotations: {{- toYaml . | nindent 4 }}
+ {{- end }}
+spec:
+ replicas: 1
+ {{- if ne $revisionHistoryLimit "" }}
+ revisionHistoryLimit: {{ $revisionHistoryLimit }}
+ {{- end }}
+ selector:
+ matchLabels:
+ tier: airflow
+ component: otel-collector
+ release: {{ .Release.Name }}
+ template:
+ metadata:
+ labels:
+ tier: airflow
+ component: otel-collector
+ release: {{ .Release.Name }}
+ {{- if or .Values.labels .Values.otelCollector.labels }}
+ {{- mustMerge .Values.otelCollector.labels .Values.labels | toYaml |
nindent 8 }}
+ {{- end }}
+ annotations:
+ checksum/otel-collector-config: {{ include (print $.Template.BasePath
"/configmaps/otel-collector-configmap.yaml") . | sha256sum }}
+ {{- with .Values.otelCollector.podAnnotations }}
+ {{- tpl (toYaml .) $ | nindent 8 }}
+ {{- end }}
+ spec:
+ nodeSelector: {{- toYaml $nodeSelector | nindent 8 }}
+ affinity: {{- toYaml $affinity | nindent 8 }}
+ tolerations: {{- toYaml $tolerations | nindent 8 }}
+ topologySpreadConstraints: {{- toYaml $topologySpreadConstraints |
nindent 8 }}
+ terminationGracePeriodSeconds: {{
.Values.otelCollector.terminationGracePeriodSeconds }}
+ {{- if .Values.otelCollector.priorityClassName }}
+ priorityClassName: {{ .Values.otelCollector.priorityClassName }}
+ {{- end }}
+ restartPolicy: Always
+ securityContext: {{- toYaml .Values.otelCollector.securityContexts.pod |
nindent 8 }}
Review Comment:
Not sure if we should use the logic like
`localPodSecurityContext`/`externalContainerSecurityContext`, but maybe someone
else will have some stronger opinion regarding that one.
##########
chart/values.schema.json:
##########
@@ -1089,6 +1089,28 @@
"default": "IfNotPresent"
Review Comment:
Most of the parameters added in the values.yaml are not specified here.
Could you add all of them to this file? Basically, it is a source for some
documentation and also allows for values.yaml validations.
##########
chart/values.yaml:
##########
@@ -3710,6 +3743,10 @@ ports:
redisDB: 6379
statsdIngest: 9125
statsdScrape: 9102
+ otelCollectorOtlpHttp: 4318
+ otelCollectorOtlpGrpc: 4317
+ otelCollectorScrape: 8889
+ jaegerOtlpGrpc: 4317
Review Comment:
```suggestion
```
I would remove it as it is not collector-specific and can be adjusted
together with overwriting of otel collector configuration file (exporters part).
##########
chart/values.yaml:
##########
@@ -3926,10 +3963,15 @@ config:
remote_logging: '{{- ternary "True" "False" (or
.Values.elasticsearch.enabled .Values.opensearch.enabled) }}'
colored_console_log: 'False'
metrics:
- statsd_on: '{{ ternary "True" "False" .Values.statsd.enabled }}'
+ statsd_on: '{{ ternary "True" "False" (and .Values.statsd.enabled (not
.Values.otelCollector.metricsEnabled)) }}'
Review Comment:
We should add this information at least next to `statsd.enabled` flag in
values.yaml and values.schema.json too (from which the part of the
documentation is generated).
##########
chart/templates/otel-collector/otel-collector-deployment.yaml:
##########
@@ -0,0 +1,118 @@
+{{/*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+*/}}
+
+################################
+## OTel Collector Deployment
+################################
+{{- if .Values.otelCollector.enabled }}
+{{- $nodeSelector := or .Values.otelCollector.nodeSelector
.Values.nodeSelector }}
+{{- $affinity := or .Values.otelCollector.affinity .Values.affinity }}
+{{- $tolerations := or .Values.otelCollector.tolerations .Values.tolerations }}
+{{- $topologySpreadConstraints := or
.Values.otelCollector.topologySpreadConstraints
.Values.topologySpreadConstraints }}
+{{- $revisionHistoryLimit := include "airflow.revisionHistoryLimit" (list
.Values.otelCollector.revisionHistoryLimit .Values.revisionHistoryLimit) }}
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+ name: {{ include "airflow.fullname" . }}-otel-collector
+ labels:
+ tier: airflow
+ component: otel-collector
+ release: {{ .Release.Name }}
+ chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
+ heritage: {{ .Release.Service }}
+ {{- if or .Values.labels .Values.otelCollector.labels }}
+ {{- mustMerge .Values.otelCollector.labels .Values.labels | toYaml |
nindent 4 }}
+ {{- end }}
+ {{- with .Values.otelCollector.annotations }}
+ annotations: {{- toYaml . | nindent 4 }}
+ {{- end }}
+spec:
+ replicas: 1
+ {{- if ne $revisionHistoryLimit "" }}
+ revisionHistoryLimit: {{ $revisionHistoryLimit }}
+ {{- end }}
+ selector:
+ matchLabels:
+ tier: airflow
+ component: otel-collector
+ release: {{ .Release.Name }}
+ template:
+ metadata:
+ labels:
+ tier: airflow
+ component: otel-collector
+ release: {{ .Release.Name }}
+ {{- if or .Values.labels .Values.otelCollector.labels }}
+ {{- mustMerge .Values.otelCollector.labels .Values.labels | toYaml |
nindent 8 }}
+ {{- end }}
+ annotations:
+ checksum/otel-collector-config: {{ include (print $.Template.BasePath
"/configmaps/otel-collector-configmap.yaml") . | sha256sum }}
+ {{- with .Values.otelCollector.podAnnotations }}
+ {{- tpl (toYaml .) $ | nindent 8 }}
+ {{- end }}
+ spec:
+ nodeSelector: {{- toYaml $nodeSelector | nindent 8 }}
+ affinity: {{- toYaml $affinity | nindent 8 }}
+ tolerations: {{- toYaml $tolerations | nindent 8 }}
+ topologySpreadConstraints: {{- toYaml $topologySpreadConstraints |
nindent 8 }}
+ terminationGracePeriodSeconds: {{
.Values.otelCollector.terminationGracePeriodSeconds }}
+ {{- if .Values.otelCollector.priorityClassName }}
+ priorityClassName: {{ .Values.otelCollector.priorityClassName }}
+ {{- end }}
+ restartPolicy: Always
+ securityContext: {{- toYaml .Values.otelCollector.securityContexts.pod |
nindent 8 }}
+ containers:
+ - name: otel-collector
+ image: {{ template "otel_collector_image" . }}
+ imagePullPolicy: {{ .Values.images.otelCollector.pullPolicy }}
+ securityContext: {{- toYaml
.Values.otelCollector.securityContexts.container | nindent 12 }}
+ args:
+ - "--config=/etc/otel-collector/config.yml"
Review Comment:
In some cases, users may want to specify additional flags for otel
collector. Could we make it a values field with that config value as the
default one?
##########
chart/templates/configmaps/otel-collector-configmap.yaml:
##########
@@ -0,0 +1,75 @@
+{{/*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+*/}}
+
+################################
+## OTel Collector ConfigMap
+################################
+{{- if .Values.otelCollector.enabled }}
Review Comment:
Could you make it possible to overwrite the whole ConfigMap or `config.yml`
instead of it? It should be possible for users to create their own
configuration, which will be used by otelCollector.
##########
chart/templates/otel-collector/otel-collector-deployment.yaml:
##########
@@ -0,0 +1,118 @@
+{{/*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+*/}}
+
+################################
+## OTel Collector Deployment
+################################
+{{- if .Values.otelCollector.enabled }}
+{{- $nodeSelector := or .Values.otelCollector.nodeSelector
.Values.nodeSelector }}
+{{- $affinity := or .Values.otelCollector.affinity .Values.affinity }}
+{{- $tolerations := or .Values.otelCollector.tolerations .Values.tolerations }}
+{{- $topologySpreadConstraints := or
.Values.otelCollector.topologySpreadConstraints
.Values.topologySpreadConstraints }}
+{{- $revisionHistoryLimit := include "airflow.revisionHistoryLimit" (list
.Values.otelCollector.revisionHistoryLimit .Values.revisionHistoryLimit) }}
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+ name: {{ include "airflow.fullname" . }}-otel-collector
+ labels:
+ tier: airflow
+ component: otel-collector
+ release: {{ .Release.Name }}
+ chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
+ heritage: {{ .Release.Service }}
+ {{- if or .Values.labels .Values.otelCollector.labels }}
+ {{- mustMerge .Values.otelCollector.labels .Values.labels | toYaml |
nindent 4 }}
+ {{- end }}
+ {{- with .Values.otelCollector.annotations }}
+ annotations: {{- toYaml . | nindent 4 }}
+ {{- end }}
+spec:
+ replicas: 1
+ {{- if ne $revisionHistoryLimit "" }}
+ revisionHistoryLimit: {{ $revisionHistoryLimit }}
+ {{- end }}
+ selector:
+ matchLabels:
+ tier: airflow
+ component: otel-collector
+ release: {{ .Release.Name }}
+ template:
+ metadata:
+ labels:
+ tier: airflow
+ component: otel-collector
+ release: {{ .Release.Name }}
+ {{- if or .Values.labels .Values.otelCollector.labels }}
+ {{- mustMerge .Values.otelCollector.labels .Values.labels | toYaml |
nindent 8 }}
+ {{- end }}
+ annotations:
+ checksum/otel-collector-config: {{ include (print $.Template.BasePath
"/configmaps/otel-collector-configmap.yaml") . | sha256sum }}
+ {{- with .Values.otelCollector.podAnnotations }}
+ {{- tpl (toYaml .) $ | nindent 8 }}
+ {{- end }}
+ spec:
+ nodeSelector: {{- toYaml $nodeSelector | nindent 8 }}
+ affinity: {{- toYaml $affinity | nindent 8 }}
+ tolerations: {{- toYaml $tolerations | nindent 8 }}
+ topologySpreadConstraints: {{- toYaml $topologySpreadConstraints |
nindent 8 }}
+ terminationGracePeriodSeconds: {{
.Values.otelCollector.terminationGracePeriodSeconds }}
+ {{- if .Values.otelCollector.priorityClassName }}
+ priorityClassName: {{ .Values.otelCollector.priorityClassName }}
+ {{- end }}
+ restartPolicy: Always
+ securityContext: {{- toYaml .Values.otelCollector.securityContexts.pod |
nindent 8 }}
+ containers:
+ - name: otel-collector
+ image: {{ template "otel_collector_image" . }}
+ imagePullPolicy: {{ .Values.images.otelCollector.pullPolicy }}
+ securityContext: {{- toYaml
.Values.otelCollector.securityContexts.container | nindent 12 }}
+ args:
+ - "--config=/etc/otel-collector/config.yml"
+ ports:
+ - name: otlp-grpc
+ containerPort: {{ .Values.ports.otelCollectorOtlpGrpc }}
+ protocol: TCP
+ - name: otlp-http
+ containerPort: {{ .Values.ports.otelCollectorOtlpHttp }}
+ protocol: TCP
+ - name: metrics
+ containerPort: {{ .Values.ports.otelCollectorScrape }}
+ protocol: TCP
+ livenessProbe:
+ httpGet:
+ path: /
+ port: 13133
+ initialDelaySeconds: 10
+ periodSeconds: 15
Review Comment:
Depending on size and the number of instances, it may require some
adjustment. Could we make them parametrizable?
##########
chart/templates/otel-collector/otel-collector-service.yaml:
##########
@@ -0,0 +1,59 @@
+{{/*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+*/}}
+
+################################
+## OTel Collector Service
+################################
+{{- if .Values.otelCollector.enabled }}
+apiVersion: v1
+kind: Service
+metadata:
+ name: {{ include "airflow.fullname" . }}-otel-collector
+ labels:
+ tier: airflow
+ component: otel-collector
+ release: {{ .Release.Name }}
+ chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
+ heritage: {{ .Release.Service }}
+ {{- if or .Values.labels .Values.otelCollector.labels }}
+ {{- mustMerge .Values.otelCollector.labels .Values.labels | toYaml |
nindent 4 }}
+ {{- end }}
+ {{- with .Values.otelCollector.service.extraAnnotations }}
Review Comment:
```suggestion
{{- with .Values.otelCollector.service.annotations }}
```
There are no other annotations on the service, so `extra` is misleading.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
