Skip to content
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

Remove superfluous @Sentry.enrich_errors #37002

Merged
merged 2 commits into from
Jan 26, 2024
Merged

Conversation

jkramer-ginkgo
Copy link
Contributor

Remove superfluous @Sentry.enrich_errors from TaskInstance._schedule_downstream_tasks

A recent refactoring broke the contract enrich_errors expect. This decorator is being removed because it's not desired to log Sentry errors for scheduling issues.

Closes: #36957
Closes: #36968 [this PR is an alternate implementation]


…le_downstream_tasks`

A recent refactoring broke the contract `enrich_errors` expect. This decorator
is being removed because it's not desired to log Sentry errors for scheduling issues.
@jkramer-ginkgo
Copy link
Contributor Author

it's not desired to log Sentry errors for scheduling issues.

I'd appreciate if someone could confirm my assumption

Copy link
Contributor

@dirrao dirrao left a comment

Choose a reason for hiding this comment

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

This is a breaking change. Need to check with sentry experts on this.
Meanwhile you can look into the failing test cases.

@jkramer-ginkgo
Copy link
Contributor Author

@dirrao Thank you for taking a look. The static and mypy failure appear completely unrelated to this change, so I'm wondering if it should fall in the scope of this PR. Also, I couldn't reproduce the integration test failure locally so I'm wondering if a retry will resolve.

@dirrao
Copy link
Contributor

dirrao commented Jan 26, 2024

@dirrao Thank you for taking a look. The static and mypy failure appear completely unrelated to this change, so I'm wondering if it should fall in the scope of this PR. Also, I couldn't reproduce the integration test failure locally so I'm wondering if a retry will resolve.

If they unrelated, ignore them. However the retry should pass those test cases.

@jkramer-ginkgo
Copy link
Contributor Author

Retry passed the integration test. Static/mypy checks are unrelated to this PR

@potiuk
Copy link
Member

potiuk commented Jan 26, 2024

Let's see. Rebased the PR. For the future @jkramer-ginkgo - the best way to check failing checks is to rebase. Super easy in GitHub - with Rebase button.

@potiuk
Copy link
Member

potiuk commented Jan 26, 2024

Yes. The MyPy issues are (somewhat) releated seems that your changes triggered it. Unfortunately MyPy checks are based on heuristics, and it sometimes only starts detecting issues after changes in loosely related places, I am afraid the only way for you to fix it, is to fix the detected error in your PR, even if you think it's not related.

Courtesy of MyPy, there is not much we can do with it other than asking people who (rarely) hit this kind of issue - to fix it in their PR.

Resolves MyPy error
@potiuk potiuk added this to the Airflow 2.8.2 milestone Jan 26, 2024
@potiuk potiuk merged commit 45b6b7a into apache:main Jan 26, 2024
56 checks passed
@ephraimbuddy ephraimbuddy added the type:bug-fix Changelog: Bug Fixes label Feb 19, 2024
ephraimbuddy pushed a commit that referenced this pull request Feb 20, 2024
* Remove superfluous `@Sentry.enrich_errors` from `TaskInstance._schedule_downstream_tasks`

A recent refactoring broke the contract `enrich_errors` expect. This decorator
is being removed because it's not desired to log Sentry errors for scheduling issues.

* Fix incorrect parameter name

Resolves MyPy error

(cherry picked from commit 45b6b7a)
ephraimbuddy pushed a commit that referenced this pull request Feb 22, 2024
* Remove superfluous `@Sentry.enrich_errors` from `TaskInstance._schedule_downstream_tasks`

A recent refactoring broke the contract `enrich_errors` expect. This decorator
is being removed because it's not desired to log Sentry errors for scheduling issues.

* Fix incorrect parameter name

Resolves MyPy error

(cherry picked from commit 45b6b7a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scheduler exceptions when Sentry and schedule_after_task_execution are enabled
4 participants