You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/11/08 21:24:19 UTC
[GitHub] [airflow] hankehly opened a new pull request, #27564: CloudWatch task handler doesn't fall back to local logs when Amazon CloudWatch logs aren't found
hankehly opened a new pull request, #27564:
URL: https://github.com/apache/airflow/pull/27564
Closes #27396
### Summary
The CloudWatch task log handler does not default to local logs (like the documentation says it should) when remote logs are unavailable. This PR fixes the issue.
### Todo
- [x] Update CloudWatch log handler
- [x] Add unit tests
- [ ] Manual check (run a test DAG without [AWS CloudWatch logging configured](https://airflow.apache.org/docs/apache-airflow-providers-amazon/stable/logging/cloud-watch-task-handlers.html), then configure AWS CloudWatch remote logging and re-run a test DAG)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hankehly commented on a diff in pull request #27564: CloudWatch task handler doesn't fall back to local logs when Amazon CloudWatch logs aren't found
Posted by GitBox <gi...@apache.org>.
hankehly commented on code in PR #27564:
URL: https://github.com/apache/airflow/pull/27564#discussion_r1018478255
##########
airflow/providers/amazon/aws/log/cloudwatch_task_handler.py:
##########
@@ -97,17 +107,12 @@ def get_cloudwatch_logs(self, stream_name: str) -> str:
:param stream_name: name of the Cloudwatch log stream to get all logs from
:return: string of all logs from the given log stream
"""
- try:
- events = self.hook.get_log_events(
- log_group=self.log_group,
- log_stream_name=stream_name,
- start_from_head=True,
- )
- return "\n".join(self._event_to_str(event) for event in events)
- except Exception:
Review Comment:
I am propagating the AWS error message to the task log. I think this will be useful for debugging IAM/connection issues.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hankehly commented on a diff in pull request #27564: CloudWatch task handler doesn't fall back to local logs when Amazon CloudWatch logs aren't found
Posted by GitBox <gi...@apache.org>.
hankehly commented on code in PR #27564:
URL: https://github.com/apache/airflow/pull/27564#discussion_r1018477719
##########
tests/providers/amazon/aws/log/test_cloudwatch_task_handler.py:
##########
@@ -154,49 +154,22 @@ def test_read(self):
[{"end_of_log": True}],
)
- def test_read_wrong_log_stream(self):
Review Comment:
I deleted these 2 test cases for the following reasons.
* They overlap with the one I added (testing for generic failure to fetch remote logs)
* AWS error messages are now propagated to the task log. These strings could change in the future and break our unit tests.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hankehly commented on pull request #27564: CloudWatch task handler doesn't fall back to local logs when Amazon CloudWatch logs aren't found
Posted by GitBox <gi...@apache.org>.
hankehly commented on PR #27564:
URL: https://github.com/apache/airflow/pull/27564#issuecomment-1309399807
@vincbeck
Thanks for the early review. Once I've handled the failing tests I'll click the "Ready for review" button.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] o-nikolas commented on pull request #27564: CloudWatch task handler doesn't fall back to local logs when Amazon CloudWatch logs aren't found
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on PR #27564:
URL: https://github.com/apache/airflow/pull/27564#issuecomment-1311111630
:rocket:
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hankehly commented on a diff in pull request #27564: CloudWatch task handler doesn't fall back to local logs when Amazon CloudWatch logs aren't found
Posted by GitBox <gi...@apache.org>.
hankehly commented on code in PR #27564:
URL: https://github.com/apache/airflow/pull/27564#discussion_r1018482000
##########
tests/providers/amazon/aws/log/test_cloudwatch_task_handler.py:
##########
@@ -154,49 +154,22 @@ def test_read(self):
[{"end_of_log": True}],
)
- def test_read_wrong_log_stream(self):
- generate_log_events(
- self.conn,
- self.remote_log_group,
- "alternate_log_stream",
- [
- {"timestamp": 10000, "message": "First"},
- {"timestamp": 20000, "message": "Second"},
- {"timestamp": 30000, "message": "Third"},
- ],
- )
-
- msg_template = "*** Reading remote log from Cloudwatch log_group: {} log_stream: {}.\n{}\n"
- error_msg = (
- "Could not read remote logs from log_group: "
- f"{self.remote_log_group} log_stream: {self.remote_log_stream}."
- )
- assert self.cloudwatch_task_handler.read(self.ti) == (
- [[("", msg_template.format(self.remote_log_group, self.remote_log_stream, error_msg))]],
- [{"end_of_log": True}],
- )
-
- def test_read_wrong_log_group(self):
- generate_log_events(
- self.conn,
- "alternate_log_group",
- self.remote_log_stream,
- [
- {"timestamp": 10000, "message": "First"},
- {"timestamp": 20000, "message": "Second"},
- {"timestamp": 30000, "message": "Third"},
- ],
- )
-
- msg_template = "*** Reading remote log from Cloudwatch log_group: {} log_stream: {}.\n{}\n"
- error_msg = (
- f"Could not read remote logs from log_group: "
- f"{self.remote_log_group} log_stream: {self.remote_log_stream}."
- )
- assert self.cloudwatch_task_handler.read(self.ti) == (
- [[("", msg_template.format(self.remote_log_group, self.remote_log_stream, error_msg))]],
- [{"end_of_log": True}],
+ def test_should_read_from_local_on_failure_to_fetch_remote_logs(self):
+ """Check that local logs are displayed on failure to fetch remote logs"""
+ self.cloudwatch_task_handler.set_context(self.ti)
+ with mock.patch.object(self.cloudwatch_task_handler, "get_cloudwatch_logs") as mock_get_logs:
+ mock_get_logs.side_effect = Exception("Failed to connect")
+ log, metadata = self.cloudwatch_task_handler._read(self.ti, self.ti.try_number)
+ expected_log = (
+ f"*** Unable to read remote logs from Cloudwatch (log_group: {self.remote_log_group}, "
+ f"log_stream: {self.remote_log_stream})\n*** Failed to connect\n\n"
+ # The value of "log_pos" is equal to the length of this next line
+ f"*** Reading local file: {self.local_log_location}/{self.remote_log_stream}\n"
)
+ assert log == expected_log
+ expected_log_pos = 26 + len(self.local_log_location) + len(self.remote_log_stream)
Review Comment:
Log position seems irrelevant in the context of the CloudWatch handler because we fetch/return all logs in one request. I'm following the example I found in the GCS task handler here.
https://github.com/apache/airflow/blob/21063267fd9764b2ca38669e8faec75d9b87179c/tests/providers/google/cloud/log/test_gcs_task_handler.py#L108
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #27564: CloudWatch task handler doesn't fall back to local logs when Amazon CloudWatch logs aren't found
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #27564:
URL: https://github.com/apache/airflow/pull/27564
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org