Use lazy logging instead of f-strings in logger calls#2672
Conversation
There was a problem hiding this comment.
Pull request overview
This PR standardizes Cosmos’ logging style by replacing eager f-string interpolation in logger calls with lazy %s formatting, preserving the format template in record.msg and avoiding unnecessary string construction when logs are filtered out.
Changes:
- Converted remaining
logger.{debug,info,warning}(f"...")call sites incosmos/to lazy logging (logger.info("...", arg)). - Fixed one mixed-style watcher log message to be fully lazy-formatted.
- Updated
tests/plugin/test_cluster_policy.pyassertions to match the new(msg_template, *args)logging convention.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/plugin/test_cluster_policy.py | Updates mock logger assertions to expect format string + args. |
| cosmos/versioning.py | Converts warning about broken symlink targets to lazy logging. |
| cosmos/plugin/cluster_policy.py | Converts queue-setting info log to lazy logging. |
| cosmos/operators/virtualenv.py | Converts virtualenv lock acquisition/release logs to lazy logging. |
| cosmos/operators/local.py | Converts XCom push info log to lazy logging. |
| cosmos/operators/_watcher/base.py | Converts fallback/retry logs to fully lazy logging (including class name). |
| cosmos/listeners/dag_run_listener.py | Converts telemetry decompression warning to lazy logging. |
| cosmos/dbt/selector.py | Converts “selector not found” warning to lazy logging. |
| cosmos/dbt/project.py | Converts several project/package helper logs to lazy logging. |
| cosmos/dbt/parser/project.py | Converts parser warnings to lazy logging. |
| cosmos/dbt/graph.py | Converts cache-related debug/info logs to lazy logging (including multi-arg performance logs). |
| cosmos/core/graph/entities.py | Converts “adding entity” info log to lazy logging. |
| cosmos/converter.py | Converts performance/debug/warning logs in converter to lazy logging. |
| cosmos/cache.py | Converts cache versioning and deletion logs to lazy logging. |
| cosmos/airflow/graph.py | Converts conversion debug logs to lazy logging. |
Comments suppressed due to low confidence (1)
cosmos/cache.py:469
- The log message has a double space before the
%splaceholder ("for %s"). Consider removing the extra space to avoid inconsistent log templates between similar messages.
logger.info("Delete the Cosmos cache stored remotely that hasn't been used for %s", max_age_last_usage)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
cosmos/cache.py:469
- There appears to be an unintended double space before the "%s" placeholder ("for %s"). Consider removing the extra space to keep log output consistent and easier to search.
logger.info("Delete the Cosmos cache stored remotely that hasn't been used for %s", max_age_last_usage)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2672 +/- ##
=======================================
Coverage 98.03% 98.03%
=======================================
Files 105 105
Lines 7843 7843
=======================================
Hits 7689 7689
Misses 154 154 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The codebase predominantly uses lazy logging
(logger.info("msg %s", arg)) but ~47 call sites across 15 files
still embedded f-strings into the log message. Eager formatting
defeats the point of lazy logging: the message is built even when
the log level filters the record out, and structured-log handlers
that key on record.msg can no longer match on the format template.
Convert every `logger.{info,warning,error,debug,exception}(f"...")`
in cosmos/ to the lazy form, fixing one mixed-style call in
cosmos/operators/_watcher/base.py that interpolated
self.__class__.__name__ via f-string while using %s for other
arguments.
Update tests/plugin/test_cluster_policy.py to assert against the
new format-string + args calling convention.
No behaviour change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Fix the remaining double space in the remote cache deletion log template, and switch the virtualenv lock log messages to self.log so they pick up the Airflow task context (task_id/dag_id) like the rest of the operator. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The two dag-versioning tests asserted on the eagerly-formatted log message, which no longer holds now that converter.py uses lazy logging. Inspect the format string and args separately so the tests match the new calling convention. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
79d13a2 to
eca00b8
Compare
tatiana
left a comment
There was a problem hiding this comment.
Thanks, @pankajastro , looks great.
Follow-up to #2672. The original sweep caught ~47 single-line logger.X(f"...") sites but missed two patterns: 1. Multi-line calls of the form logger.exception( f"..." ) in cosmos/plugin/airflow3.py (three sites) and cosmos/dbt/parser/output.py (one site). The original regex required the f-string on the same line as the logger call. 2. self.log.X(f"...") calls in operators that inherit Airflow's LoggingMixin. These never matched the original logger.X(f"...") pattern even though they violate the lazy logging rule identically. Affects azure_container_instance, docker, kubernetes, aws_ecs, gcp_cloud_run_job, and virtualenv operators. Also converts one eager percent-interpolation call in cosmos/cache.py ("%s" % repr(e)) that used %-formatting instead of an f-string and so was outside the scope of #2672. No behaviour change. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The codebase predominantly uses lazy logging
(logger.info("msg %s", arg)) but ~47 call sites across 15 files still embedded f-strings into the log message. Eager formatting defeats the point of lazy logging: the message is built even when the log level filters the record out, and structured-log handlers that key on record.msg can no longer match on the format template.
Convert every
logger.{info,warning,error,debug,exception}(f"...")in cosmos/ to the lazy form, fixing one mixed-style call in cosmos/operators/_watcher/base.py that interpolatedself.__class__.__name__via f-string while using %s for other arguments.Update tests/plugin/test_cluster_policy.py to assert against the new format-string + args calling convention.
No behaviour change.