Re: [PR] Rework StackdriverTaskHandler for the structlog era #65191 [airflow]
shahar1 merged PR #65198: URL: https://github.com/apache/airflow/pull/65198 -- 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]
Re: [PR] Rework StackdriverTaskHandler for the structlog era #65191 [airflow]
ashb commented on PR #65198: URL: https://github.com/apache/airflow/pull/65198#issuecomment-4545725062 Taking a look now -- 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]
Re: [PR] Rework StackdriverTaskHandler for the structlog era #65191 [airflow]
shahar1 commented on PR #65198: URL: https://github.com/apache/airflow/pull/65198#issuecomment-4524319337 LGTM, @ashb? -- 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]
Re: [PR] Rework StackdriverTaskHandler for the structlog era #65191 [airflow]
haseebmalik18 commented on PR #65198: URL: https://github.com/apache/airflow/pull/65198#issuecomment-4524274728 @shahar1 Just following up on this whenever you get a chance -- 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]
Re: [PR] Rework StackdriverTaskHandler for the structlog era #65191 [airflow]
haseebmalik18 closed pull request #65198: Rework StackdriverTaskHandler for the structlog era #65191 URL: https://github.com/apache/airflow/pull/65198 -- 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]
Re: [PR] Rework StackdriverTaskHandler for the structlog era #65191 [airflow]
potiuk commented on PR #65198: URL: https://github.com/apache/airflow/pull/65198#issuecomment-4476939776 @haseebmalik18 — There are 1 unresolved review thread on this PR from @eladkal. Could you either push a fix or reply in each thread explaining why the feedback doesn't apply? Once you believe the feedback is addressed, mark the thread as resolved so the reviewer isn't re-pinged needlessly. Thanks! --- _Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this [two-stage triage process](https://github.com/apache/airflow/blob/main/contributing-docs/25_maintainer_pr_triage.md#why-the-first-pass-is-automated) so that our maintainers' limited time is spent where it matters most: the conversation with you._ -- 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]
Re: [PR] Rework StackdriverTaskHandler for the structlog era #65191 [airflow]
haseebmalik18 commented on PR #65198: URL: https://github.com/apache/airflow/pull/65198#issuecomment-433906 Made the requested changes -- 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]
Re: [PR] Rework StackdriverTaskHandler for the structlog era #65191 [airflow]
shahar1 commented on code in PR #65198: URL: https://github.com/apache/airflow/pull/65198#discussion_r3223550467 ## providers/google/src/airflow/providers/google/cloud/log/stackdriver_task_handler.py: ## @@ -56,6 +68,204 @@ ["https://www.googleapis.com/auth/logging.read";, "https://www.googleapis.com/auth/logging.write";] ) +LABEL_TASK_ID = "task_id" +LABEL_DAG_ID = "dag_id" +LABEL_LOGICAL_DATE = "logical_date" if AIRFLOW_V_3_0_PLUS else "execution_date" +LABEL_TRY_NUMBER = "try_number" + + [email protected](kw_only=True) +class StackdriverRemoteLogIO(LoggingMixin): +"""Remote log IO that streams logs to and reads from Google Cloud Stackdriver Logging.""" + +base_log_folder: Path = attrs.field(converter=Path) +delete_local_copy: bool = True + +gcp_key_path: str | None = None +scopes: Collection[str] | None = _DEFAULT_SCOPESS +gcp_log_name: str = DEFAULT_LOGGER_NAME +transport_type: type[Transport] = BackgroundThreadTransport +resource: Resource = _GLOBAL_RESOURCE +labels: dict[str, str] | None = None + +@cached_property +def credentials_and_project(self) -> tuple[Credentials, str]: +credentials, project = get_credentials_and_project_id( +key_path=self.gcp_key_path, scopes=self.scopes, disable_logging=True +) +return credentials, project + +@cached_property +def _client(self) -> gcp_logging.Client: +"""The Cloud Library API client.""" +credentials, project = self.credentials_and_project +return gcp_logging.Client( +credentials=credentials, +project=project, +client_info=CLIENT_INFO, +) + +@cached_property +def _logging_service_client(self) -> LoggingServiceV2Client: +"""The Cloud logging service v2 client.""" +credentials, _ = self.credentials_and_project +return LoggingServiceV2Client( +credentials=credentials, +client_info=CLIENT_INFO, +) + +@cached_property +def transport(self) -> Transport: +"""Object responsible for sending data to Stackdriver.""" +return self.transport_type(self._client, self.gcp_log_name) + +@cached_property +def processors(self) -> tuple[structlog.typing.Processor, ...]: +import structlog.stdlib + +from airflow.sdk.log import relative_path_from_logger + +log_record_factory = getLogRecordFactory() +_transport = self.transport + +def proc( +logger: structlog.typing.WrappedLogger, +method_name: str, +event: structlog.typing.EventDict, +): +if not logger or not relative_path_from_logger(logger): +return event + +name = event.get("logger_name") or event.get("logger", "") +level = structlog.stdlib.NAME_TO_LEVEL.get(method_name.lower(), logging.INFO) +msg = copy.copy(event) +created = None +if ts := msg.pop("timestamp", None): +with contextlib.suppress(Exception): +created = datetime.fromisoformat(ts) +record = log_record_factory( +name, +level, +pathname="", +lineno=0, +msg=msg, +args=(), +exc_info=None, +func=None, +sinfo=None, +) +if created is not None: +ct = created.timestamp() +record.created = ct +record.msecs = int((ct - int(ct)) * 1000) + 0.0 +_transport.send( +record, str(msg.get("event", "")), resource=self.resource, labels=self.labels or {} +) Review Comment: Bug: Here you send only the static labels (without TI-derived ones), but in lines 188-193 you filter by task_id/dag_id/try_number - so reads won't return anything. -- 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]
Re: [PR] Rework StackdriverTaskHandler for the structlog era #65191 [airflow]
shahar1 commented on code in PR #65198:
URL: https://github.com/apache/airflow/pull/65198#discussion_r3223538767
##
providers/google/tests/unit/google/cloud/log/test_stackdriver_task_handler.py:
##
@@ -50,6 +56,231 @@ def clean_stackdriver_handlers():
del handler
+class TestStackdriverRemoteLogIO:
+def setup_method(self):
+self.local_log_location = str(Path(tempfile.gettempdir()) /
"local/stackdriver/logs")
Review Comment:
nit: It is better to use pytest's native `tmp_path`
https://docs.pytest.org/en/stable/how-to/tmp_path.html
##
providers/google/src/airflow/providers/google/cloud/log/stackdriver_task_handler.py:
##
@@ -88,10 +298,10 @@ class StackdriverTaskHandler(logging.Handler):
:param labels: (Optional) Mapping of labels for the entry.
"""
-LABEL_TASK_ID = "task_id"
-LABEL_DAG_ID = "dag_id"
-LABEL_LOGICAL_DATE = "logical_date" if AIRFLOW_V_3_0_PLUS else
"execution_date"
-LABEL_TRY_NUMBER = "try_number"
+LABEL_TASK_ID = LABEL_TASK_ID
+LABEL_DAG_ID = LABEL_DAG_ID
+LABEL_LOGICAL_DATE = LABEL_LOGICAL_DATE
+LABEL_TRY_NUMBER = LABEL_TRY_NUMBER
Review Comment:
```suggestion
# Re-export module-level constants for back-compat with external code
reading them off the class
LABEL_TASK_ID = LABEL_TASK_ID
LABEL_DAG_ID = LABEL_DAG_ID
LABEL_LOGICAL_DATE = LABEL_LOGICAL_DATE
LABEL_TRY_NUMBER = LABEL_TRY_NUMBER
```
otherwise it reads like dead code and someone will try to delete it.
##
providers/google/src/airflow/providers/google/cloud/log/stackdriver_task_handler.py:
##
@@ -245,112 +425,9 @@ def read(
return [((self.task_instance_hostname, messages),)], [new_metadata]
-def _prepare_log_filter(self, ti_labels: dict[str, str]) -> str:
-"""
-Prepare the filter that chooses which log entries to fetch.
-
-More information:
-
https://cloud.google.com/logging/docs/reference/v2/rest/v2/entries/list#body.request_body.FIELDS.filter
-https://cloud.google.com/logging/docs/view/advanced-queries
-
-:param ti_labels: Task Instance's labels that will be used to search
for logs
-:return: logs filter
-"""
-
-def escape_label_key(key: str) -> str:
-return f'"{key}"' if "." in key else key
-
-def escale_label_value(value: str) -> str:
-escaped_value = value.replace("\\", "").replace('"', '\\"')
-return f'"{escaped_value}"'
-
-_, project = self._credentials_and_project
-log_filters = [
-f"resource.type={escale_label_value(self.resource.type)}",
-f'logName="projects/{project}/logs/{self.gcp_log_name}"',
-]
-
-for key, value in self.resource.labels.items():
-
log_filters.append(f"resource.labels.{escape_label_key(key)}={escale_label_value(value)}")
-
-for key, value in ti_labels.items():
-
log_filters.append(f"labels.{escape_label_key(key)}={escale_label_value(value)}")
-return "\n".join(log_filters)
-
-def _read_logs(
-self, log_filter: str, next_page_token: str | None, all_pages: bool
-) -> tuple[str, bool, str | None]:
-"""
-Send requests to the Stackdriver service and downloads logs.
-
-:param log_filter: Filter specifying the logs to be downloaded.
-:param next_page_token: The token of the page from which the log
download will start.
-If None is passed, it will start from the first page.
-:param all_pages: If True is passed, all subpages will be downloaded.
Otherwise, only the first
-page will be downloaded
-:return: A token that contains the following items:
-* string with logs
-* Boolean value describing whether there are more logs,
-* token of the next page
-"""
-messages = []
-new_messages, next_page_token = self._read_single_logs_page(
-log_filter=log_filter,
-page_token=next_page_token,
-)
-messages.append(new_messages)
-if all_pages:
-while next_page_token:
-new_messages, next_page_token = self._read_single_logs_page(
-log_filter=log_filter, page_token=next_page_token
-)
-messages.append(new_messages)
-if not messages:
-break
-
-end_of_log = True
-next_page_token = None
-else:
-end_of_log = not bool(next_page_token)
-return "\n".join(messages), end_of_log, next_page_token
-
-def _read_single_logs_page(self, log_filter: str, page_token: str | None =
None) -> tuple[str, str]:
-"""
-Send requests to the Stackdriver service and downloads single pages
with logs.
-
-:param log_filter: Filter specifying the logs to be downloaded.
-
Re: [PR] Rework StackdriverTaskHandler for the structlog era #65191 [airflow]
haseebmalik18 commented on code in PR #65198: URL: https://github.com/apache/airflow/pull/65198#discussion_r3199119894 ## airflow-core/src/airflow/config_templates/airflow_local_settings.py: ## Review Comment: @phanikumv Created a seperate PR #66513 -- 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]
Re: [PR] Rework StackdriverTaskHandler for the structlog era #65191 [airflow]
haseebmalik18 commented on code in PR #65198: URL: https://github.com/apache/airflow/pull/65198#discussion_r3199119894 ## airflow-core/src/airflow/config_templates/airflow_local_settings.py: ## Review Comment: Created a seperate PR #66513 -- 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]
Re: [PR] Rework StackdriverTaskHandler for the structlog era #65191 [airflow]
amoghrajesh commented on code in PR #65198: URL: https://github.com/apache/airflow/pull/65198#discussion_r3194776807 ## providers/google/src/airflow/providers/google/cloud/log/stackdriver_task_handler.py: ## @@ -56,6 +66,209 @@ ["https://www.googleapis.com/auth/logging.read";, "https://www.googleapis.com/auth/logging.write";] ) +LABEL_TASK_ID = "task_id" +LABEL_DAG_ID = "dag_id" +LABEL_LOGICAL_DATE = "logical_date" if AIRFLOW_V_3_0_PLUS else "execution_date" +LABEL_TRY_NUMBER = "try_number" + + [email protected](kw_only=True) +class StackdriverRemoteLogIO(LoggingMixin): +"""Remote log IO that streams logs to and reads from Google Cloud Stackdriver Logging.""" + +base_log_folder: Path = attrs.field(converter=Path) +delete_local_copy: bool = True + +gcp_key_path: str | None = None +scopes: Collection[str] | None = _DEFAULT_SCOPESS +gcp_log_name: str = DEFAULT_LOGGER_NAME +transport_type: type[Transport] = BackgroundThreadTransport +resource: Resource = _GLOBAL_RESOURCE +labels: dict[str, str] | None = None + +@cached_property +def _credentials_and_project(self) -> tuple[Credentials, str]: +credentials, project = get_credentials_and_project_id( +key_path=self.gcp_key_path, scopes=self.scopes, disable_logging=True +) +return credentials, project + +@cached_property +def _client(self) -> gcp_logging.Client: +"""The Cloud Library API client.""" +credentials, project = self._credentials_and_project +return gcp_logging.Client( +credentials=credentials, +project=project, +client_info=CLIENT_INFO, +) + +@cached_property +def _logging_service_client(self) -> LoggingServiceV2Client: +"""The Cloud logging service v2 client.""" +credentials, _ = self._credentials_and_project +return LoggingServiceV2Client( +credentials=credentials, +client_info=CLIENT_INFO, +) + +@cached_property +def _transport(self) -> Transport: +"""Object responsible for sending data to Stackdriver.""" +return self.transport_type(self._client, self.gcp_log_name) + +@cached_property +def processors(self) -> tuple[structlog.typing.Processor, ...]: +from datetime import datetime +from logging import getLogRecordFactory Review Comment: These can go to top level ## providers/google/src/airflow/providers/google/cloud/log/stackdriver_task_handler.py: ## @@ -375,12 +455,12 @@ def get_external_log_url(self, task_instance: TaskInstance, try_number: int) -> :param try_number: task instance try_number to read logs from :return: URL to the external log collection service """ -_, project_id = self._credentials_and_project +_, project_id = self.io._credentials_and_project -ti_labels = self._task_instance_to_labels(task_instance) -ti_labels[self.LABEL_TRY_NUMBER] = str(try_number) +ti_labels = _task_instance_to_labels(task_instance) +ti_labels[LABEL_TRY_NUMBER] = str(try_number) -log_filter = self._prepare_log_filter(ti_labels) +log_filter = self.io._prepare_log_filter(ti_labels) Review Comment: Same comment as above here ## providers/google/tests/unit/google/cloud/log/test_stackdriver_task_handler.py: ## @@ -17,15 +17,21 @@ from __future__ import annotations import logging +import tempfile Review Comment: Need to add missing test for processors. ## providers/google/src/airflow/providers/google/cloud/log/stackdriver_task_handler.py: ## @@ -225,18 +408,18 @@ def read( if not metadata: metadata = {} -ti_labels = self._task_instance_to_labels(task_instance) +ti_labels = _task_instance_to_labels(task_instance) if try_number is not None: -ti_labels[self.LABEL_TRY_NUMBER] = str(try_number) +ti_labels[LABEL_TRY_NUMBER] = str(try_number) else: -del ti_labels[self.LABEL_TRY_NUMBER] +del ti_labels[LABEL_TRY_NUMBER] -log_filter = self._prepare_log_filter(ti_labels) +log_filter = self.io._prepare_log_filter(ti_labels) next_page_token = metadata.get("next_page_token", None) all_pages = "download_logs" in metadata and metadata["download_logs"] -messages, end_of_log, next_page_token = self._read_logs(log_filter, next_page_token, all_pages) +messages, end_of_log, next_page_token = self.io._read_logs(log_filter, next_page_token, all_pages) Review Comment: This is still reaching into private internals. The `CloudWatchTaskHandler` only calls public methods for example. You can make `_prepare_log_filter` and `_read_logs` ## providers/google/src/airflow/providers/google/cloud/log/stackdriver_task_handler.py: ## @@ -56,6 +66,209 @@
Re: [PR] Rework StackdriverTaskHandler for the structlog era #65191 [airflow]
phanikumv commented on code in PR #65198: URL: https://github.com/apache/airflow/pull/65198#discussion_r3193303679 ## airflow-core/src/airflow/config_templates/airflow_local_settings.py: ## Review Comment: @haseebmalik18 the local_settings split into a separate PR that Elad asked for is still open. -- 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]
Re: [PR] Rework StackdriverTaskHandler for the structlog era #65191 [airflow]
haseebmalik18 commented on code in PR #65198: URL: https://github.com/apache/airflow/pull/65198#discussion_r3192604642 ## providers/google/src/airflow/providers/google/cloud/log/stackdriver_task_handler.py: ## @@ -56,6 +66,209 @@ ["https://www.googleapis.com/auth/logging.read";, "https://www.googleapis.com/auth/logging.write";] ) +LABEL_TASK_ID = "task_id" +LABEL_DAG_ID = "dag_id" +LABEL_LOGICAL_DATE = "logical_date" if AIRFLOW_V_3_0_PLUS else "execution_date" +LABEL_TRY_NUMBER = "try_number" + + [email protected](kw_only=True) +class StackdriverRemoteLogIO(LoggingMixin): +"""Remote log IO that streams logs to and reads from Google Cloud Stackdriver Logging.""" + +base_log_folder: Path = attrs.field(converter=Path) +delete_local_copy: bool = True + +gcp_key_path: str | None = None +scopes: Collection[str] | None = _DEFAULT_SCOPESS +gcp_log_name: str = DEFAULT_LOGGER_NAME +transport_type: type[Transport] = BackgroundThreadTransport +resource: Resource = _GLOBAL_RESOURCE +labels: dict[str, str] | None = None + +@cached_property +def _credentials_and_project(self) -> tuple[Credentials, str]: +credentials, project = get_credentials_and_project_id( +key_path=self.gcp_key_path, scopes=self.scopes, disable_logging=True +) +return credentials, project + +@cached_property +def _client(self) -> gcp_logging.Client: +"""The Cloud Library API client.""" +credentials, project = self._credentials_and_project +return gcp_logging.Client( +credentials=credentials, +project=project, +client_info=CLIENT_INFO, +) + +@cached_property +def _logging_service_client(self) -> LoggingServiceV2Client: +"""The Cloud logging service v2 client.""" +credentials, _ = self._credentials_and_project +return LoggingServiceV2Client( +credentials=credentials, +client_info=CLIENT_INFO, +) + +@cached_property +def _transport(self) -> Transport: +"""Object responsible for sending data to Stackdriver.""" +return self.transport_type(self._client, self.gcp_log_name) + +@cached_property +def processors(self) -> tuple[structlog.typing.Processor, ...]: +from datetime import datetime +from logging import getLogRecordFactory + +import structlog.stdlib Review Comment: `structlog` and `airflow.sdk.log` are lazy here to match the `CloudWatchRemoteLogIO.processors` pattern. `datetime` and `getLogRecordFactory` can move to the top. Want me to move just those two? -- 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]
Re: [PR] Rework StackdriverTaskHandler for the structlog era #65191 [airflow]
shahar1 commented on code in PR #65198: URL: https://github.com/apache/airflow/pull/65198#discussion_r3190073527 ## providers/google/src/airflow/providers/google/cloud/log/stackdriver_task_handler.py: ## @@ -56,6 +66,209 @@ ["https://www.googleapis.com/auth/logging.read";, "https://www.googleapis.com/auth/logging.write";] ) +LABEL_TASK_ID = "task_id" +LABEL_DAG_ID = "dag_id" +LABEL_LOGICAL_DATE = "logical_date" if AIRFLOW_V_3_0_PLUS else "execution_date" +LABEL_TRY_NUMBER = "try_number" + + [email protected](kw_only=True) +class StackdriverRemoteLogIO(LoggingMixin): +"""Remote log IO that streams logs to and reads from Google Cloud Stackdriver Logging.""" + +base_log_folder: Path = attrs.field(converter=Path) +delete_local_copy: bool = True + +gcp_key_path: str | None = None +scopes: Collection[str] | None = _DEFAULT_SCOPESS +gcp_log_name: str = DEFAULT_LOGGER_NAME +transport_type: type[Transport] = BackgroundThreadTransport +resource: Resource = _GLOBAL_RESOURCE +labels: dict[str, str] | None = None + +@cached_property +def _credentials_and_project(self) -> tuple[Credentials, str]: +credentials, project = get_credentials_and_project_id( +key_path=self.gcp_key_path, scopes=self.scopes, disable_logging=True +) +return credentials, project + +@cached_property +def _client(self) -> gcp_logging.Client: +"""The Cloud Library API client.""" +credentials, project = self._credentials_and_project +return gcp_logging.Client( +credentials=credentials, +project=project, +client_info=CLIENT_INFO, +) + +@cached_property +def _logging_service_client(self) -> LoggingServiceV2Client: +"""The Cloud logging service v2 client.""" +credentials, _ = self._credentials_and_project +return LoggingServiceV2Client( +credentials=credentials, +client_info=CLIENT_INFO, +) + +@cached_property +def _transport(self) -> Transport: +"""Object responsible for sending data to Stackdriver.""" +return self.transport_type(self._client, self.gcp_log_name) + +@cached_property +def processors(self) -> tuple[structlog.typing.Processor, ...]: +from datetime import datetime +from logging import getLogRecordFactory + +import structlog.stdlib Review Comment: Can we put these imports at the top? -- 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]
Re: [PR] Rework StackdriverTaskHandler for the structlog era #65191 [airflow]
haseebmalik18 commented on PR #65198: URL: https://github.com/apache/airflow/pull/65198#issuecomment-4339413477 Static checks are good now -- 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]
Re: [PR] Rework StackdriverTaskHandler for the structlog era #65191 [airflow]
haseebmalik18 commented on PR #65198: URL: https://github.com/apache/airflow/pull/65198#issuecomment-4337571443 I don't believe so -- 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]
Re: [PR] Rework StackdriverTaskHandler for the structlog era #65191 [airflow]
leestro commented on PR #65198: URL: https://github.com/apache/airflow/pull/65198#issuecomment-4331718659 Is the failing Static checks job anything to be concerned about? -- 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]
Re: [PR] Rework StackdriverTaskHandler for the structlog era #65191 [airflow]
potiuk commented on PR #65198: URL: https://github.com/apache/airflow/pull/65198#issuecomment-4300311479 Quick follow-up to the triage comment above — one clarification on the "Unresolved review comments" item: Once you believe a thread has been addressed — whether by pushing a fix, or by replying in-thread with an explanation of why the suggestion doesn't apply — **please mark the thread as resolved yourself** by clicking the "Resolve conversation" button at the bottom of each thread. Reviewers don't auto-close their own threads, so an addressed-but-unresolved thread reads as "still waiting on the author" and keeps the PR from moving forward. The author doing the resolve-click is the expected convention on this project. -- 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]
Re: [PR] Rework StackdriverTaskHandler for the structlog era #65191 [airflow]
potiuk commented on PR #65198: URL: https://github.com/apache/airflow/pull/65198#issuecomment-4299430033 @haseebmalik18 This PR has been converted to **draft** because it does not yet meet our [Pull Request quality criteria](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-quality-criteria). **Issues found:** - :x: **Unresolved review comments** (1 thread from a maintainer): please walk through each unresolved review thread. **Even if a suggestion looks incorrect or irrelevant** — and some of them will be, especially any comments left by automated reviewers like GitHub Copilot — it is still the author's responsibility to respond: apply the fix, reply in-thread with a brief explanation of why the suggestion does not apply, or resolve the thread if the feedback is no longer relevant. Leaving threads unaddressed for weeks blocks the PR from moving forward. **What to do next:** - Walk through each unresolved review thread and respond as described above. - Make sure static checks pass locally: `prek run --from-ref main --stage pre-commit`. - Mark the PR as "Ready for review" when you're done. Converting a PR to draft is **not** a rejection — it is an invitation to bring the PR up to the project's standards so that maintainer review time is spent productively. There is no rush — take your time and work at your own pace. We appreciate your contribution and are happy to wait for updates. If you have questions, feel free to ask on the [Airflow Slack](https://s.apache.org/airflow-slack). -- 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]
Re: [PR] Rework StackdriverTaskHandler for the structlog era #65191 [airflow]
jason810496 commented on code in PR #65198: URL: https://github.com/apache/airflow/pull/65198#discussion_r3121211757 ## airflow-core/src/airflow/config_templates/airflow_local_settings.py: ## Review Comment: Yes, Amogh tracked in https://github.com/apache/airflow/issues/65281 -- 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]
Re: [PR] Rework StackdriverTaskHandler for the structlog era #65191 [airflow]
ashb commented on code in PR #65198: URL: https://github.com/apache/airflow/pull/65198#discussion_r3079918568 ## airflow-core/src/airflow/config_templates/airflow_local_settings.py: ## Review Comment: @amoghrajesh This sort of change (needing this in the provider for it to be functional) was exactly the sort of thing I wanted us to look at -- 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]
Re: [PR] Rework StackdriverTaskHandler for the structlog era #65191 [airflow]
eladkal commented on code in PR #65198: URL: https://github.com/apache/airflow/pull/65198#discussion_r3078767851 ## airflow-core/src/airflow/config_templates/airflow_local_settings.py: ## Review Comment: If possible this file should be extracted to a separated PR since core and providers don't have the same release cycle -- 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]
