[flake8-logging] .exception() and exc_info= outside exception handlers (LOG004, LOG014)

[InSyncWithFoo]

Summary

Part of #7248.

This change adds two new rules: LOG004 and LOG014. They are closely related and thus share the same handlers detecting logic. Tests and documentation are adapted from that of the upstream linter.

Test Plan

cargo nextest run and cargo insta test.

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+38 -0 violations, +0 -0 fixes in 4 projects; 51 projects unchanged)

apache/airflow (+12 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL

+ providers/edge/src/airflow/providers/edge/worker_api/auth.py:59:5: LOG004 `.exception()` call outside exception handlers
+ providers/edge/src/airflow/providers/edge/worker_api/routes/_v2_routes.py:56:5: LOG004 `.exception()` call outside exception handlers
+ providers/src/airflow/providers/cncf/kubernetes/operators/pod.py:1290:13: LOG004 `.exception()` call outside exception handlers
+ providers/src/airflow/providers/google/cloud/operators/cloud_sql.py:1053:13: LOG004 `.exception()` call outside exception handlers
+ providers/src/airflow/providers/google/cloud/operators/dataproc.py:1754:13: LOG004 `.exception()` call outside exception handlers
+ providers/src/airflow/providers/google/cloud/operators/dataproc.py:1915:13: LOG004 `.exception()` call outside exception handlers
+ providers/src/airflow/providers/google/cloud/operators/dataproc.py:2379:17: LOG004 `.exception()` call outside exception handlers
+ providers/src/airflow/providers/google/cloud/operators/kubernetes_engine.py:305:13: LOG004 `.exception()` call outside exception handlers
... 4 additional changes omitted for rule LOG004
+ task_sdk/src/airflow/sdk/execution_time/supervisor.py:741:13: LOG014 `exc_info=` outside exception handlers

apache/superset (+15 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL

+ superset/commands/report/base.py:106:75: LOG014 `exc_info=` outside exception handlers
+ superset/db_engine_specs/clickhouse.py:181:17: LOG014 `exc_info=` outside exception handlers
+ superset/db_engine_specs/hive.py:592:13: LOG014 `exc_info=` outside exception handlers
+ superset/sql_lab.py:137:69: LOG014 `exc_info=` outside exception handlers
+ superset/sql_lab.py:141:75: LOG014 `exc_info=` outside exception handlers
+ superset/tasks/cache.py:298:31: LOG014 `exc_info=` outside exception handlers
+ superset/utils/core.py:572:43: LOG014 `exc_info=` outside exception handlers
+ superset/utils/decorators.py:231:13: LOG004 `.exception()` call outside exception handlers
+ superset/views/error_handling.py:140:50: LOG014 `exc_info=` outside exception handlers
+ superset/views/error_handling.py:145:51: LOG014 `exc_info=` outside exception handlers
+ superset/views/error_handling.py:151:52: LOG014 `exc_info=` outside exception handlers
... 4 additional changes omitted for rule LOG014
+ superset/views/error_handling.py:210:9: LOG004 `.exception()` call outside exception handlers

rotki/rotki (+2 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview

+ rotkehlchen/api/server.py:438:13: LOG004 `.exception()` call outside exception handlers
+ rotkehlchen/api/server.py:441:13: LOG014 `exc_info=` outside exception handlers

zulip/zulip (+9 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL

+ zerver/decorator.py:333:5: LOG004 `.exception()` call outside exception handlers
+ zerver/decorator.py:342:9: LOG004 `.exception()` call outside exception handlers
+ zerver/decorator.py:344:9: LOG004 `.exception()` call outside exception handlers
+ zerver/decorator.py:346:9: LOG004 `.exception()` call outside exception handlers
+ zerver/lib/push_notifications.py:1334:21: LOG014 `exc_info=` outside exception handlers
+ zerver/lib/remote_server.py:291:9: LOG004 `.exception()` call outside exception handlers
+ zerver/worker/base.py:265:17: LOG004 `.exception()` call outside exception handlers
... 3 additional changes omitted for rule LOG004

Changes by rule (2 rules affected)

code	total	+ violation	- violation	+ fix	- fix
LOG004	22	22	0	0	0
LOG014	16	16	0	0	0

[InSyncWithFoo]

They could have been defined in the same module/function, but I was worried that there might be too much coupling between them. I'll merge them if that is indeed the better approach.

[InSyncWithFoo]

I just realized #14682 exists (which, unfortunately, hasn't been active for more than a month). I'm willing to remove my LOG014 in favor of that and rewrite LOG004 accordingly if that is desirable.

[bluetech]

I see that the code does handle it, but I think it would be good to test that the lint doesn't trigger for exc_info=False, exc_info=exc.

[InSyncWithFoo]

@bluetech Done for the former and already done for the latter.

[MichaReiser]

Thanks. This overall looks good. We should add some tests with custom logger objects (in dedicated files) to see if the seen_module short-circuit check is valid.

[MichaReiser]

The ecosystem check also report a few false-positives e.g:

https://github.com/apache/airflow/blob/41403c1221025f5ffe737f1a8e2c488212b3c493/providers/edge/src/airflow/providers/edge/worker_api/auth.py#L59

This was discussed in your linked PR, see #14682 (comment) and #14682 (comment)

I'm interested in your opinion on this.

I'm somewhat leaning towards using noqa comments for such functions because:

The function should document that it should only be called from inside a except handler and calling it outside of it may lead to incorrect result. This is similar to documenting unsafe in Rust.

Did you review the other ecosystem checks? Considering that there are more LOG014, maybe consider splitting the PR into two?

[InSyncWithFoo]

Did you review the other ecosystem checks?

I'm aware of them. The original rule also intentionally reports them, so I think it's fine.

As for splitting, I'm not sure. They are closely related enough.

[MichaReiser]

Huh, this one is interesting. Would you mind taking a look at why it is flagged

https://github.com/apache/airflow/blob/3004da95e97ba79eba2ab6b743a75e3f3f8dc170/tests/core/test_settings.py#L168

[InSyncWithFoo]

@MichaReiser Fixed. The method name should also have been checked.

[MichaReiser]

Thanks