Skip to content

Fix Cloudwatch remote logging #48774

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

Jump to bottom
Merged
ashb merged 1 commit into from
Apr 4, 2025
Merged

Fix Cloudwatch remote logging #48774

ashb merged 1 commit into from
Apr 4, 2025

Conversation

@o-nikolas
Copy link
Contributor

@o-nikolas o-nikolas commented Apr 3, 2025 •
edited by ashb
Loading

There were three main issues:

  1. A circular loop that eventually fails due to call depth exceeded. This is because the handler was lazily initted during the first log emission. But when the handler is created some code down stream tries to log, and since there is no handler yet (because we're in the middle of creating it), it tries to create another one, and we were spinning ad infinitum.
  2. The stream name is not set on read, because we don't call set_context anywhere in the SDK path, and the processor doesn't have access to the TI anyway (which is used for the stream name). So a 0 byte stream name was being used and was causing a failure in Watchtower.
  3. read is also failing because it is using the relative_path as the stream name, which is almost right, but the name isn't sanitized (there are some characters that cloudwatch doesn't allow in a stream name). set_context used to sanitize the name and set it, but it isn't called in the SDK path.

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

@o-nikolas
Fix Cloudwatch remote logging
ad86d22 
There were three main issues:

1) A circular loop that eventually fails due to call depth exceeded. This is because the handler was lazily initted during the first log emission. But when the handler is created some code down stream tries to log, and since there is no handler yet (because we're in the middle of creating it), it tries to create another one, and we were spinning ad infinitum.
2) The stream name is not set on read, because we don't call set_context anywhere in the SDK path, and the processor doesn't have access to the TI anyway (which is used for the stream name). So a 0 byte stream name was being used and was causing a failure in Watchtower.
3) read is also failing because it is using the relative_path as the stream name, which is almost right,  but the name isn't sanitized (there are some characters that cloudwatch doesn't allow in a stream name). set_context used to sanitize the name and set it, but it isn't called in the SDK path.
ashb
ashb approved these changes Apr 4, 2025
View reviewed changes
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

Thanks for catching this

@ashb ashb merged commit 5ca62b8 into apache:main Apr 4, 2025
70 checks passed
nailo2c pushed a commit to nailo2c/airflow that referenced this pull request Apr 4, 2025
@o-nikolas @nailo2c
Fix Cloudwatch remote logging (apache#48774)
07c7517 
There were three main issues:

1) A circular loop that eventually fails due to call depth exceeded. This is because the handler was lazily initted during the first log emission. But when the handler is created some code down stream tries to log, and since there is no handler yet (because we're in the middle of creating it), it tries to create another one, and we were spinning ad infinitum.
2) The stream name is not set on read, because we don't call set_context anywhere in the SDK path, and the processor doesn't have access to the TI anyway (which is used for the stream name). So a 0 byte stream name was being used and was causing a failure in Watchtower.
3) read is also failing because it is using the relative_path as the stream name, which is almost right,  but the name isn't sanitized (there are some characters that cloudwatch doesn't allow in a stream name). set_context used to sanitize the name and set it, but it isn't called in the SDK path.
@eladkal eladkal mentioned this pull request Apr 6, 2025
Closed
diogotrodrigues pushed a commit to diogotrodrigues/airflow that referenced this pull request Apr 6, 2025
@o-nikolas @diogotrodrigues
Fix Cloudwatch remote logging (apache#48774)
5c52388 
There were three main issues:

1) A circular loop that eventually fails due to call depth exceeded. This is because the handler was lazily initted during the first log emission. But when the handler is created some code down stream tries to log, and since there is no handler yet (because we're in the middle of creating it), it tries to create another one, and we were spinning ad infinitum.
2) The stream name is not set on read, because we don't call set_context anywhere in the SDK path, and the processor doesn't have access to the TI anyway (which is used for the stream name). So a 0 byte stream name was being used and was causing a failure in Watchtower.
3) read is also failing because it is using the relative_path as the stream name, which is almost right,  but the name isn't sanitized (there are some characters that cloudwatch doesn't allow in a stream name). set_context used to sanitize the name and set it, but it isn't called in the SDK path.
simonprydden pushed a commit to simonprydden/airflow that referenced this pull request Apr 8, 2025
@o-nikolas
Fix Cloudwatch remote logging (apache#48774)
a679578 
There were three main issues:

1) A circular loop that eventually fails due to call depth exceeded. This is because the handler was lazily initted during the first log emission. But when the handler is created some code down stream tries to log, and since there is no handler yet (because we're in the middle of creating it), it tries to create another one, and we were spinning ad infinitum.
2) The stream name is not set on read, because we don't call set_context anywhere in the SDK path, and the processor doesn't have access to the TI anyway (which is used for the stream name). So a 0 byte stream name was being used and was causing a failure in Watchtower.
3) read is also failing because it is using the relative_path as the stream name, which is almost right,  but the name isn't sanitized (there are some characters that cloudwatch doesn't allow in a stream name). set_context used to sanitize the name and set it, but it isn't called in the SDK path.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rawwar rawwar rawwar left review comments

@ashb ashb ashb approved these changes

@eladkal eladkal eladkal approved these changes

@kaxil kaxil Awaiting requested review from kaxil kaxil is a code owner

@amoghrajesh amoghrajesh Awaiting requested review from amoghrajesh amoghrajesh is a code owner

Assignees

No one assigned

Labels

area:logging area:providers area:task-sdk provider:amazon AWS/Amazon - related issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@o-nikolas @ashb @rawwar @eladkal