-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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.
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
[AIRFLOW-4741] Include Sentry for Airflow #5382
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes needed to the code as it stands, but I'm not sure I like the money-patching approach.
Does this PR even work? Nothing is loading the SentryHook to add the integrations.
Could it instead be attached to the existing task success/failure callbacks?
airflow/contrib/hooks/sentry_hook.py
Outdated
|
||
original_task_init = TaskInstance.__init__ | ||
original_clear_xcom = TaskInstance.clear_xcom_data | ||
SCOPE_TAGS = frozenset(("task_id", "dag_id", "execution_date", "ds", "operator")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why execution_date and ds? Why operator too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed ds, so it will only be execution_date and operator. These are to keep tags of tasks that fail. These tags are helpful to group and know whether the failure is due to an operator or perhaps just the execution date.
airflow/contrib/hooks/sentry_hook.py
Outdated
init(dsn=self.dsn, integrations=integrations) | ||
|
||
if not getattr(TaskInstance, "_sentry_integration_", False): | ||
TaskInstance.__init__ = add_sentry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing this doesn't feel very clean. Is there another way?
Where is this function actually called from too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function patches the init function of a TaskInstance. I went for the monkey patching route since I believe that someone who is using airflow does not need any of the sentry code. However, if they do opt in by adding the DSN to the connections, then the code is added to the init function of the tasks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am very glad that this integration appears. I have doubts whether the current implementation is good. I think the init logic of this code does not necessarily have to be in the hook. I think that it is not a problem to initiate this integration in the core.
I also think that hooking methods in this way is not a good idea. This can be very confusing to other people.
Make sure you have checked all steps below.
Jira
Description
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.
Tests
Commits
Documentation
Code Quality
flake8