Re: [PR] Add metrics_allow_list and metrics_block_list to statsd deployments env [airflow]
jedcunningham commented on code in PR #38697:
URL: https://github.com/apache/airflow/pull/38697#discussion_r1628091310
##
chart/templates/statsd/statsd-deployment.yaml:
##
@@ -104,8 +104,17 @@ spec:
- "--statsd.mapping-config=/etc/statsd-exporter/mappings.yml"
{{- end }}
resources: {{- toYaml .Values.statsd.resources | nindent 12 }}
- {{- with .Values.statsd.env }}
- env: {{- toYaml . | nindent 12 }}
+ env:
+ {{- if .Values.statsd.env }}
+ {{- toYaml .Values.statsd.env | nindent 12 }}
+ {{- end }}
+ {{- if .Values.config.metrics.metrics_allow_list }}
+- name: AIRFLOW__METRICS__METRICS_ALLOW_LIST
Review Comment:
Not sure about defaulting it, but if you want to expose it as an optional
config, go for it!
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Add metrics_allow_list and metrics_block_list to statsd deployments env [airflow]
Xiroo commented on code in PR #38697:
URL: https://github.com/apache/airflow/pull/38697#discussion_r1595036924
##
chart/templates/statsd/statsd-deployment.yaml:
##
@@ -104,8 +104,17 @@ spec:
- "--statsd.mapping-config=/etc/statsd-exporter/mappings.yml"
{{- end }}
resources: {{- toYaml .Values.statsd.resources | nindent 12 }}
- {{- with .Values.statsd.env }}
- env: {{- toYaml . | nindent 12 }}
+ env:
+ {{- if .Values.statsd.env }}
+ {{- toYaml .Values.statsd.env | nindent 12 }}
+ {{- end }}
+ {{- if .Values.config.metrics.metrics_allow_list }}
+- name: AIRFLOW__METRICS__METRICS_ALLOW_LIST
Review Comment:
Thank you for your advice.
Then, I'm wondering is it bad idea adding a default TTL value here, aimed at
ensuring they expire after a specified period of no updates.
I think it is distracting if metrics are not expired after changing
metrics_allow_list and metrics_block_list so it is needed for others.
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Add metrics_allow_list and metrics_block_list to statsd deployments env [airflow]
jedcunningham commented on code in PR #38697:
URL: https://github.com/apache/airflow/pull/38697#discussion_r1594326160
##
chart/templates/statsd/statsd-deployment.yaml:
##
@@ -104,8 +104,17 @@ spec:
- "--statsd.mapping-config=/etc/statsd-exporter/mappings.yml"
{{- end }}
resources: {{- toYaml .Values.statsd.resources | nindent 12 }}
- {{- with .Values.statsd.env }}
- env: {{- toYaml . | nindent 12 }}
+ env:
+ {{- if .Values.statsd.env }}
+ {{- toYaml .Values.statsd.env | nindent 12 }}
+ {{- end }}
+ {{- if .Values.config.metrics.metrics_allow_list }}
+- name: AIRFLOW__METRICS__METRICS_ALLOW_LIST
Review Comment:
We shouldn't inject these env vars just to cause a restart. Annotations is
the "right way" to cause that behavior.
That said, this is just how the statsd world behaves by default. e.g.
[deleteIdleStats in StatsD
itself](https://github.com/statsd/statsd/blob/7c07eec4e7cebbd376d8313b230cea96c6571423/exampleConfig.js#L61C3-L63).
I believe the equivalent is [ttl in
statsd-exporter](https://github.com/prometheus/statsd_exporter/blob/58769c7b4d128e7ceae1cf8d893260ed5b5afa4d/README.md?plain=1#L442).
You can probably set it with `statsd.overrideMappings`.
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Add metrics_allow_list and metrics_block_list to statsd deployments env [airflow]
romsharon98 commented on PR #38697: URL: https://github.com/apache/airflow/pull/38697#issuecomment-2067948902 @eladkal looks good, can we merge it? -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add metrics_allow_list and metrics_block_list to statsd deployments env [airflow]
Xiroo commented on code in PR #38697:
URL: https://github.com/apache/airflow/pull/38697#discussion_r1554627220
##
chart/templates/statsd/statsd-deployment.yaml:
##
@@ -104,8 +104,17 @@ spec:
- "--statsd.mapping-config=/etc/statsd-exporter/mappings.yml"
{{- end }}
resources: {{- toYaml .Values.statsd.resources | nindent 12 }}
- {{- with .Values.statsd.env }}
- env: {{- toYaml . | nindent 12 }}
+ env:
Review Comment:
There's empty list of value in default values file.
So maybe it'll be empty list if user did not intentionally set env without
any value.
Please let me know if this is wrong or a bad case
https://github.com/apache/airflow/blob/4d65f216dec5705f82262d9c4a9f799e92b55944/chart/values.yaml#L1924
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Add metrics_allow_list and metrics_block_list to statsd deployments env [airflow]
romsharon98 commented on code in PR #38697:
URL: https://github.com/apache/airflow/pull/38697#discussion_r1554628445
##
chart/templates/statsd/statsd-deployment.yaml:
##
@@ -104,8 +104,17 @@ spec:
- "--statsd.mapping-config=/etc/statsd-exporter/mappings.yml"
{{- end }}
resources: {{- toYaml .Values.statsd.resources | nindent 12 }}
- {{- with .Values.statsd.env }}
- env: {{- toYaml . | nindent 12 }}
+ env:
Review Comment:
My bad, looking like it's a good practice to do so
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Add metrics_allow_list and metrics_block_list to statsd deployments env [airflow]
Xiroo commented on code in PR #38697:
URL: https://github.com/apache/airflow/pull/38697#discussion_r1554627220
##
chart/templates/statsd/statsd-deployment.yaml:
##
@@ -104,8 +104,17 @@ spec:
- "--statsd.mapping-config=/etc/statsd-exporter/mappings.yml"
{{- end }}
resources: {{- toYaml .Values.statsd.resources | nindent 12 }}
- {{- with .Values.statsd.env }}
- env: {{- toYaml . | nindent 12 }}
+ env:
Review Comment:
There's empty list of value in default value metrics.
So maybe it'll be empty list if user did not intentionally set env without
any value.
Please let me know if this is wrong or bad case
https://github.com/apache/airflow/blob/4d65f216dec5705f82262d9c4a9f799e92b55944/chart/values.yaml#L1924
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Add metrics_allow_list and metrics_block_list to statsd deployments env [airflow]
Xiroo commented on code in PR #38697:
URL: https://github.com/apache/airflow/pull/38697#discussion_r1554627220
##
chart/templates/statsd/statsd-deployment.yaml:
##
@@ -104,8 +104,17 @@ spec:
- "--statsd.mapping-config=/etc/statsd-exporter/mappings.yml"
{{- end }}
resources: {{- toYaml .Values.statsd.resources | nindent 12 }}
- {{- with .Values.statsd.env }}
- env: {{- toYaml . | nindent 12 }}
+ env:
Review Comment:
There's empty list of value in default value metrics.
So maybe it'll be empty list if user did not intentionally set env without
any value.
Please let me know if this is wrong or a bad case
https://github.com/apache/airflow/blob/4d65f216dec5705f82262d9c4a9f799e92b55944/chart/values.yaml#L1924
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Add metrics_allow_list and metrics_block_list to statsd deployments env [airflow]
Xiroo commented on code in PR #38697:
URL: https://github.com/apache/airflow/pull/38697#discussion_r1554627220
##
chart/templates/statsd/statsd-deployment.yaml:
##
@@ -104,8 +104,17 @@ spec:
- "--statsd.mapping-config=/etc/statsd-exporter/mappings.yml"
{{- end }}
resources: {{- toYaml .Values.statsd.resources | nindent 12 }}
- {{- with .Values.statsd.env }}
- env: {{- toYaml . | nindent 12 }}
+ env:
Review Comment:
There's empty list of value in default value metrics.
So maybe it'll be empty list if user did not intensively set env without any
value.
Please let me know if this is wrong or bad case
https://github.com/apache/airflow/blob/4d65f216dec5705f82262d9c4a9f799e92b55944/chart/values.yaml#L1924
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Add metrics_allow_list and metrics_block_list to statsd deployments env [airflow]
romsharon98 commented on PR #38697: URL: https://github.com/apache/airflow/pull/38697#issuecomment-2041092273 Except this looks good -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add metrics_allow_list and metrics_block_list to statsd deployments env [airflow]
romsharon98 commented on code in PR #38697:
URL: https://github.com/apache/airflow/pull/38697#discussion_r1554584784
##
chart/templates/statsd/statsd-deployment.yaml:
##
@@ -104,8 +104,17 @@ spec:
- "--statsd.mapping-config=/etc/statsd-exporter/mappings.yml"
{{- end }}
resources: {{- toYaml .Values.statsd.resources | nindent 12 }}
- {{- with .Values.statsd.env }}
- env: {{- toYaml . | nindent 12 }}
+ env:
Review Comment:
If no environment you will have
```python
env:
```
without any value. I this is is valid chart but don't think its good.
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[PR] Add metrics_allow_list and metrics_block_list to statsd deployments env [airflow]
Xiroo opened a new pull request, #38697:
URL: https://github.com/apache/airflow/pull/38697
We need to ensure that the metrics_allow_list and metrics_block_list are
added to the StatsD pod's environment variables. This update is crucial for
refreshing the pod configuration effectively.
Initially configuring the StatsD pod without these lists and adding them
later can lead to inconsistencies. Specifically, while changes to
metrics_allow_list or metrics_block_list update the airflow.cfg and trigger a
refresh of the related Airflow components (like the scheduler and webserver),
the StatsD pod does not get automatically updated because its configuration
remains unchanged.
This behavior results in the metrics not being updated according to the
newly set rules, although the fields themselves are not deleted—they simply
stop updating. For example, when using the StatsD scraper for Prometheus, I
noticed that metrics intended to be blocked were still present, albeit without
updates. Initially, I suspected an issue with the block list and allow list
functionality, but it turned out that the real issue was the StatsD pod not
being refreshed, causing the old metric fields to persist.
Therefore, it seems logical to include these configurations directly in the
StatsD pod's environment variables, ensuring the pod is replaced and updated
whenever these settings are configured.
---
**^ Add meaningful description above**
Read the **[Pull Request
Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)**
for more information.
In case of fundamental code changes, an Airflow Improvement Proposal
([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals))
is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party
License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a
newsfragment file, named `{pr_number}.significant.rst` or
`{issue_number}.significant.rst`, in
[newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
