You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "ersalil (via GitHub)" <gi...@apache.org> on 2026/06/09 13:00:00 UTC
[PR] Fix DagProcessor crash: add missing name_is_otel_safe() guard to gauge() and timer() [airflow]
ersalil opened a new pull request, #68284:
URL: https://github.com/apache/airflow/pull/68284
## Summary
`SafeOtelLogger.gauge()` and `timer()` were missing the `name_is_otel_safe()` guard that `incr()`, `decr()`, and `timing()` all have. A DAG filename containing a space (e.g. `PBI_SKU_Performance copy.py`) caused the DagProcessor to crash on every loop iteration when OTel metrics were enabled.
## Why
The DagProcessor emits a gauge metric for every known DAG file on every parsing loop:
```python
Stats.gauge(f"dag_processing.last_run.seconds_ago.{file_name}", seconds_ago)
```
`gauge()` only validated against the allow/block list (`metrics_validator.test()`), which does not check OTel naming rules. A filename with a space passed that check and reached `meter.create_gauge()` inside the OTel SDK, which raised a plain Exception that propagated uncaught and crashed the process:
```
Exception: Expected ASCII string of maximum length 63 characters but got airflow.dag_processing.last_run.seconds_ago.PBI_SKU_Performance copy
```
Note: the OTel SDK error message says "63 characters" but this is a stale message — the actual SDK validation regex allows up to 255 characters (matching OTEL_NAME_MAX_LENGTH). The real rejection reason is the space character, which is not a valid OTel instrument name character. See [open-telemetry/opentelemetry-python#3442](https://github.com/open-telemetry/opentelemetry-python/pull/3442).
The same gap existed in `timer()`: it had no validation at all before being passed to `_OtelTimer`, which would then call `record_histogram_value()` with the invalid name at `.stop()` time.
## Fix
Added `metrics_validator.test(stat) and name_is_otel_safe(self.prefix, stat)` to `gauge()` (both the `stat` and `back_compat_name` paths) and `timer()`, making all five recording methods consistent:
| Method | Guard before | Guard after |
|--------|-------------|-------------|
| `incr()` | ✅ both checks | ✅ unchanged |
| `decr()` | ✅ both checks | ✅ unchanged |
| `timing()` | ✅ both checks | ✅ unchanged |
| `gauge()` | ❌ validator only | ✅ both checks |
| `timer()` | ❌ none | ✅ both checks |
## Testing
- `test_gauge_with_invalid_stat_names_skipped_without_raising` — covers the exact reported scenario (space in filename) and non-ASCII, both reproducing the DagProcessor crash path.
- `test_timer_with_invalid_stat_name_does_not_record` — covers non-ASCII and space for the timer path.
closes: #68282
##### Was generative AI tooling used to co-author this PR?
- [X] Yes — Claude Code (Sonnet 4.6)
This PR was prepared with Gen-AI assistance (Claude). I reviewed all generated code.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix DagProcessor crash: add missing name_is_otel_safe() guard to gauge() and timer() [airflow]
Posted by "ersalil (via GitHub)" <gi...@apache.org>.
ersalil commented on code in PR #68284:
URL: https://github.com/apache/airflow/pull/68284#discussion_r3384618015
##########
shared/observability/src/airflow_shared/observability/metrics/otel_logger.py:
##########
@@ -299,7 +303,12 @@ def timer(
**kwargs,
) -> Timer:
"""Timer context manager returns the duration and can be cancelled."""
- return _OtelTimer(self, stat, tags)
+ safe_stat = (
+ stat
+ if stat is not None and self.metrics_validator.test(stat) and name_is_otel_safe(self.prefix, stat)
Review Comment:
Thanks @kaxil , great suggestion! Done — pulled it into a _is_recordable() helper and routed all six checks (incr, decr, gauge ×2, timing, timer) through it so none can miss the guard. 🙏
Drafted-by: Claude Code (Opus 4.8); reviewed by @ersalil before posting
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix DagProcessor crash: add missing name_is_otel_safe() guard to gauge() and timer() [airflow]
Posted by "ersalil (via GitHub)" <gi...@apache.org>.
ersalil commented on code in PR #68284:
URL: https://github.com/apache/airflow/pull/68284#discussion_r3384685456
##########
shared/observability/src/airflow_shared/observability/metrics/otel_logger.py:
##########
@@ -270,12 +270,16 @@ def gauge(
if _skip_due_to_rate(rate):
return
- if back_compat_name and self.metrics_validator.test(back_compat_name):
+ if (
+ back_compat_name
+ and self.metrics_validator.test(back_compat_name)
+ and name_is_otel_safe(self.prefix, back_compat_name)
+ ):
self.metrics_map.set_gauge_value(
full_name(prefix=self.prefix, name=back_compat_name), value, delta, tags
)
- if self.metrics_validator.test(stat):
+ if self.metrics_validator.test(stat) and name_is_otel_safe(self.prefix, stat):
Review Comment:
Good point. That makes sense! Since it matches incr()'s current behaviour, I've kept this PR focused on the crash fix for now. The log-once dedupe sounds like a good follow-up — happy to pick it up separately, or here if you'd prefer. Let me know what works best.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix DagProcessor crash: add missing name_is_otel_safe() guard to gauge() and timer() [airflow]
Posted by "ersalil (via GitHub)" <gi...@apache.org>.
ersalil commented on PR #68284:
URL: https://github.com/apache/airflow/pull/68284#issuecomment-4669659606
Thanks a lot for the review, @kaxil! I've pushed the changes:
- Pulled the safety check into a _is_recordable() helper so all six recording methods (incr, decr, gauge, timing, timer) share the same guard and none can miss it.
- Fixed the mypy narrowing (str | None → str) it surfaced.
All CI checks are green now. Really appreciate the guidance — let me know if there's anything else you'd like me to adjust!
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix DagProcessor crash: add missing name_is_otel_safe() guard to gauge() and timer() [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on code in PR #68284:
URL: https://github.com/apache/airflow/pull/68284#discussion_r3384350636
##########
shared/observability/src/airflow_shared/observability/metrics/otel_logger.py:
##########
@@ -299,7 +303,12 @@ def timer(
**kwargs,
) -> Timer:
"""Timer context manager returns the duration and can be cancelled."""
- return _OtelTimer(self, stat, tags)
+ safe_stat = (
+ stat
+ if stat is not None and self.metrics_validator.test(stat) and name_is_otel_safe(self.prefix, stat)
Review Comment:
The `metrics_validator.test(stat) and name_is_otel_safe(self.prefix, stat)` predicate now appears six times across `incr`, `decr`, `gauge` (twice), `timing`, and `timer`, and this bug happened because two methods missed it. Maybe pull it into a helper so the next recording method can't forget it:
```python
def _is_recordable(self, stat: str | None) -> bool:
return bool(stat) and self.metrics_validator.test(stat) and name_is_otel_safe(self.prefix, stat)
```
##########
shared/observability/src/airflow_shared/observability/metrics/otel_logger.py:
##########
@@ -270,12 +270,16 @@ def gauge(
if _skip_due_to_rate(rate):
return
- if back_compat_name and self.metrics_validator.test(back_compat_name):
+ if (
+ back_compat_name
+ and self.metrics_validator.test(back_compat_name)
+ and name_is_otel_safe(self.prefix, back_compat_name)
+ ):
self.metrics_map.set_gauge_value(
full_name(prefix=self.prefix, name=back_compat_name), value, delta, tags
)
- if self.metrics_validator.test(stat):
+ if self.metrics_validator.test(stat) and name_is_otel_safe(self.prefix, stat):
Review Comment:
FYI this will spam the logs for the reported case: `name_is_otel_safe()` logs `Invalid stat name: ...` every time the DagProcessor emits this gauge, so `PBI_SKU_Performance copy.py` produces a recurring warning for as long as the file exists. `incr()` already does the same for invalid names, so not blocking. If the noise comes up later, a log-once-per-name dedupe in `name_is_otel_safe` would handle 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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix DagProcessor crash: add missing name_is_otel_safe() guard to gauge() and timer() [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil merged PR #68284:
URL: https://github.com/apache/airflow/pull/68284
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org