Skip to content

CloudwatchTaskHandler params to create log group/stream #19022

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
anaynayak wants to merge 4 commits into from
Closed

CloudwatchTaskHandler params to create log group/stream #19022

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6cdb3ea
CloudwatchTaskHandler params to create log group/stream
anaynayak Oct 16, 2021
5575c66
Provide config option to override cloudwatch log group/stream creation
anaynayak Oct 27, 2021
15760e2
Add cloudwatch log group/stream creation to config templates
anaynayak Oct 27, 2021
3d59634
Use conf.getboolean for fetching log group/stream values
anaynayak Oct 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion airflow/config_templates/airflow_local_settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
# settings.py and cli.py. Please see AIRFLOW-1455.
LOG_LEVEL: str = conf.get('logging', 'LOGGING_LEVEL').upper()


# Flask appbuilder's info level log is very verbose,
# so it's set to 'WARN' by default.
FAB_LOG_LEVEL: str = conf.get('logging', 'FAB_LOGGING_LEVEL').upper()
Expand Down Expand Up @@ -194,13 +193,17 @@

DEFAULT_LOGGING_CONFIG['handlers'].update(S3_REMOTE_HANDLERS)
elif REMOTE_BASE_LOG_FOLDER.startswith('cloudwatch://'):
create_log_group = conf.getboolean('logging', 'CREATE_CLOUDWATCH_LOG_GROUP', fallback=True)
create_log_stream = conf.getboolean('logging', 'CREATE_CLOUDWATCH_LOG_STREAM', fallback=True)

@o-nikolas o-nikolas Oct 28, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should allow this setting to be configured. Setting stream creation to anything but True will catastrophically break cloudwatch logging.
In cloudwatch Log Groups are like folders and streams are like files. It is possible to pre-create the log group "folder" but the stream log "files" are named on the fly after the task instances themselves (just like the default Airflow local filesystem logging). There's no (reasonable) way you could pre-create those stream names and thus watchtower, if not allowed to create the streams itself, will fail to log any task logs.

@o-nikolas o-nikolas Oct 28, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the log group name is already configurable through the arn. And it should be harmless for watchtower to attempt to re-create it (it correctly handles the resource already existing exception). Your original issue was an authentication issue, not an idempotentcy issue.

The only reason I can think of that you'd ever want to restrict log group creation is if the aws role/user you are using does not, and can not, have that policy permission allowed. However the role still needs stream creation permissions (see above) so I don't see why one would ever restrict log group creation but not stream creation. It's a very rare case that I don't think is worth the extra complexity to handle IMHO.

@anaynayak anaynayak Oct 30, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the original issue, I assumed that the issue was around a restrictive role since the log group was already created via Terraform. Based on what you've said, The bits around log stream definitely make sense. I'll remove that as an option.

If the option to not create the log group isn't required, I can close the PR and we can create a new one when required.

@o-nikolas o-nikolas Nov 1, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, great discussion :)

@nik-davis nik-davis Jan 13, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies if this is the wrong place, but here feels appropriate - feel free to let me know otherwise.

I'd like to make the case for being able to disable creation of the log group (but not the log stream).

We recently noticed a lot of errors (multiple times a minute in some cases) causing our tasks to fail to start, along the lines of "Failed to execute task An error occurred (ThrottlingException) when calling the CreateLogGroup operation (reached max retries: 4): Rate exceeded." which led us to find the AWS service quota for "CreateLogGroup throttle limit in transactions per second" which is 5 per second by default, and we were frequently in breach of that (at peak times we run a lot of tasks!).

If my understanding is correct, every time a task runs, watchtower will make a CreateLogGroup request and pass on a ResourceAlreadyExistsException if it exists. If the log group never changes, this part shouldn't be necessary - though as @o-nikolas mentioned, it needs to be able to create a unique log stream for each task. FYI the default AWS quota for that is 50 per second (called "CreateLogStream throttle limit in transactions per second"), which allows for a lot more headroom.

It would be nice to have a way to prevent these thousands of requests to create a log group that already exists, or even better, perhaps it could attempt to create the log group on startup, then each task wouldn't need to make the request.

@o-nikolas o-nikolas Jan 13, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nik-davis
Hey Nik, This is a good callout and it's something we ran into while developing MWAA (which leverages Cloudawatch for all log types and thus creates a LOT of log groups and log stream requests). The correct solution here is to actually not call create log group until you're sure it doesn't exist. Which involves first calling describe_log_groups (which has a much higher quota) and if the log group in question doesn't exist, then create it. This is what we do in MWAA and this is what the Watchtower package should be doing as well. And in fact, it is what they do here since v2.0.0. And I recently upgraded Airflow to use v2.0.1 here. So this problem should already be solved!

@nik-davis nik-davis Jan 14, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing, thank you for the detailed response. We're a bit behind latest and have Airflow v2.1.2 with Watchtower v1.0.6 installed, so sounds like we just need to upgrade. Thanks again!

@o-nikolas o-nikolas Jan 14, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries :)
Let me know if you spot any other possible issues!

CLOUDWATCH_REMOTE_HANDLERS: Dict[str, Dict[str, str]] = {
'task': {
'class': 'airflow.providers.amazon.aws.log.cloudwatch_task_handler.CloudwatchTaskHandler',
'formatter': 'airflow',
'base_log_folder': str(os.path.expanduser(BASE_LOG_FOLDER)),
'log_group_arn': urlparse(REMOTE_BASE_LOG_FOLDER).netloc,
'filename_template': FILENAME_TEMPLATE,
'create_log_group': create_log_group,
'create_log_stream': create_log_stream,
},
}

Expand Down
14 changes: 14 additions & 0 deletions airflow/config_templates/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -620,6 +620,20 @@
type: string
example: ~
default: "8793"
- name: create_cloudwatch_log_group
description: |
Automatically create Cloudwatch log group for Cloudwatch logs
version_added: 2.3.0
type: string
example: ~
default: "True"
- name: create_cloudwatch_log_stream
description: |
Automatically create Cloudwatch log stream for Cloudwatch logs
version_added: 2.3.0
type: string
example: ~
default: "True"
- name: metrics
description: |
StatsD (https://github.com/etsy/statsd) integration settings.
Expand Down
6 changes: 6 additions & 0 deletions airflow/config_templates/default_airflow.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,12 @@ extra_logger_names =
# visible from the main web server to connect into the workers.
worker_log_server_port = 8793

# Automatically create Cloudwatch log group for Cloudwatch logs
create_cloudwatch_log_group = True

# Automatically create Cloudwatch log stream for Cloudwatch logs
create_cloudwatch_log_stream = True

[metrics]

# StatsD (https://github.com/etsy/statsd) integration settings.
Expand Down
17 changes: 16 additions & 1 deletion airflow/providers/amazon/aws/log/cloudwatch_task_handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,16 +43,29 @@ class CloudwatchTaskHandler(FileTaskHandler, LoggingMixin):
:type log_group_arn: str
:param filename_template: template for file name (local storage) or log stream name (remote)
:type filename_template: str
:param create_log_group: whether to create Cloudwatch log group
:type create_log_group: bool
:param create_log_stream: whether to create Cloudwatch log stream
:type create_log_stream: bool
"""

def __init__(self, base_log_folder: str, log_group_arn: str, filename_template: str):
def __init__(
self,
base_log_folder: str,
log_group_arn: str,
filename_template: str,
create_log_group: bool = True,
create_log_stream: bool = True,
):
super().__init__(base_log_folder, filename_template)
split_arn = log_group_arn.split(':')

self.handler = None
self.log_group = split_arn[6]
self.region_name = split_arn[3]
self.closed = False
self.create_log_group = create_log_group
self.create_log_stream = create_log_stream

@cached_property
def hook(self):
Expand Down Expand Up @@ -82,6 +95,8 @@ def set_context(self, ti):
log_group=self.log_group,
stream_name=self._render_filename(ti, ti.try_number),
boto3_session=self.hook.get_session(self.region_name),
create_log_group=self.create_log_group,
create_log_stream=self.create_log_stream,
)

def close(self):
Expand Down
5 changes: 5 additions & 0 deletions tests/providers/amazon/aws/log/test_cloudwatch_task_handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,11 @@ def test_handler(self):
self.cloudwatch_task_handler.set_context(self.ti)
assert isinstance(self.cloudwatch_task_handler.handler, CloudWatchLogHandler)

def test_create_log_stream(self):
handler = self.cloudwatch_task_handler
handler.set_context(self.ti)
assert handler.handler.create_log_stream is True

def test_write(self):
handler = self.cloudwatch_task_handler
handler.set_context(self.ti)
Expand Down