[AIRFLOW-4741] Include Sentry into core Airflow #5407

tiopi · 2019-06-11T23:31:03Z

Make sure you have checked all steps below.

Jira

My PR addresses the following Airflow Jira issues and references them in the PR title:

https://issues.apache.org/jira/browse/AIRFLOW-4741

Description

Here are some details about my PR, including screenshots of any UI changes:
This PR is to add Sentry SDK functionality to Airflow. The functionality includes adding tags per task event and breadcrumbs of previous tasks that ran on the DAG.

Differences between #5416 and #5407:
#5416: PR focuses on adding Sentry functionality to specific DAGs. This is achieved by adding a sentry hook to the contrib folder. Users can then simply call the hook with their SENTRY_DSN for each DAG.
#5407: PR focuses on adding Sentry to the Airflow ecosystem. Its aim is to handle all errors within airflow (Scheduler, worker, DAGs, tasks). (This is the continuation of #5382, which I closed by accident.)

Tests

My PR adds unit tests for sentry.py.

Commits

My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation under error.rst.

Code Quality

Passes flake8

pgagnon · 2019-06-12T17:30:07Z

@tiopi CI is sad due to a couple of pylint errors: https://travis-ci.org/apache/airflow/jobs/544497697

edit: Wrong mentions. Sorry! My github UI bugged out. 😥

codecov-io · 2019-06-14T00:25:41Z

Codecov Report

❗ No coverage uploaded for pull request base (master@7506c95). Click here to learn what that means.
The diff coverage is 87.34%.

@@            Coverage Diff            @@
##             master    #5407   +/-   ##
=========================================
  Coverage          ?   79.82%           
=========================================
  Files             ?      609           
  Lines             ?    35126           
  Branches          ?        0           
=========================================
  Hits              ?    28040           
  Misses            ?     7086           
  Partials          ?        0

Impacted Files	Coverage Δ
airflow/models/taskinstance.py	`93.75% <100%> (ø)`
airflow/models/dag.py	`91.7% <60%> (ø)`
airflow/sentry.py	`88.88% <88.88%> (ø)`

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 7506c95...fe0af33. Read the comment docs.

tiopi · 2019-06-14T20:40:09Z

@tiopi CI is sad due to a couple of pylint errors: https://travis-ci.org/apache/airflow/jobs/544497697

edit: Wrong mentions. Sorry! My github UI bugged out. 😥

I got the tests to finally pass!

pgagnon

Cursory glance.

docs/errors.rst

scripts/ci/pylint_todo.txt

airflow/config_templates/default_airflow.cfg

airflow/sentry.py

houqp · 2019-07-18T17:09:27Z

Is this PR still blocked on any requested change? If so, I would like to help so it can be merged for our internal use.

ashb · 2019-07-18T18:34:32Z

It's blocked on me finding the time to review it (but a lot of my available time has been taking up with Release Management work).

setup.py

airflow/sentry.py

houqp · 2019-08-05T23:57:22Z

Looks like all review feedbacks have been addressed, any blocker I could help resolve to get it merged?

tiopi · 2019-08-21T23:48:10Z

cc @jmcarp @ashb

pgagnon

I have done a couple of passes on this and it's LGTM to me at this point barring a minor doc nit.

@ashb WDYT?

docs/errors.rst

ashb

Sorry it took me so long to review this.

I have some questions about the data we sent to Sentry that I think would be cleared up by some example screenshots.

airflow/sentry.py

docs/errors.rst

houqp · 2019-09-18T23:11:03Z

we are almost there! @tiopi let me know if there is anything I can help to get it over the finish line.

airflow/models/taskinstance.py

airflow/sentry.py

docs/errors.rst

airflow/sentry.py

This commit intends to add Sentry to the core functionality of Airflow. It takes an approach like Statsd for simple integration. The commit makes use of tagging and breadcrumbs for more error information. Add testing for tagging and breadcrumbs. Add documentation for Airflow Error tracking.

… and fixed documentation.

ashb

It took many months (sorry!) but we got there in the end!

houqp · 2019-09-24T17:06:18Z

great job @tiopi!

This commit intends to add Sentry to the core functionality of Airflow. It takes an approach like Statsd for simple integration. The commit makes use of tagging and breadcrumbs for more error information. (cherry picked from commit dc3bfe4)

tiopi mentioned this pull request Jun 13, 2019

[AIRFLOW-4792] Include Sentry into Contrib Airflow #5416

Closed

6 tasks

tiopi force-pushed the add_sentry branch 2 times, most recently from 38378a9 to 6dd09d5 Compare June 14, 2019 19:53

tiopi force-pushed the add_sentry branch 2 times, most recently from a070b4b to b7a1c76 Compare June 17, 2019 22:46

pgagnon reviewed Jun 18, 2019

View reviewed changes

docs/errors.rst Outdated Show resolved Hide resolved

scripts/ci/pylint_todo.txt Outdated Show resolved Hide resolved

tiopi force-pushed the add_sentry branch 2 times, most recently from 7cdf31f to 069d34d Compare June 18, 2019 22:57

pgagnon reviewed Jun 19, 2019

View reviewed changes

airflow/config_templates/default_airflow.cfg Outdated Show resolved Hide resolved

airflow/sentry.py Outdated Show resolved Hide resolved

tiopi force-pushed the add_sentry branch from 069d34d to b675f21 Compare June 19, 2019 21:58

mik-laj reviewed Jun 20, 2019

View reviewed changes

airflow/sentry.py Outdated Show resolved Hide resolved

tiopi force-pushed the add_sentry branch from b675f21 to d63b29e Compare June 20, 2019 06:33

pgagnon reviewed Jun 26, 2019

View reviewed changes

airflow/sentry.py Outdated Show resolved Hide resolved

airflow/sentry.py Show resolved Hide resolved

airflow/sentry.py Outdated Show resolved Hide resolved

tiopi force-pushed the add_sentry branch 5 times, most recently from 83cd366 to 7f296b0 Compare July 2, 2019 07:57

untitaker reviewed Jul 2, 2019

View reviewed changes

airflow/sentry.py Show resolved Hide resolved

tiopi mentioned this pull request Jul 3, 2019

Does not capture errors from reading config file getsentry/sentry_airflow#5

Closed

tiopi force-pushed the add_sentry branch from 7f296b0 to 058e6f2 Compare July 10, 2019 18:22

ashb reviewed Jul 18, 2019

View reviewed changes

setup.py Show resolved Hide resolved

jmcarp reviewed Jul 20, 2019

View reviewed changes

airflow/sentry.py Outdated Show resolved Hide resolved

jmcarp reviewed Jul 20, 2019

View reviewed changes

airflow/sentry.py Outdated Show resolved Hide resolved

jmcarp reviewed Jul 20, 2019

View reviewed changes

airflow/sentry.py Outdated Show resolved Hide resolved

tiopi force-pushed the add_sentry branch 2 times, most recently from 374c344 to ec878b2 Compare July 23, 2019 22:11

tiopi force-pushed the add_sentry branch 2 times, most recently from 326af63 to 99ada8c Compare August 16, 2019 07:46

pgagnon reviewed Aug 27, 2019

View reviewed changes

docs/errors.rst Show resolved Hide resolved

ashb requested changes Aug 29, 2019

View reviewed changes

tiopi force-pushed the add_sentry branch from ecc80b4 to a4828c1 Compare September 9, 2019 18:16

ashb requested changes Sep 23, 2019

View reviewed changes

Osmar Coronel and others added 5 commits September 23, 2019 12:37

Remove Tornado integration

4c2f2a0

Fixed docs

9af15c9

Fixed flake and docs styling

23356c8

Changed variable type, changed wrapper function name, added more tags…

3cf3550

… and fixed documentation.

tiopi force-pushed the add_sentry branch from 3af88ef to 3cf3550 Compare September 23, 2019 17:39

Fixed tests

fe0af33

tiopi requested a review from ashb September 23, 2019 20:14

ashb approved these changes Sep 24, 2019

View reviewed changes

ashb merged commit dc3bfe4 into apache:master Sep 24, 2019

gravesm mentioned this pull request Nov 22, 2019

Add sentry support MITLibraries/workflow#19

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-4741] Include Sentry into core Airflow #5407

[AIRFLOW-4741] Include Sentry into core Airflow #5407

tiopi commented Jun 11, 2019 •

edited

Loading

pgagnon commented Jun 12, 2019 •

edited

Loading

codecov-io commented Jun 14, 2019 •

edited

Loading

tiopi commented Jun 14, 2019

pgagnon left a comment

houqp commented Jul 18, 2019

ashb commented Jul 18, 2019

houqp commented Aug 5, 2019

tiopi commented Aug 21, 2019

pgagnon left a comment

ashb left a comment

houqp commented Sep 18, 2019

ashb left a comment

houqp commented Sep 24, 2019

[AIRFLOW-4741] Include Sentry into core Airflow #5407

[AIRFLOW-4741] Include Sentry into core Airflow #5407

Conversation

tiopi commented Jun 11, 2019 • edited Loading

Jira

Description

Tests

Commits

Documentation

Code Quality

pgagnon commented Jun 12, 2019 • edited Loading

codecov-io commented Jun 14, 2019 • edited Loading

Codecov Report

tiopi commented Jun 14, 2019

pgagnon left a comment

Choose a reason for hiding this comment

houqp commented Jul 18, 2019

ashb commented Jul 18, 2019

houqp commented Aug 5, 2019

tiopi commented Aug 21, 2019

pgagnon left a comment

Choose a reason for hiding this comment

ashb left a comment

Choose a reason for hiding this comment

houqp commented Sep 18, 2019

ashb left a comment

Choose a reason for hiding this comment

houqp commented Sep 24, 2019

tiopi commented Jun 11, 2019 •

edited

Loading

pgagnon commented Jun 12, 2019 •

edited

Loading

codecov-io commented Jun 14, 2019 •

edited

Loading