[AIRFLOW-6569] Flush pending Sentry exceptions before exiting forked process #7232

mikeclarke · 2020-01-21T16:30:37Z

After switching to os.fork() for the task runner, there is the possibility that exceptions queued by Sentry will not be emitted prior to the process exiting.

This fixes AIRFLOW-6569 by explicitly flushing pending exceptions prior to calling os._exit() within the forked task runner.

This is covered by existing unit tests within tests/task/task_runner/test_standard_task_runner.py.

Issue link: AIRFLOW-6569

Make sure to mark the boxes below before creating PR: [x]

Description above provides context of the change
Commit message/PR title starts with [AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID^*
Unit tests coverage for changes (not needed for documentation changes)
Commits follow "How to write a good git commit message"
Relevant documentation is updated including usage instructions.
I will engage committers as explained in Contribution Workflow Example.

^* For document-only changes commit message can start with [AIRFLOW-XXXX].

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.
Read the Pull Request Guidelines for more information.

boring-cyborg · 2020-01-21T16:30:41Z

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/master/CONTRIBUTING.rst)

Here are some useful points:

Pay attention to the quality of your code (flake8, pylint 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](https://github
.com/apache/airflow/blob/master/docs/howto/custom-operator.rst) 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.

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://apache-airflow-slack.herokuapp.com/

robinedwards · 2020-01-21T17:03:38Z

Hahah I was just working on an almost identical solution. Nice one :-)

airflow/task/task_runner/standard_task_runner.py

codecov-io · 2020-02-04T06:19:07Z

Codecov Report

Merging #7232 into master will increase coverage by 54.78%.
The diff coverage is 22.22%.

@@             Coverage Diff             @@
##           master    #7232       +/-   ##
===========================================
+ Coverage   33.23%   88.01%   +54.78%     
===========================================
  Files         935      935               
  Lines       45170    45177        +7     
===========================================
+ Hits        15010    39764    +24754     
+ Misses      30160     5413    -24747

Impacted Files	Coverage Δ
airflow/task/task_runner/standard_task_runner.py	`64.06% <0.00%> (+41.11%)`	⬆️
airflow/sentry.py	`87.01% <50.00%> (+32.21%)`	⬆️
airflow/cli/cli_parser.py	`97.24% <0.00%> (+0.91%)`	⬆️
.../providers/google/cloud/operators/presto_to_gcs.py	`38.66% <0.00%> (+2.66%)`	⬆️
airflow/executors/celery_executor.py	`88.51% <0.00%> (+3.37%)`	⬆️
airflow/utils/decorators.py	`90.47% <0.00%> (+4.76%)`	⬆️
airflow/models/base.py	`100.00% <0.00%> (+6.25%)`	⬆️
airflow/exceptions.py	`100.00% <0.00%> (+6.66%)`	⬆️
airflow/www/forms.py	`100.00% <0.00%> (+6.77%)`	⬆️
airflow/utils/timeout.py	`76.00% <0.00%> (+8.00%)`	⬆️
... and 807 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0dafdd0...212485b. Read the comment docs.

ashb · 2020-02-05T17:32:05Z

Does this need a specific version of the sentry library to work?

airflow/sentry.py

stale · 2020-03-21T20:28:32Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

robinedwards · 2020-04-06T09:22:14Z

Any chance you could rebase this again? Cheers

mikeclarke · 2020-04-06T16:53:59Z

Rebased again, thanks for the heads up.

maxcountryman · 2020-05-19T03:23:46Z

I believe the bug this patch addresses is actively preventing us from receiving alerts in production.

will-misslin · 2020-05-19T15:19:40Z

I believe the bug this patch addresses is actively preventing us from receiving alerts in production.

I second this affecting our alerting.

kellyldougan · 2020-05-19T18:53:26Z

When can we expect this bug fix to get merged?

mik-laj · 2020-05-19T19:12:04Z

@tiopi @AbhiPrasad Can I ask for review? Can you share your experience with applications that use fork and Sentry?

AbhiPrasad · 2020-05-20T11:50:14Z

Yeah these changes LGTM, and are in line with how we have to use the python sdk until we add extended multiprocessing support. We should merge after rebase if tests pass.

mikeclarke · 2020-05-20T18:04:59Z

Rebased master just now - last time the tests were failing for unrelated reasons, but we'll see.

maxcountryman · 2020-05-22T14:47:17Z

It seems like the tests that are failing are unrelated to this patch and seem to be the same tests that are also failing on master.

What's blocking merging this in at this point?

mik-laj · 2020-05-22T17:53:22Z

Can you do rebase to the latest master? I would like to accept these changes, but I need green tests (excluding quarantine)

Fixes AIRFLOW-6569 by explicitly flushing pending exceptions prior to calling `os._exit` within the forked task runner.

mikeclarke · 2020-05-22T21:28:45Z

Can you do rebase to the latest master? I would like to accept these changes, but I need green tests (excluding quarantine)

Rebased again, thanks.

boring-cyborg · 2020-05-23T06:34:37Z

Awesome work, congrats on your first merged pull request!

nacosta · 2020-05-26T18:51:50Z

@mikeclarke thanks a lot for the fix.

@mik-laj , do you know if this fix will go out in the next release 1.10.11?

Many thanks

mik-laj · 2020-05-26T21:09:34Z

@kaxil do you have any information about it?

kaxil · 2020-05-27T10:46:30Z

Yes can be included in 1.10.11

ad-m · 2020-05-27T10:53:30Z

@kaxil , thank you for information and add to milestone "Airflow 1.10.11".

Fixes AIRFLOW-6569 by explicitly flushing pending exceptions prior to calling `os._exit` within the forked task runner. (cherry picked from commit db70da2)

boring-cyborg bot added the area:Scheduler including HA (high availability) scheduler label Jan 21, 2020

robinedwards reviewed Jan 21, 2020

View reviewed changes

airflow/task/task_runner/standard_task_runner.py Show resolved Hide resolved

turbaszek reviewed Feb 3, 2020

View reviewed changes

airflow/task/task_runner/standard_task_runner.py Outdated Show resolved Hide resolved

turbaszek approved these changes Feb 3, 2020

View reviewed changes

ashb reviewed Feb 5, 2020

View reviewed changes

airflow/sentry.py Outdated Show resolved Hide resolved

stale bot added stale Stale PRs per the .github/workflows/stale.yml policy file and removed stale Stale PRs per the .github/workflows/stale.yml policy file labels Mar 21, 2020

mikeclarke force-pushed the fix/flush-sentry-errors-AIRFLOW-6569 branch from 3408e42 to 212485b Compare April 6, 2020 16:53

mikeclarke force-pushed the fix/flush-sentry-errors-AIRFLOW-6569 branch 3 times, most recently from 261facf to 7b46d39 Compare April 10, 2020 21:52

mikeclarke mentioned this pull request May 19, 2020

Airflow native integration getsentry/sentry_airflow#36

Open

mikeclarke force-pushed the fix/flush-sentry-errors-AIRFLOW-6569 branch from 7b46d39 to 1d19d01 Compare May 20, 2020 18:04

maxcountryman mentioned this pull request May 20, 2020

Sentry tags appear to be missing #8243

Closed

Flush pending Sentry exceptions before exiting

b113457

Fixes AIRFLOW-6569 by explicitly flushing pending exceptions prior to calling `os._exit` within the forked task runner.

mikeclarke force-pushed the fix/flush-sentry-errors-AIRFLOW-6569 branch from 1d19d01 to b113457 Compare May 22, 2020 21:28

ad-m approved these changes May 22, 2020

View reviewed changes

mik-laj approved these changes May 23, 2020

View reviewed changes

mik-laj merged commit db70da2 into apache:master May 23, 2020

kaxil added this to the Airflow 1.10.11 milestone May 27, 2020

kaxil pushed a commit that referenced this pull request Jun 22, 2020

Flush pending Sentry exceptions before exiting (#7232)

3368cc4

Fixes AIRFLOW-6569 by explicitly flushing pending exceptions prior to calling `os._exit` within the forked task runner. (cherry picked from commit db70da2)

potiuk pushed a commit that referenced this pull request Jun 29, 2020

Flush pending Sentry exceptions before exiting (#7232)

1d816cd

Fixes AIRFLOW-6569 by explicitly flushing pending exceptions prior to calling `os._exit` within the forked task runner. (cherry picked from commit db70da2)

kaxil added the type:improvement Changelog: Improvements label Jul 1, 2020

kaxil pushed a commit that referenced this pull request Jul 1, 2020

Flush pending Sentry exceptions before exiting (#7232)

37c769c

Fixes AIRFLOW-6569 by explicitly flushing pending exceptions prior to calling `os._exit` within the forked task runner. (cherry picked from commit db70da2)

ncolomer added a commit to ncolomer/airflow that referenced this pull request Oct 20, 2020

Backport patch apache#7232 into 1.10.9

292df71

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-6569] Flush pending Sentry exceptions before exiting forked process #7232

[AIRFLOW-6569] Flush pending Sentry exceptions before exiting forked process #7232

mikeclarke commented Jan 21, 2020 •

edited by boring-cyborg bot

Loading

boring-cyborg bot commented Jan 21, 2020

robinedwards commented Jan 21, 2020

codecov-io commented Feb 4, 2020 •

edited

Loading

ashb commented Feb 5, 2020

stale bot commented Mar 21, 2020

robinedwards commented Apr 6, 2020

mikeclarke commented Apr 6, 2020

maxcountryman commented May 19, 2020

will-misslin commented May 19, 2020

kellyldougan commented May 19, 2020

mik-laj commented May 19, 2020

AbhiPrasad commented May 20, 2020

mikeclarke commented May 20, 2020

maxcountryman commented May 22, 2020

mik-laj commented May 22, 2020

mikeclarke commented May 22, 2020

boring-cyborg bot commented May 23, 2020

nacosta commented May 26, 2020

mik-laj commented May 26, 2020

kaxil commented May 27, 2020

ad-m commented May 27, 2020

[AIRFLOW-6569] Flush pending Sentry exceptions before exiting forked process #7232

[AIRFLOW-6569] Flush pending Sentry exceptions before exiting forked process #7232

Conversation

mikeclarke commented Jan 21, 2020 • edited by boring-cyborg bot Loading

boring-cyborg bot commented Jan 21, 2020

robinedwards commented Jan 21, 2020

codecov-io commented Feb 4, 2020 • edited Loading

Codecov Report

ashb commented Feb 5, 2020

stale bot commented Mar 21, 2020

robinedwards commented Apr 6, 2020

mikeclarke commented Apr 6, 2020

maxcountryman commented May 19, 2020

will-misslin commented May 19, 2020

kellyldougan commented May 19, 2020

mik-laj commented May 19, 2020

AbhiPrasad commented May 20, 2020

mikeclarke commented May 20, 2020

maxcountryman commented May 22, 2020

mik-laj commented May 22, 2020

mikeclarke commented May 22, 2020

boring-cyborg bot commented May 23, 2020

nacosta commented May 26, 2020

mik-laj commented May 26, 2020

kaxil commented May 27, 2020

ad-m commented May 27, 2020

mikeclarke commented Jan 21, 2020 •

edited by boring-cyborg bot

Loading

codecov-io commented Feb 4, 2020 •

edited

Loading