kaxil commented on code in PR #66230:
URL: https://github.com/apache/airflow/pull/66230#discussion_r3175306796
##########
shared/logging/tests/logging/test_issue_66195.py:
##########
@@ -0,0 +1,39 @@
+from __future__ import annotations
+
+from airflow_shared.logging.percent_formatter import PercentFormatRender
+
+
+class TestIssue66195:
Review Comment:
Test files (and classes) should be named after the behavior they verify, not
the issue number. These two methods belong in the existing
`test_percent_formatter.py::TestPercentFormatRender` -- that class already has
`test_no_callsite` and `test_lineno_is_none` covering the same surface. Suggest
deleting this file and adding two methods to the existing class with
descriptive names, e.g. `test_process_thread_missing_returns_zero` and
`test_process_thread_unknown_string_returns_zero`.
##########
shared/logging/tests/logging/test_issue_66195.py:
##########
@@ -0,0 +1,39 @@
+from __future__ import annotations
Review Comment:
The Apache 2.0 license header is missing from this new file. The
`update-license-header` prek hook will fail CI on this. Copy the standard
17-line header from `test_percent_formatter.py:1-17`.
##########
shared/logging/src/airflow_shared/logging/percent_formatter.py:
##########
@@ -53,8 +53,11 @@ def __getitem__(self, key):
# If there is no callsite info (often for stdout/stderr), show the
same sort of thing that stdlib
# logging would
#
https://github.com/python/cpython/blob/d3c888b4ec15dbd7d6b6ef4f15b558af77c228af/Lib/logging/__init__.py#L1652C34-L1652C48
- if key == "lineno":
- return self.event.get("lineno") or 0
+ if key in ("lineno", "thread", "process"):
+ val = self.event.get(key)
+ if val is None or val == "(unknown)":
+ return 0
+ return val
Review Comment:
This is a defensive renderer-level patch, but it loses real diagnostic data
and may be papering over the actual bug.
**1. Root cause is probably upstream.** Look at
`shared/logging/src/airflow_shared/logging/structlog.py:716` -- warning
emission already routes through `CallsiteParameterAdder`. The fact that
`process`/`thread` arrive here as `"(unknown)"` (rather than real PID/TID)
suggests either `additional_ignores=[__name__]` is skipping the wrong frame, or
`reconfigure_logger` isn't keeping the adder in the chain for the `py.warnings`
logger. A fix at that layer would benefit every renderer, not just
`PercentFormatRender`, and would preserve real values in JSON output too.
**2. If we keep a renderer fallback, don't return `0`.** PID and thread ID
are always obtainable from the current process via `os.getpid()` /
`threading.get_ident()`. Returning `0` means every warning-derived log line
will render `pid=0, thread=0` and obscure which scheduler/worker process
actually emitted it -- which defeats the purpose of including those fields in
the log format. Prefer something like:
```python
import os, threading
...
if key == "process":
val = self.event.get("process")
return val if val not in (None, "(unknown)") else os.getpid()
if key == "thread":
val = self.event.get("thread")
return val if val not in (None, "(unknown)") else threading.get_ident()
if key == "lineno":
val = self.event.get("lineno")
return val if val not in (None, "(unknown)") else 0
```
**3. Minor semantic shift on `lineno`.** Old code was
`self.event.get("lineno") or 0` (any falsy value collapsed to 0); the new code
only collapses `None`/`"(unknown)"`. Probably fine, but worth noting in case
anything downstream sets `lineno=""` or `lineno=False`.
##########
airflow-core/newsfragments/66230.bugfix.rst:
##########
Review Comment:
This file was committed with a UTF-8 BOM (`EF BB BF`) and CRLF line endings,
which is why GitHub renders it as a binary diff (`additions: 0, deletions: 0`,
no patch). The towncrier release-notes builder won't pick it up correctly.
Re-create as plain UTF-8 / LF, e.g.:
```bash
rm airflow-core/newsfragments/66230.bugfix.rst
printf 'Fix scheduler crash when ``InsecureKeyLengthWarning`` is emitted
under a log format that uses ``%%(process)d`` or ``%%(thread)d``.\n' >
airflow-core/newsfragments/66230.bugfix.rst
```
Also worth tightening the wording -- right now it just restates the PR
title. A release-notes reader cares about the user-visible symptom: "scheduler
crashes at startup when the configured JWT secret is short enough to trigger
`InsecureKeyLengthWarning`."
--
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]
