Add Redis task handler #31855

michalc · 2023-06-12T14:17:09Z

This stores log lines in Redis up to a configured maximum log lines, always keeping the most recent, up to a configured TTL.

This deviates from other existing task handlers in that it accepts a connection ID. This allows it to be used in addition to other handlers, and so allows a graceful/reversible transition from one logging system to another.

This is particularly useful in situations that use Redis as a message broker, where additional infrastructure isn't desired.

closes: #31834

boring-cyborg · 2023-06-12T14:17:14Z

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

Pay attention to the quality of your code (ruff, mypy and type annotations). Our pre-commits will help you with that.
In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
Be sure to read the Airflow Coding style.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack

michalc · 2023-06-12T20:09:20Z

@hussein-awala This is a PR for #31834, and is ready for a first pass at a review I hope.

(I'm guessing at a good number of things...)

utkarsharma2 · 2023-06-13T01:42:06Z

airflow/providers/redis/log/redis_task_handler.py

+        If omitted, this is the equivalent of 28 days.
+    :param conn_id:
+        Airflow connection ID for the Redis hook to use
+        If omitted or None, conf.get("logging", "REMOTE_LOG_CONN_ID") is used.


conf.get("logging", "REMOTE_LOG_CONN_ID") is this supposed to be here?
If you intend to specify that airflow's logging config will be used, would be better to mention that explicitly. This probably won't make sense to users, unless they are aware of internals.

Have changed it to logging.remote_log_conn_id, which is how the documentation refers to the setting at https://airflow.apache.org/docs/apache-airflow/stable/configurations-ref.html#logging

utkarsharma2 · 2023-06-13T01:47:41Z

airflow/providers/redis/log/redis_task_handler.py

+        log_str = b"\n".join(self.conn.lrange(self._render_filename(ti, try_number), start=0, end=-1)).decode(
+            "utf-8"
+        )


Suggested change

log_str = b"\n".join(self.conn.lrange(self._render_filename(ti, try_number), start=0, end=-1)).decode(

"utf-8"

)

log_str = b"\n".join(self.conn.lrange(self._render_filename(ti, try_number), start=0, end=-1)).decode()

default encoding format is utf-8 ref - https://docs.python.org/3/library/stdtypes.html#bytes.decode

Now removed

utkarsharma2 · 2023-06-13T01:50:22Z

airflow/utils/log/file_task_handler.py

@@ -147,7 +147,7 @@ class FileTaskHandler(logging.Handler):

    def __init__(self, base_log_folder: str, filename_template: str | None = None):
        super().__init__()
-        self.handler: logging.FileHandler | None = None
+        self.handler: logging.Handler | None = None


This change look out of place?

This is true - but it allows the lower level handler, _RedisHandler to not inherit from logging.FileHandler, which I think is appropriate since there is no file involved(?)

It looks like the codebase already doesn't enforce that the lower level handler extends from logging.FileHandler -

airflow/airflow/providers/amazon/aws/log/cloudwatch_task_handler.py

Line 67 in 2940b9f

self.handler = watchtower.CloudWatchLogHandler(

which is defined in https://github.com/kislyuk/watchtower/blob/698ff2241befb6586269b6bbce0e305032c2d45c/watchtower/__init__.py#L139

This touches on the bits that I'm unsure about - it looks like FileTaskHandler has a lot of file-related things, but also it looks like it's expected to extend from it when not logging to a file. Not quite sure how to tackle that

Should have been more specific initially. :)

What I meant was that maybe this change is not required by this PR? since you have anyway overridden the self.handler in class RedisTaskHandler. Also, FileTaskHandler is used in many places, and changing it here might affect those implementations.

About inheriting from FileTaskHandler I'm not sure either, maybe @bolkedebruin might have more context.

What I meant was that maybe this change is not required by this PR? since you have anyway overridden the self.handler in class RedisTaskHandler

Ah - so if I change it back to how it was before this PR, I get a type error:

airflow/providers/redis/log/redis_task_handler.py:61: error: Incompatible types in assignment (expression has type "Optional[_RedisHandler]", base class "FileTaskHandler" defined the type as "Optional[FileHandler]") [assignment]

So something else would have to change too. The easiest way that I've come up with is to de-facto disable type checking on self.handler by removing type annotations in a few places, to make it more like the Cloudwatch handler, but that seems a bit... cheat-y.

_RedisHandler could really inherit from logging.FileHandler to satisfy type checking, but its constructor seems to require a path to a file, which seems odd in this case.

This change is OK to me since the base class only really needs to inner handler to be a logging.Handler. It could be moved to a separate PR but since this is where the change becomes necessary (instead of just a generally good thing to do) it’s fine to be included here.

If this specific change is not tightly coupled to this PR I would suggest to do it in a separated PR.
this is because utils/log goes to core release cycle while the other files goes to provider release cycle.
@michalc Is it possible to separate?

@michalc can you check my above comment?

@eladkal Sounds reasonable to separate to another PR. Will do that now

@eladkal Done in #32773

michalc · 2023-07-22T17:07:23Z

@michalc can you rebase and resolve the error?

@eladkal Now done (hopefully...)

eladkal · 2023-07-22T17:14:58Z

@eladkal Now done (hopefully...)

Cool. Running CI... lets wait to make sure its green

FileTaskHandler is the base class for logging handlers, including those that don't log to files via delegating to logging.FileHandler, e.g. in the CloudwatchTaskHandler at https://github.com/apache/airflow/blob/2940b9fa55a6a72c60c2162e541631addec3d6b8/airflow/providers/amazon/aws/log/cloudwatch_task_handler.py#L67 It is suspected that type checking is not enabled in this part of the CloudwatchTaskHandler otherwise it would have already been failing. This change adjusts the base class so if type checking is enabled in the task handler, if it delegates to a logging.Handler that is not a logging.FileHandler as the CloudWatchHandler, then the type checking should pass. This was originally part of apache#31855 and split out. related: apache#31834

This stores log lines in Redis up to a configured maximum log lines, always keeping the most recent, up to a configured TTL. This deviates from other existing task handlers in that it accepts a connection ID. This allows it to be used in addition to other handlers, and so allows a graceful/reversible transition from one logging system to another. This is particularly useful in situations that use Redis as a message broker, where additional infrastructure isn't desired. closes: apache#31834

michalc · 2023-07-22T18:49:04Z

@eladkal I saw a failing test that I didn't think is related to changes here, so have rebased on main (and rebased #32773)

boring-cyborg · 2023-07-23T06:43:38Z

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

FileTaskHandler is the base class for logging handlers, including those that don't log to files via delegating to logging.FileHandler, e.g. in the CloudwatchTaskHandler at https://github.com/apache/airflow/blob/2940b9fa55a6a72c60c2162e541631addec3d6b8/airflow/providers/amazon/aws/log/cloudwatch_task_handler.py#L67 It is suspected that type checking is not enabled in this part of the CloudwatchTaskHandler otherwise it would have already been failing. This change adjusts the base class so if type checking is enabled in the task handler, if it delegates to a logging.Handler that is not a logging.FileHandler as the CloudWatchHandler, then the type checking should pass. This was originally part of apache#31855 and split out. related: apache#31834

* Allow FileTaskHandler to delegate to instances of logging.Handler FileTaskHandler is the base class for logging handlers, including those that don't log to files via delegating to logging.FileHandler, e.g. in the CloudwatchTaskHandler at https://github.com/apache/airflow/blob/2940b9fa55a6a72c60c2162e541631addec3d6b8/airflow/providers/amazon/aws/log/cloudwatch_task_handler.py#L67 It is suspected that type checking is not enabled in this part of the CloudwatchTaskHandler otherwise it would have already been failing. This change adjusts the base class so if type checking is enabled in the task handler, if it delegates to a logging.Handler that is not a logging.FileHandler as the CloudWatchHandler, then the type checking should pass. This was originally part of apache/airflow#31855 and split out. related: apache/airflow#31834 * Add Redis task handler This stores log lines in Redis up to a configured maximum log lines, always keeping the most recent, up to a configured TTL. This deviates from other existing task handlers in that it accepts a connection ID. This allows it to be used in addition to other handlers, and so allows a graceful/reversible transition from one logging system to another. This is particularly useful in situations that use Redis as a message broker, where additional infrastructure isn't desired. closes: apache/airflow#31834 GitOrigin-RevId: 42b4b43c4c2ccf0b6e7eaa105c982df495768d01

boring-cyborg bot added area:logging area:providers provider:redis labels Jun 12, 2023

michalc force-pushed the redis-task-handler branch 14 times, most recently from 994a902 to 9e5e40f Compare June 12, 2023 17:47

michalc marked this pull request as ready for review June 12, 2023 18:09

michalc force-pushed the redis-task-handler branch 3 times, most recently from d289b21 to b675ebc Compare June 12, 2023 20:00

michalc force-pushed the redis-task-handler branch from b675ebc to f110f0c Compare June 12, 2023 20:11

hussein-awala self-requested a review June 12, 2023 21:58

utkarsharma2 reviewed Jun 13, 2023

View reviewed changes

michalc force-pushed the redis-task-handler branch 2 times, most recently from d069378 to 7707b2c Compare June 13, 2023 05:23

michalc force-pushed the redis-task-handler branch 3 times, most recently from a281203 to 807a638 Compare July 22, 2023 15:38

michalc mentioned this pull request Jul 22, 2023

Allow FileTaskHandler to delegate to instances of logging.Handler #32773

Closed

michalc force-pushed the redis-task-handler branch 3 times, most recently from 2e43225 to 21f5a7d Compare July 22, 2023 17:07

michalc added 2 commits July 22, 2023 19:46

michalc force-pushed the redis-task-handler branch from 21f5a7d to 9ca7921 Compare July 22, 2023 18:46

eladkal approved these changes Jul 23, 2023

View reviewed changes

eladkal merged commit 42b4b43 into apache:main Jul 23, 2023
42 checks passed

michalc deleted the redis-task-handler branch July 23, 2023 06:50

eladkal added this to the Airflow 2.6.4 milestone Jul 23, 2023

eladkal added the type:misc/internal Changelog: Misc changes that should appear in change log label Jul 23, 2023

eladkal mentioned this pull request Jul 29, 2023

Status of testing Providers that were prepared on July 29, 2023 #32936

Closed

69 tasks

eladkal modified the milestones: Airflow 2.6.4, Airflow 2.7.0 Aug 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Redis task handler #31855

Add Redis task handler #31855

michalc commented Jun 12, 2023 •

edited

Loading

boring-cyborg bot commented Jun 12, 2023

michalc commented Jun 12, 2023

utkarsharma2 Jun 13, 2023

michalc Jun 13, 2023

utkarsharma2 Jun 13, 2023

michalc Jun 13, 2023

utkarsharma2 Jun 13, 2023

michalc Jun 13, 2023

utkarsharma2 Jun 14, 2023

michalc Jun 14, 2023 •

edited

Loading

uranusjr Jun 14, 2023

eladkal Jun 27, 2023 •

edited

Loading

eladkal Jul 22, 2023

michalc Jul 22, 2023

michalc Jul 22, 2023

michalc commented Jul 22, 2023

eladkal commented Jul 22, 2023

michalc commented Jul 22, 2023 •

edited

Loading

boring-cyborg bot commented Jul 23, 2023

Add Redis task handler #31855

Add Redis task handler #31855

Conversation

michalc commented Jun 12, 2023 • edited Loading

boring-cyborg bot commented Jun 12, 2023

michalc commented Jun 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalc Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladkal Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalc commented Jul 22, 2023

eladkal commented Jul 22, 2023

michalc commented Jul 22, 2023 • edited Loading

boring-cyborg bot commented Jul 23, 2023

michalc commented Jun 12, 2023 •

edited

Loading

michalc Jun 14, 2023 •

edited

Loading

eladkal Jun 27, 2023 •

edited

Loading

michalc commented Jul 22, 2023 •

edited

Loading