Upgrade watchtower to 3.0.1 (#25019) #34747

cBiscuitSurprise · 2023-10-04T05:33:51Z

This PR upgrades watchtower from 2.0.1 to 3.0.1. A new config item is introduced to allow customer to opt-in to the "new" serialization format.

Watchtower functionality

Watchtower introduced a change whereby they use repr for any non-serializable objects in place of what was just null.

Source

{"datetime": datetime(2023,1,1), "customObject": SomeCustomObject(1, 2, 3)}

Was

{"datetime": "2023-01-01T00:00:00+00:00", "customObject": null}

Now

{"datetime": "2023-01-01T00:00:00+00:00", "customObject": "SomeCustomSerializationProvidedByRepr(...)"}

Airflow functionality

The default behavior for airflow will be to maintain the null serialization, but allow the option to use the new style (or provide your own).

The new config is logging.json_serializer which is an import path (string). The import should be a callable taking an object and returning a string.

closes: #25019

@ferruzzi

boring-cyborg · 2023-10-04T05:33:55Z

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.
Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: [email protected]
Slack: https://s.apache.org/airflow-slack

uranusjr · 2023-10-04T05:59:57Z

The config option should be prefixed with cloudwatch_task_handler since it only applies to it.

eladkal · 2023-10-04T07:54:30Z

airflow/config_templates/config.yml

+    json_serializer:
+      description: |
+        By default, for non-string logged messages all non-json-parsable objects are logged as `null` except
+        `datetime` objects which are ISO formatted. Users can optionally provide their own JSON serializer or
+        opt to use a `repr` serializer which calls `repr(object)` for any non-JSON-serializable objects in the
+        logged message. The `airflow.providers.amazon.aws.log.cloudwatch_task_handler.json_serialize` uses
+        `repr` while `airflow.providers.amazon.aws.log.cloudwatch_task_handler.json_serialize_legacy` uses
+        `null`. If a custom serializer is provide, it must adhear to `Callable[[Any], str]`
+        (`def my_serializer(o: Any) -> str`). Be aware, that if opting in to using the `repr` serializer, you
+        should take extra care that no new, sensitive, data is logged (e.g. credentials). If creating your own
+        json-serializer take special care to fail gracefully, without throwing.
+      type: string
+      version_added: 2.7.1
+      example: airflow.providers.amazon.aws.log.cloudwatch_task_handler.json_serialize
+      default: airflow.providers.amazon.aws.log.cloudwatch_task_handler.json_serialize_legacy


Should this be core configuration or provider configuration?
we now have the option to define configurations as part of the amazon provider

+1, it should be part of Amazon provider package config to me

If it is only being used by the cloudwatch logger then yeah, Provider config sounds right.

I think it would also a good idea move all Amazon specific loggers configs from core to Provider (as separate PR), I'm just not sure is we have any mechanism to deprecate config in core and move in provider.

I've moved this new config to the aws provider.

airflow/config_templates/config.yml

vincbeck · 2023-10-04T14:15:11Z

airflow/config_templates/config.yml

+    json_serializer:
+      description: |
+        By default, for non-string logged messages all non-json-parsable objects are logged as `null` except
+        `datetime` objects which are ISO formatted. Users can optionally provide their own JSON serializer or
+        opt to use a `repr` serializer which calls `repr(object)` for any non-JSON-serializable objects in the
+        logged message. The `airflow.providers.amazon.aws.log.cloudwatch_task_handler.json_serialize` uses
+        `repr` while `airflow.providers.amazon.aws.log.cloudwatch_task_handler.json_serialize_legacy` uses
+        `null`. If a custom serializer is provide, it must adhear to `Callable[[Any], str]`
+        (`def my_serializer(o: Any) -> str`). Be aware, that if opting in to using the `repr` serializer, you
+        should take extra care that no new, sensitive, data is logged (e.g. credentials). If creating your own
+        json-serializer take special care to fail gracefully, without throwing.
+      type: string
+      version_added: 2.7.1
+      example: airflow.providers.amazon.aws.log.cloudwatch_task_handler.json_serialize
+      default: airflow.providers.amazon.aws.log.cloudwatch_task_handler.json_serialize_legacy


+1, it should be part of Amazon provider package config to me

airflow/config_templates/config.yml

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

cBiscuitSurprise · 2023-10-04T19:39:45Z

Thank you everyone for your input. I've addressed your comments.

P.S.
Would someone be able to add the hacktoberfest-accepted label to this PR?

ferruzzi · 2023-10-04T20:37:39Z

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

@@ -33,22 +33,33 @@
    from airflow.models import TaskInstance




@bolkedebruin - Does the following method step on your serializing PR any?

I do not think so, but I am not familiar with this code.

eladkal · 2023-10-04T20:39:36Z

qouting @o-nikolas comment from the issue

But note: This change will need a careful check of Airflow logs, since v3 of Watcher can now include much more information in the logs than previous

@vincbeck @ferruzzi just to confirm that your review take it into account

vincbeck · 2023-10-04T20:43:50Z

cBiscuitSurprise

Good point! @cBiscuitSurprise Could you check that and maybe some sample of logs before and after the change? From the code it is impossible to say

cBiscuitSurprise · 2023-10-04T20:50:43Z

@vincbeck @eladkal

The breaking change is that in watchtower 3.0.1 json-serialized log messages now get serialized with repr instead of just returning None (if you log an object as opposed to a string). Depending on the implementation of an object's repr this could include more information than previously logged. The airflow tests did not have any instances of logging an object.

We're preserving the "legacy" behavior by default, so no new information will be logged. Customers can "opt-in" to the new behavior by setting the configuration cloudwatch_task_handler_json_serializer to "airflow.providers.amazon.aws.log.cloudwatch_task_handler.json_serialize" (as opposed to the default value of "airflow.providers.amazon.aws.log.cloudwatch_task_handler.json_serialize_legacy").

I've added tests to illustrate the difference (the first case is the default, legacy, behavior and the second case is the opt-in new watchtower behavior):

airflow/tests/providers/amazon/aws/log/test_cloudwatch_task_handler.py

Lines 179 to 183 in d03759e

    
                       (None, '{"datetime": "2023-01-01T00:00:00+00:00", "customObject": null}'), 
        
                       ( 
        
                           "airflow.providers.amazon.aws.log.cloudwatch_task_handler.json_serialize", 
        
                           '{"datetime": "2023-01-01T00:00:00+00:00", "customObject": "SomeCustomSerialization(...)"}', 
        
                       ),

ferruzzi

Approved pending confirmation from @bolkedebruin that this isn't going to conflict with his changes in #34683

ferruzzi · 2023-10-05T18:28:06Z

Failed CI test is helm failing in the KubernetesPodOperator unit test... I can't imagine how this PR would trigger that so I've re-run it, it may just be a bit of unrelated flake.

vincbeck · 2023-10-05T18:55:02Z

Failed CI test is helm failing in the KubernetesPodOperator unit test... I can't imagine how this PR would trigger that so I've re-run it, it may just be a bit of unrelated flake.

Yes this test has been failing intermittently a lot these last days

boring-cyborg · 2023-10-06T14:35:12Z

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

eladkal · 2023-10-06T18:43:57Z

P.S.
Would someone be able to add the hacktoberfest-accepted label to this PR?

we are not participating in hacktoberfest this year but if this helps you I'm happy to accommodate the request.
let me know if still relevant.

o-nikolas · 2023-10-11T18:04:00Z

Nice work @cBiscuitSurprise, I'm glad to see this one finally closed out. Smart idea to make the new behaviour configurable and off by default, it limits the blast radius of doing this upgrade! Thanks for the contribution.

cBiscuitSurprise requested review from eladkal and o-nikolas as code owners October 4, 2023 05:33

boring-cyborg bot added area:logging area:providers kind:documentation provider:amazon AWS/Amazon - related issues labels Oct 4, 2023

eladkal reviewed Oct 4, 2023

View reviewed changes

eladkal requested review from ferruzzi and vincbeck October 4, 2023 09:01

vincbeck reviewed Oct 4, 2023

View reviewed changes

Upgrade watchtower to 3.0.1 (apache#25019)

d07c74e

cBiscuitSurprise force-pushed the main branch from 40ddbd7 to d07c74e Compare October 4, 2023 16:09