jedcunningham commented on code in PR #25219:
URL: https://github.com/apache/airflow/pull/25219#discussion_r939123022


##########
chart/values.yaml:
##########
@@ -1314,9 +1314,96 @@ statsd:
 
   priorityClassName: ~
 
+  histogramBuckets:
+    - 0.005
+    - 0.01
+    - 0.025
+    - 0.05
+    - 0.1
+    - 0.25
+    - 0.5
+    - 1
+    - 2.5
+
+  defaultMappings:

Review Comment:
   I wonder if we should call this '`mappings' instead`?



##########
chart/values.yaml:
##########
@@ -1314,9 +1314,96 @@ statsd:
 
   priorityClassName: ~
 
+  histogramBuckets:
+    - 0.005
+    - 0.01
+    - 0.025
+    - 0.05
+    - 0.1
+    - 0.25
+    - 0.5
+    - 1
+    - 2.5
+
+  defaultMappings:
+    # Map dot separated stats to labels
+    - match: airflow.dagrun.dependency-check.*.*
+      name: "airflow_dagrun_dependency_check"
+      labels:
+        dag_id: "$1"
+    - match: airflow.ti_failures
+      name: "airflow_ti_tasks_failures_total"
+    - match: airflow.ti_successes
+      name: "airflow_ti_tasks_succeeded_total"
+    - match: airflow.ti.start.*.*
+      name: "airflow_ti_tasks_started_total"
+      labels:
+        dag_id: "$1"
+        task_id: "$2"
+    - match: airflow.ti.finish.*.*.*
+      name: "airflow_ti_tasks_finished_total"
+      labels:
+        dag_id: "$1"
+        task_id: "$2"
+        task_state: "$3"

Review Comment:
   These are all new, right? I think this will also break backwards 
compatibility, no?



##########
chart/templates/configmaps/statsd-configmap.yaml:
##########
@@ -34,6 +34,20 @@ metadata:
 {{- end }}
 data:
   mappings.yml: |-
-{{ .Files.Get "dockerfiles/statsd-exporter/mappings.yml" | indent 4 }}
+    defaults:
+      observer_type: histogram
+      histogram_options:
+        buckets:
+{{ toYaml .Values.statsd.histogramBuckets | indent 10 }}

Review Comment:
   So looking at this from a backwards compatibility lens, I wonder if we 
should default histogramBuckets to null and only add this if a user elects to.
   
   I'm not familiar enough to know if this is a breaking change or not in 
prometheus.
   
   For example, before it was like this:
   
   ```
   airflow_dag_processing_last_duration_hello_world{quantile="0.5"} 0.122281
   airflow_dag_processing_last_duration_hello_world{quantile="0.9"} 0.146598
   airflow_dag_processing_last_duration_hello_world{quantile="0.99"} 0.234025
   ```
   With this change, it looks like this:
   
   ```
   airflow_dag_processing_last_duration_hello_world_bucket{le="0.005"} 0
   airflow_dag_processing_last_duration_hello_world_bucket{le="0.01"} 0 
   ...
   airflow_dag_processing_last_duration_hello_world_bucket{le="2.5"} 23 
   airflow_dag_processing_last_duration_hello_world_bucket{le="+Inf"} 23        
  
   ```



-- 
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]

Reply via email to