jason810496 commented on code in PR #53598:
URL: https://github.com/apache/airflow/pull/53598#discussion_r2280748143
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -343,6 +343,8 @@ def __init__(
progress_callback: Callable[[str], None] | None = None,
logging_interval: int | None = None,
trigger_kwargs: dict | None = None,
+ log_prefix: bool = True,
+ log_formatter: Callable[[str, str], str] | None = None,
Review Comment:
The docstring of `container_name_log_prefix_enabled` and `log_formatter` is
missing.
https://github.com/apache/airflow/blob/16ee837a91cf9ee48c1596197b4df41bcb46ee29/providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/pod.py#L237-L238
Additionally, it would be nice to describe the type of `log_formatter` in
the docstring, as first str is `container_name` and second str is
`message_to_log`.
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -343,6 +343,8 @@ def __init__(
progress_callback: Callable[[str], None] | None = None,
logging_interval: int | None = None,
trigger_kwargs: dict | None = None,
+ log_prefix: bool = True,
Review Comment:
Would it be better to rename `log_prefix` to
`container_name_log_prefix_enabled`?
Since the purpose of the `log_prefix` flag is to control whether the
`[container_name]` prefix appears in the logs or not.
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -532,7 +534,17 @@ def consume_logs(*, since_time: DateTime | None = None) ->
tuple[DateTime | None
if is_log_group_marker(message_to_log):
print(message_to_log)
else:
- self.log.info("[%s] %s",
container_name, message_to_log)
+ # Apply custom formatter or prefix
logic
+ if log_formatter:
+ formatted_message =
log_formatter(container_name, message_to_log)
+ self.log.info("%s",
formatted_message)
+ else:
+ log_message = (
+ f"[{container_name}]
{message_to_log}"
+ if log_prefix
+ else message_to_log
+ )
+ self.log.info("%s", log_message)
Review Comment:
It seems we can add a common function to avoid duplicate logic.
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -551,7 +563,14 @@ def consume_logs(*, since_time: DateTime | None = None) ->
tuple[DateTime | None
if is_log_group_marker(message_to_log):
print(message_to_log)
else:
- self.log.info("[%s] %s", container_name,
message_to_log)
+ if log_formatter:
+ formatted_message =
log_formatter(container_name, message_to_log)
+ self.log.info("%s", formatted_message)
+ else:
+ log_message = (
+ f"[{container_name}] {message_to_log}" if
log_prefix else message_to_log
+ )
+ self.log.info("%s", log_message)
Review Comment:
Since the above logic is exactly same here.
--
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]
