Change default log filename template to include map_index by ashb · Pull Request #21495 · apache/airflow

ashb · 2022-02-10T13:59:16Z

With the recently added LogTemplate mechanism old TIs will still use the format they had at creation time (with the change here to ensure that we create a LogTemplate row for the just-upgraded-in-place) so the logs can still be viewed in the UI.

Unfortuantely the only way I can find to have map_index only appear for mapped tasks, but also to retain full control of the filename to users was to put a conditional in the template, which I am not happy about.

And since it was now getting quite "deep" I have chosen to "label" the components in the "hive partition style".

~~I'll need to test this change with Elasticsearch too~~ ES working now.

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

ashb · 2022-02-11T17:30:25Z

Example of current layout:

$ tree ~/airflow/logs
/home/ash/airflow/logs
├── dag_id=maptest
│   ├── run_id=backfill__2022-02-10T00:00:00+00:00
│   │   ├── task_id=consumer
│   │   │   ├── map_index=0
│   │   │   │   └── attempt=1.log
│   │   │   ├── map_index=1
│   │   │   │   └── attempt=1.log
│   │   │   └── map_index=2
│   │   │       └── attempt=1.log
│   │   └── task_id=make_list
│   │       └── attempt=1.log
└── scheduler
    ├── 2022-02-10
    └── latest -> /home/ash/airflow/logs/scheduler/2022-02-10

uranusjr

Generally lgtm except two minor issues

uranusjr · 2022-02-14T17:09:22Z

Should these be None?

No, cos of this in BaseOperator init:

self.inlets: List = [] self.outlets: List = []

Or not. I was wrong and these aren't needed at all actually.

uranusjr · 2022-02-14T17:10:17Z

Was setting task.params here unintended or simply unnecessary?

I forget the error, but setting it here was giving some kind of error (I think from it being set twice on the same in-memory object, once in the supervisor, and once in the actual runner.)

So I took the approach that get_* should never have any sideffects!

ashb · 2022-02-15T10:13:00Z

@kaxil @uranusjr Final(?) review please?

With the recently added LogTemplate mechanism old TIs will still use the format they had at creation time (with the change here to ensure that we create a LogTemplate row for the just-upgraded-in-place) so the logs can still be viewed in the UI And since it was now getting quite "deep" I have chosen to "label" the components in the "hive partition style"

The `except AttributeError` was _also_ catching more than just "this handler doesn't have a set_context attribute", but also errors from calling that function which lead to hard-to-track-down errors (missing inlets/outlets) I have also changed `get_template_context` to a side-effect-free function, so it no longer mutates task.params!

And instead of having to duplicate the default config value in to configuration.py for the update process, change it to get the new default value out of the loaded default_airflow.cfg

It has been broken in main for a while (it works fine in 2.2.x series), but because we were catching _all_ AttributeErrors we never noticed.

Since the log_id is never really visible to users I have taken the easier approach of just always including the map_index, even for unmapped tasks.

Co-authored-by: Kaxil Naik <kaxilnaik@gmail.com>

github-actions · 2022-02-15T11:24:08Z

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

A bug was discovered in #21495

ashb added the area:dynamic-task-mapping AIP-42 label Feb 10, 2022

ashb requested review from dstandish and uranusjr February 10, 2022 13:59

ashb commented Feb 10, 2022

View reviewed changes

Comment thread airflow/config_templates/config.yml Outdated

ashb requested review from XD-DENG and kaxil as code owners February 10, 2022 16:52

ashb force-pushed the mapped-task-logs branch from f87d6f6 to ea10d94 Compare February 11, 2022 21:11

ashb changed the title ~~🚧 Change default log filename template to include map_index~~ Change default log filename template to include map_index Feb 11, 2022

ashb force-pushed the mapped-task-logs branch 2 times, most recently from 6dddeda to 3028972 Compare February 14, 2022 15:03

uranusjr reviewed Feb 14, 2022

View reviewed changes

Comment thread airflow/config_templates/config.yml Outdated

uranusjr reviewed Feb 14, 2022

View reviewed changes

Comment thread airflow/models/mappedoperator.py Outdated

uranusjr reviewed Feb 14, 2022

View reviewed changes

ashb force-pushed the mapped-task-logs branch from 3028972 to c71a661 Compare February 14, 2022 16:58

ashb requested a review from potiuk as a code owner February 14, 2022 16:58

uranusjr mentioned this pull request Feb 14, 2022

Type TaskInstance.task to Operator and call unmap() when needed #21563

Merged

uranusjr reviewed Feb 14, 2022

View reviewed changes

kaxil reviewed Feb 14, 2022

View reviewed changes

Comment thread airflow/sensors/smart_sensor.py Outdated

kaxil reviewed Feb 14, 2022

View reviewed changes

Comment thread docs/apache-airflow/logging-monitoring/logging-tasks.rst Outdated

kaxil reviewed Feb 14, 2022

View reviewed changes

Comment thread docs/apache-airflow/logging-monitoring/logging-tasks.rst Outdated

ashb added 7 commits February 15, 2022 11:19

Change default template to put run_id before task_id

51084a7

And instead of having to duplicate the default config value in to configuration.py for the update process, change it to get the new default value out of the loaded default_airflow.cfg

Use dag_id not dag for consistency with other parameters

0af97a1

Fix smart sensor logging

a1561d8

It has been broken in main for a while (it works fine in 2.2.x series), but because we were catching _all_ AttributeErrors we never noticed.

Fix tests

355f23b

Code review changes

8153a5b

ashb and others added 5 commits February 15, 2022 11:19

Update Elasticsearch logging config to include map_index

058de30

Since the log_id is never really visible to users I have taken the easier approach of just always including the map_index, even for unmapped tasks.

Apply suggestions from code review

26aaaaa

Co-authored-by: Kaxil Naik <kaxilnaik@gmail.com>

fixup! Code review changes

905e563

fixup! Fix tests

7e17ef0

Revert unnecessary changes to smart_sensor

6ae4f31

ashb force-pushed the mapped-task-logs branch from 344bed9 to 6ae4f31 Compare February 15, 2022 11:20

kaxil approved these changes Feb 15, 2022

View reviewed changes

github-actions Bot added the full tests needed We need to run full set of tests for this PR to merge label Feb 15, 2022

ashb merged commit 2258e13 into apache:main Feb 15, 2022

ashb deleted the mapped-task-logs branch February 15, 2022 12:06

uranusjr mentioned this pull request Feb 22, 2022

Deprecate template variables ts etc. and change the default log_filename_template #19625

Closed

1 task

jedcunningham added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Feb 28, 2022

potiuk mentioned this pull request Mar 7, 2022

Status of testing Providers that were prepared on March 07, 2022 #22063

Closed

84 tasks

potiuk mentioned this pull request Mar 31, 2022

'dict' object has no attribute 'dump' #22638

Closed

2 tasks

potiuk added a commit that referenced this pull request Apr 7, 2022

Update constraints for elasticsearch provider after a bug

677c24e

A bug was discovered in #21495

jedcunningham added this to the Airflow 2.3.0 milestone Apr 26, 2022

potiuk added a commit that referenced this pull request Jun 17, 2022

Update constraints for elasticsearch provider after a bug

00898fb

A bug was discovered in #21495

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Change default log filename template to include map_index#21495

Change default log filename template to include map_index#21495
ashb merged 12 commits into
apache:mainfrom
astronomer:mapped-task-logs

ashb commented Feb 10, 2022 •

edited

Loading

Uh oh!

Uh oh!

ashb commented Feb 11, 2022

Uh oh!

Uh oh!

Uh oh!

uranusjr left a comment

Uh oh!

uranusjr Feb 14, 2022

Uh oh!

ashb Feb 14, 2022

Uh oh!

ashb Feb 14, 2022

Uh oh!

uranusjr Feb 14, 2022

Uh oh!

ashb Feb 14, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ashb commented Feb 15, 2022

Uh oh!

github-actions Bot commented Feb 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

ashb commented Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ashb commented Feb 11, 2022

Uh oh!

Uh oh!

Uh oh!

uranusjr left a comment

Choose a reason for hiding this comment

Uh oh!

uranusjr Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

ashb Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

ashb Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

uranusjr Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

ashb Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ashb commented Feb 15, 2022

Uh oh!

github-actions Bot commented Feb 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ashb commented Feb 10, 2022 •

edited

Loading