Fix double logging with some task logging handler #27591
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ashb
added
type:bug-fix
boring-cyborg
bot
added
area:logging
area:providers
provider:amazon-aws
2 tasks
potiuk
reviewed
Nov 10, 2022
ephraimbuddy
approved these changes
Nov 10, 2022
potiuk
reviewed
Nov 10, 2022
|
|
||
|
|
||
| def __getattr__(name): | ||
| if name in ("DISABLE_PROPOGATE", "DISABLE_PROPAGATE"): |
There was a problem hiding this comment.
We all knew I couldn't spell right?
(The switch to an Enum was for typing reasons, no easy way of saying "returns none or this specific object type")
There was a problem hiding this comment.
Looking forward to the depreciation warning hahahaha :D
There was a problem hiding this comment.
Luckily in this case I don't think we need one: it was only for a single point release, and it didn't really work in practice
😅
There was a problem hiding this comment.
But I want to see you suffer 😆 You need to learn from those mistakes 😆
potiuk
approved these changes
Nov 10, 2022
|
Nice |
jedcunningham
approved these changes
Nov 10, 2022
|
It appears my PR skills have attrophied to the point where I'm just making silly mistakes :D |
A previous change to fix disappearing log messages turned on propagation for the `airflow.task` logger, and disabled it again after `set_context()` was called, but only if that function returned a special sentinel value. For the "core" task log handlers we returned them, but some providers weren't "correctly" subclassing and weren't returning this sentinel value. The fix here is to change the logic from disable only on speical value to disable by default and maintain propagation on special value; this means that if a handler doesn't return the value from `super()` (or if they don't even subclass the default handler) propagation will still be disabled by default.
…ore Secrets Backend (apache#27134)
ashb
force-pushed
the
prevent-double-logging-some-task-handlers
branch
from
4820ec6 to
d7f50d8
Compare
ephraimbuddy
pushed a commit
that referenced
this pull request
Nov 10, 2022
A previous change to fix disappearing log messages turned on propagation for the `airflow.task` logger, and disabled it again after `set_context()` was called, but only if that function returned a special sentinel value. For the "core" task log handlers we returned them, but some providers weren't "correctly" subclassing and weren't returning this sentinel value. The fix here is to change the logic from disable only on special value to disable by default and maintain propagation on special value; this means that if a handler doesn't return the value from `super()` (or if they don't even subclass the default handler) propagation will still be disabled by default. (cherry picked from commit 933fefc)
ephraimbuddy
pushed a commit
that referenced
this pull request
Nov 10, 2022
A previous change to fix disappearing log messages turned on propagation for the `airflow.task` logger, and disabled it again after `set_context()` was called, but only if that function returned a special sentinel value. For the "core" task log handlers we returned them, but some providers weren't "correctly" subclassing and weren't returning this sentinel value. The fix here is to change the logic from disable only on special value to disable by default and maintain propagation on special value; this means that if a handler doesn't return the value from `super()` (or if they don't even subclass the default handler) propagation will still be disabled by default. (cherry picked from commit 933fefc)
18 tasks
Adityamalik123
pushed a commit
to Adityamalik123/airflow
that referenced
this pull request
Nov 12, 2022
A previous change to fix disappearing log messages turned on propagation for the `airflow.task` logger, and disabled it again after `set_context()` was called, but only if that function returned a special sentinel value. For the "core" task log handlers we returned them, but some providers weren't "correctly" subclassing and weren't returning this sentinel value. The fix here is to change the logic from disable only on special value to disable by default and maintain propagation on special value; this means that if a handler doesn't return the value from `super()` (or if they don't even subclass the default handler) propagation will still be disabled by default.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A previous change to fix disappearing log messages turned on propagation for the
airflow.tasklogger, and disabled it again afterset_context()was called, but only if that function returned a special sentinel value.For the "core" task log handlers we returned them, but some providers weren't "correctly" subclassing and weren't returning this sentinel value.
The fix here is to change the logic from disable only on special value to disable by default and maintain propagation on special value; this means that if a handler doesn't return the value from
super()(or if they don't even subclass the default handler) propagation will still be disabled by default.Fixes #27345